-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
auth-pam: Immediately report instructions to clients and fix handling in ssh client #452
base: master
Are you sure you want to change the base?
Conversation
52bca91
to
e007e0c
Compare
FYI this has been tested also with PuTTy, and it properly supports the changes without any modification. |
Thanks for raising this @3v1n0! I'm running into issues with PAM/SSH and would love to see your fix implemented. Hopefully this can gain some traction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at the PAM code. Some comments on these changes
misc.c
Outdated
@@ -3063,3 +3063,42 @@ lib_contains_symbol(const char *path, const char *s) | |||
return ret; | |||
#endif /* HAVE_NLIST_H */ | |||
} | |||
|
|||
ssize_t | |||
write_msg(const char *str, int flags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should use one of the utf8.c *mprintf()
functions here, otherwise you are writing arbitrary characters from a (usually fine but potentially untrustworthy) server to the user's tty. The logit()
calls that you're replacing do filter these using strnvis()
, but as you observe this is too restrictive wrt UTF8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this can be done with those indeed, but they look like too complex for this purpose, so I used another behavior but let me know if you prefer using *mprintf funcs.
misc.c
Outdated
} | ||
|
||
output = output_fileno; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this section is necessary or correct. I guess it's mimicking the behaviour of read_passphrase()
, but it isn't complete as that function will also call out to $SSH_ASKPASS
and has some additional logic besides.
You might be better off using the readpass.c notify_start()
API, as it tries to do all this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done too now, not sure if it can be done better.
auth-pam.c
Outdated
sshpam_free_ctx | ||
}; | ||
|
||
KbdintDevice mm_sshpam_device_legacy = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reiterating an earlier comment: I don't think we should add this unless there is a clear motivating case of a client that is broken by these changes. Do you have an example of one?
An alternative to adding a whole other method is to add a compat.c
pattern to detect these clients and automatically adapt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I indeed have no case for this, so we can drop it and if the problem shows for some older clients (most wouldn't work anyways since they don't support other features) then we can reiterate adding compatibility code.
Are you fine with that?
Another reason why this was added was not to change the behavior for some clients who are expecting the old way, mostly because if a client pre-this-PR will connect to a server with this PR, it may not receive valid UTF-8 chars output, and this could be an issue.
By providing an option, it's possible to use the old behavior. But that's the only case I may think we could create problems. So basically a client passing -o KbdInteractiveDevices=pam-legacy-instructions
can just get the old behavior even on newer servers.
In fact we've the situation in which some old clients want the new server behavior, but other clients may not want it because they may end up writing wrong utf-8 text.
I think implementing it without backwards compatibility is fine, we could
land this early in the release cycle and see if anyone finds breakage with
non-OpenSSH clients. If there are problems we can use a compat.[ch] hack to
turn it off for the broken clients somehow.
… Message ID: ***@***.***
com>
|
Oh, I see... Well I thought they were also more prone to errors, but will do.
There's very little type safety gained by enums, so it doesn't help as much
as we'd like :(
|
Fair... My only concerns is for those new clients (modulo this PR), that may still want the old behavior because otherwise they may get wrong utf-8 output, but if this is not a problem, we can avoid supporting |
On Wed, 6 Dec 2023 at 08:26, Marco Trevisan ***@***.***> wrote:
Fair... My only concerns is for those new clients (modulo this PR), that
may still want the old behavior because otherwise they may get wrong utf-8
output, but if this is not a problem, we can avoid supporting -o
KbdInteractiveDevices=pam-legacy-instructions too.
Wouldn't those clients either 1) not be using notification only PAM modules
because they don't work properly now or 2) using them before a module that
prompts and therefore be seeing the messed up UTF-8 (as the messages are
accumulated) already?
… Message ID: ***@***.***>
|
However, I've been conservative since there are tons of setups I may not have considered, and so providing working-already workaround for old clients seemed to me sane. But I'll drop this. :) |
8df6fea
to
1391eff
Compare
cf6e36a
to
5d3b8db
Compare
@djmdjm can you have a look again on that please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@3v1n0 Added a few questions inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks better now! The return value defines are not strictly necessary but I think they improve readability.
@djmdjm any chance you could have another look at this? Ubuntu plans to ship this in 24.04 and I'd very much prefer if we had an upstream approved fix instead of an out-of-tree patch. |
Is there any progress on this PR? We are writing customized multi-factor authentication pam module for ssh and hope to have the ability to prompt some message without requiring any user input during authentication. |
@djmdjm ping? |
The Samba team's Himmelblau (Azure Entra ID auth) depends on this. What is the hold up? |
kbdint result vfunc may return various values, so use an enum to make it clearer what each result means without having to dig into the struct documentation.
Makes things more readable and easier to extend
SSH keyboard-interactive authentication method supports instructions but sshd didn't show them until an user prompt was requested. This is quite inconvenient for various PAM modules that need to notify an user without requiring for their explicit input. So, properly implement RFC4256 making instructions to be shown to users when they are requested from PAM. Closes: https://bugzilla.mindrot.org/show_bug.cgi?id=2876
…utf-8 As per the previous server change now the keyboard-interactive service and instruction values could be reported as soon as they are available and so they're not prompts anymore and not parsed like them. While this was already supported by the SSH client, these messages were not properly written as the escaped sequences they contained were not correctly reported. So for example a message containing "\" was represented as "\\" and similarly for all the other C escape sequences. This was leading to more problems when it come to utf-8 chars, as they were only represented by their octal representation. This was easily testable by adding a line like the one below to the sshd PAM service: auth requisite pam_echo.so Hello SSHD! Want some 🍕? Which was causing this to be written instead: Hello SSHD! Want some \360\237\215\225? To handle this, instead of simply using fmprintf, we're using the notifier in a way can be exposed to users in the proper format and UI.
We were only checking if the prefix of a device name was matching what we had in the devices list, so if the device list contained "pam", then also the device "pam-foo" was matching.
99656ca
to
541850b
Compare
As it has been discussed some long time ago in bug #2876, I'm adding support for immediate reporting of instructions to clients as defined by RFC4256. There was a similar attempt on #337, however this implied few more changes (see each commit for further details), in particular:
Add an option to disable this behavior server-sidessh
client to properly handle the instructions, parsing them properly as per the standard formatI'm not fully sure if the server-side setting is needed given that SSHv1 is not supported anymore by sshd, but probably it still makes sense to disable this behavior form server-side, even though using the device selection is possible too now.