google / google-authenticator-libpam

Apache License 2.0
1.77k stars 281 forks source link

Process exits: failure to restore uid on Mac #111

Open NWilson opened 5 years ago

NWilson commented 5 years ago

On Mac, there is no setfsuid, so the module is using seteuid to control the UID of the filesystem checks.

If the user does not exist, then the "nobody" user is used as a fallback.

However, after switching euid to "nobody" the module is unable to switch back to root, and _exit is called after logging the error at level LOG_EMERG.

To reproduce on MacOS:

auth pam_opendirectory.so
auth pam_google_authenticator.so

Then connect to a running service and enter a bogus username/password.

_exit is called from within pam_google_authenticator.so, which abruptly terminates the process, after logging "We switched users from 0 to 4294967294, but can't switch back".

ThomasHabets commented 5 years ago

Could you confirm that the UID of nobody on your system is 4294967294?

Looks like there may be an integer overflow. Old UID is stored in an int, and if that's a signed 32bit int then yeah sounds like this would fail.

ThomasHabets commented 5 years ago

Could you try the code at HEAD? If the problem is what I think it is, then it should now be fixed.

NWilson commented 5 years ago

That commit now breaks normal operation of the PAM module on Linux :( I have done a before-and-after test, and a normal user registered on the system no longer works on Linux.

You are right that on my Mac machine, "nobody" is 4294967294.

ThomasHabets commented 5 years ago

Oops. Yeah I must have made a typo or something. That commit should be mostly right to fix this, but clearly not enough.

I expect to continue to be completely swamped this week, so for now I've rolled back that change. I don't know if I'll have time during the next two weeks to try again and, ehem, actually test the code before committing.

So either if someone could tweak my broken commit, or I'll look at this in 2-3 weeks. In the mean time HEAD should work for all but Mac.

NWilson commented 5 years ago

No problem at all, I'm not in any rush. Hope things clear up for you.

CamJN commented 5 years ago

To be clear, this fails safe right? It doesn't fail open?

ThomasHabets commented 5 years ago

As far as I can see, yes correct.