su, sudo: Check the user didn't change during PAM transaction

[3v1n0]

PAM modules can change the user during their execution, in such case,
sudo would still use the user that has been provided giving potentially
access to another user with the credentials of another one.

So prevent this to happen, by ensuring that the final PAM user is
matching the one which started the transaction

Similar to:

• login-utils/su-common: Check the user didn't change during PAM transaction util-linux/util-linux#3206
• plugins/pam: Check the user didn't change during PAM transaction sudo-project/sudo#412

[squell]

@3v1n0 Thanks for the PR 🙏 (and for providing the context in the form of PR's to su and sudo)! I've also requested @rnijveld to look at this for a high level perspective since he has spent the most time studying PAM.

I would also like to see an end-to-end test for this in our test framework; is there an example /etc/pam.d/sudo config out that we can use for that to save us some time?

[3v1n0]

I would also like to see an end-to-end test for this in our test framework; is there an example /etc/pam.d/sudo config out that we can use for that to save us some time?

I would like that too... But in order to get that we should also craft a module that does it, since AFAIK no module provided by default by unix-pam does this. Although it's technically possible for a third party one.

I'll look into that, assuming we want to have test-dependency on pam-rs or similar.

[squell]

Building our own PAM module (esp. if it's not on fairly solid grounds dependency-wise) that nobody should ever use to provide a negative test case probably goes too far.

I was more thinking along the lines of using a readily available module or a script with pam_exec. Of course this is a "nice to have", not a necessary condition for merging this particular PR.

[3v1n0]

I was more thinking along the lines of using a readily available module or a script with pam_exec. Of course this is a "nice to have", not a necessary condition for merging this particular PR.

AFAIK pam_exec doesn't allow to modify the environments, so no other debug module I'm aware of... :(

[rnijveld]

I am a little bit worried this removes functionality that doesn't need to be removed. What this protects against is misconfiguration where the sudo (or sudo-i) service file is misconfigured to ask for a completely unrelated username before running authentication modules, thus not respecting the user that sudo previously sets for which it wants authentication. By default no such module exists in PAM however, so a specifically made-for-purpose module would need to be made. Also, the username could change between any module anyway. So initially some module could ask for a different username, whereas a final module in the service configuration could restore the username to the previous value. All we know after authentication succeeded is that 'some' authentication succeeded, that is not necessarily related to the value of PAM_USER. Could you think of any other scenario that your code is meant to prevent, or would it just be this specific case of misconfiguration?

Much more likely are cases like pam_canonicalize_user, which purposely change the username to something else, in cases where the user database has an updated username for a specific user. The example I have in my head is a large enterprise organization which allows users to log in using their email-address. E-mail addresses aren't typically valid usernames in Linux, so the user database could be configured to change [email protected] to user_example_com for the username on your system, technically making the username different from the original one provided to sudo. The question for me would be: are there any legitimate use-cases of username changing which we remove by applying your patch? Specifically in more non-typical deployments.

Aside from all this: I'm always somewhat surprised by how loose usernames are in PAM and how little a PAM_SUCCESS tells us about what actually happened within PAM. This is all aside from PAM loading a whole bunch of modules inside our process space and hopefully those modules don't break our memory, hopefully they don't have memory leaks, hopefully they don't use too many resources etc etc. I personally would really like for an alternative to PAM in the future.

[3v1n0]

I am a little bit worried this removes functionality that doesn't need to be removed.

Mhmh, like?

What this protects against is misconfiguration where the sudo (or sudo-i) service file is misconfigured to ask for a completely unrelated username before running authentication modules, thus not respecting the user that sudo previously sets for which it wants authentication.

No, it has not to ask it, but a module may modify the user without being asked the user.

By default no such module exists in PAM however, so a specifically made-for-purpose module would need to be made. Also, the username could change between any module anyway.

Sure that is true, but at least it's a consistency check on the status, reason why this very same change is now part of C implementations of su and sudo.

Much more likely are cases like pam_canonicalize_user, which purposely change the username to something else, in cases where the user database has an updated username for a specific user. The example I have in my head is a large enterprise organization which allows users to log in using their email-address. E-mail addresses aren't typically valid usernames in Linux, so the user database could be configured to change [email protected] to user_example_com

Indeed that is a case I considered and tested, and this check is even more important for those, as this won't block that scenario because what happens will be:

• User is typed in the form you like (e.g. [email protected]) via sudo -u [email protected]
• The NSS module that is handling the user and that sudo calls will return the passwd entry for that user, but that will be a the new one (user_example_com)
  • The rename is something that must happen at this level, always as otherwise the returned user won't be found anyways on startup
• sudo now starts the PAM request with user_example_com, not the initial version
• PAM transaction is completed and the user still matches.

Now, in the case a PAM module renames an user in a different way that NSS knows, this shouldn't allow access.

But even in the case it's PAM changing the user, then if it matches the entry on the NSS db, then there's no problem at all.

(note that [email protected] is a fully legal username anyways)

for the username on your system, technically making the username different from the original one provided to sudo. The question for me would be: are there any legitimate use-cases of username changing which we remove by applying your patch? Specifically in more non-typical deployments.

They are legitimate... And indeed in a project we manage we do case conversion all the times, but this change won't prevent it to work, exactly because the transaction always starts with an user, but internally it's both NSS and - in case - PAM to do the conversion. All that it matters is that the two things agree.

Aside from all this: I'm always somewhat surprised by how loose usernames are in PAM and how little a PAM_SUCCESS tells us about what actually happened within PAM. This is all aside from PAM loading a whole bunch of modules inside our process space and hopefully those modules don't break our memory, hopefully they don't have memory leaks, hopefully they don't use too many resources etc etc. I personally would really like for an alternative to PAM in the future.

That's a true story, I'm still wondering how long will systemd still take, to take over PAM too :)

[rnijveld]

Indeed that is a case I considered and tested, and this check is even more important for those, as this won't block that scenario because what happens will be:

* User is typed in the form you like (e.g. `[email protected]`) via `sudo -u [email protected]`

* The NSS module that is handling the user and that sudo calls will return the `passwd` entry for that user, but that will be a the new one (`user_example_com`)
  
  * The rename is something that must happen at this level, always as otherwise the returned user won't be found anyways on startup

* sudo now starts the PAM request with `user_example_com`, not the initial version

* PAM transaction is completed and the user still matches.

Now, in the case a PAM module renames an user in a different way that NSS knows, this shouldn't allow access.

I think this was my main worry and answers all of my questions. But you're absolutely right that NSS and PAM should always agree on usernames. And given we always use a lookup via NSS (or maybe in the future the systemd varlink interface as an alternative) this check makes a whole bunch of sense. Even if it won't be able to actually verify what PAM did, it makes sense as a sanity check! Thanks!

[squell]

Thanks both for discussing this issue so thoroughly, and thanks for the contribution!