PAM module Make two step verification optional #32

Closed
ThomasHabets opened this Issue Oct 10, 2014 · 22 comments

Comments

Projects
None yet
2 participants
Contributor

ThomasHabets commented Oct 10, 2014

Original issue 32 created by brian@microcomaustralia.com.au on 2011-02-15T23:42:53.000Z:

What steps will reproduce the problem?

  1. Build, install, and configure pam module.
  2. Configure pam module for user X.
  3. Other user Y cannot login any more.

What is the expected output? What do you see instead?

User Y should be able to login without using two verification when the $HOME/.google_authenticator doesn't exist; this allows testing of two step verification without having to enable it for all users.

What version of the product are you using? On what operating system?

Debian. 9:77c50ff7b7fa.

Please provide any additional information below.

None.

Contributor

ThomasHabets commented Oct 10, 2014

Fixed by 8f2cb7b

Contributor

ThomasHabets commented Oct 10, 2014

Comment #1 originally posted by brian@microcomaustralia.com.au on 2011-02-16T00:02:22.000Z:

Sorry, somehow got the version information completely wrong. This is the latest hg version, 44:bd9e0af3a6d5. Running on Ubuntu maverick.

Contributor

ThomasHabets commented Oct 10, 2014

Comment #2 originally posted by brian@microcomaustralia.com.au on 2011-02-16T02:06:19.000Z:

Just noticed this is a duplicate of bug # 27.

Contributor

ThomasHabets commented Oct 10, 2014

Comment #3 originally posted by jeremy.kitchen on 2011-02-16T19:50:32.000Z:

Feb 16 11:47:37 inara sshd(pam_google_authenticator)[32378]: Failed to read "/home/gpgverify/.google_authenticator"
Feb 16 11:47:39 inara sshd[32374]: error: PAM: Cannot make/remove an entry for the specified session for gpgverify from ip-66-33-206-8.dreamhost.com

it should simply issue a pass if they don't have google_authenticator configured.

Note that this introduces an issue if you are also wishing to protect sudo with google authenticator, as the user can simply remove their .google_authenticator and it will let them in.

Contributor

ThomasHabets commented Oct 10, 2014

Comment #4 originally posted by brian@microcomaustralia.com.au on 2011-02-17T00:50:35.000Z:

Hello Jeremy,

Presumably an intruder that wishes to gain sudo rights without the validator can simply run google-authenticator and rewrite .google_authenticator anyway. So this change doesn't change that.

Not sure what the solution is to the sudo problem. You could move the file to somewhere only root has write access (this would also solve bugs like # 24), however the user still needs to be able to somehow update it too. Maybe only allow updates if the user authenticates first, like with passwd?

Brian May

Contributor

ThomasHabets commented Oct 10, 2014

Comment #5 originally posted by paul.devrieze on 2011-02-28T16:23:01.000Z:

You can do a lot with pam files and how you handle failure. The main problem is that the pam module does not return any other status code other than PAM_SUCCESS or PAM_SESSION_ERR. The attached patch changes the system to issue PAM_PERM_DENIED when the token is not matched, and PAM_CRED_UNAVAIL when the auth file does not exist and PAM_AUTHTOK_ERR when retrieving the secret fails because of a file-related error.

Contributor

ThomasHabets commented Oct 10, 2014

Comment #6 originally posted by brian@microcomaustralia.com.au on 2011-02-28T21:24:29.000Z:

Interesting approach taken in comment # 5. A lot simpler then the approach taken in bug # 27, although that patch goes further with the suffix= option.

Trouble is that normally it should be the project members (Google) that pick the best approach and merge it into the official source, unfortunately they don't seem to be interested any more. I haven't noticed any responses to any of these issues from Google. I don't know if it is possible to get write access to the code here, if not maybe we should be making an official fork?

Contributor

ThomasHabets commented Oct 10, 2014

Comment #7 originally posted by kroot@google.com on 2011-02-28T21:31:42.000Z:

We're still interested and will look at the possible approaches. I promise :-)

Contributor

ThomasHabets commented Oct 10, 2014

Comment #8 originally posted by markus@google.com on 2011-03-09T20:25:43.000Z:

Issue 21 has been merged into this issue.

Contributor

ThomasHabets commented Oct 10, 2014

Comment #9 originally posted by markus@google.com on 2011-03-09T20:26:02.000Z:

Issue 18 has been merged into this issue.

Contributor

ThomasHabets commented Oct 10, 2014

Comment #10 originally posted by markus@google.com on 2011-03-09T20:26:23.000Z:

Issue 27 has been merged into this issue.

Contributor

ThomasHabets commented Oct 10, 2014

Comment #11 originally posted by markus@google.com on 2011-03-09T20:26:50.000Z:

<empty>

Contributor

ThomasHabets commented Oct 10, 2014

Comment #12 originally posted by djk7wb@mst.edu on 2011-03-24T05:08:32.000Z:

As an alternative workaround, PAM is able to limit this to certain users, using something like this:

auth [default=ignore success=1] pam_succeed_if.so quiet user notingroup secure
auth required pam_google_authenticator.so

I agree, though... the idea of needing a user to set up the authenticator before trying to use it is vital, or else this becomes something of a chicken/egg problem.

Contributor

ThomasHabets commented Oct 10, 2014

Comment #13 originally posted by charles.lacroix on 2011-05-13T15:45:43.000Z:

I like this idea:

auth [default=ignore success=1] pam_succeed_if.so quiet user notingroup secure
auth required pam_google_authenticator.so

Solves chicken and egg problem and it also gives an opportunity to require the authenticator code for only a certain amount of users

In my case i want ssh accounts ( at least the ones that can su to root ) to require authenticator, but still allow sftp without the authenticator.

Contributor

ThomasHabets commented Oct 10, 2014

Comment #14 originally posted by michael.cordover on 2011-06-09T17:18:17.000Z:

Hi All,

Pardon the extraordinarily ugly code but the basic idea is to return a more appropriate error from google_authenticator() than PAM_SESSION_ERR on every failure. Using this hell-ugly patch (it's 3am here) and:

auth [success=ok authinfo_unavail=ignore default=bad] pam_google_authenticator.so

the PAM module will require the verification code if and only if the .google_authenticator file exists (or alternate if location specified). I think this is preferable to the other methods specified as it uses the existing PAM famework; it has error messages for these sorts of situations. I'm not sure I've used the right ones in every situation (not a PAM expert!) but as I say, this particular ugly hack seems to be working for me.

HTH someone.

mjec

Contributor

ThomasHabets commented Oct 10, 2014

Comment #15 originally posted by jaearick on 2011-09-12T19:37:26.000Z:

Regarding comment 5 and the associated patchfile: when the patch is applied "make test" fails as a result:

Testing failed login attempt
pam_google_authenticator_unittest: pam_google_authenticator_unittest.c:196: main: Assertion `pam_sm_open_session(((void *)0), 0, targc, targv) == 14' failed.
Did not receive verification code from user

because the test does not match what the new return codes are.

Contributor

ThomasHabets commented Oct 10, 2014

Comment #16 originally posted by jaearick on 2011-09-13T19:51:33.000Z:

I propose a PAM configuration option, eg:

auth required pam_google_authenticator.so nosecretfile-is-ok

see the attached patch file to implement this idea.

If this option is set, this says "it is ok to let a user login if they do not have a secret file" -- ie have pam_google_authenticator go ahead and return PAM_SUCCESS if the file does not exist. If the file does exist (or if the option is not set) then the default is that all users have to enter an authentication code.

This solves the sudo problem, because the .google_authenticator file could be a requirement in the /etc/pam.d/sudo PAM file (ie, the option is not used), but not required for /etc/pam.d/sshd. So a user could login initially to get their .google_authenticator file set up, yet they could not sudo until they do so.

This also avoids the gnarly PAM configuration issues in comments 12 and 13, which I could never get to work on SUSE 11.

Ranting now... I consider the big IF block in google_authenticator() right after parse_args() to be extremely ugly code. This whole chunk of code should be redone. It should examine the return values of each routine called here individually, and kick back appropriate PAM return codes after each step. If the user does not exist after get_user_name(), then return PAM_USER_UNKNOWN, and so on.

Contributor

ThomasHabets commented Oct 10, 2014

Comment #17 originally posted by jaearick on 2011-09-14T19:36:55.000Z:

Followup on comment 15: I discovered that my code can be interrupted and bypassed with a control-D from the ssh session. In my case the pam stack is verification code, password, kerberos password if necessary. Two control-D's jump thru the first two items, enter kerberos password, logged in. Sigh...

Contributor

ThomasHabets commented Oct 10, 2014

Comment #18 originally posted by cantlep@cantle.me on 2011-09-16T15:14:54.000Z:

I confirm this patch works on Fedora 13. Excellent, thanks :-) Although it works fine, an error is still logged (in my case in /var/log/secure), is there anyway that can be avoided?

i.e. my test user mnash is not using the 2FA.

Sep 16 16:22:18 cyclops sshd(pam_google_authenticator)[6940]: Failed to read "/home/mnash/.google_authenticator"
Sep 16 16:22:20 cyclops sshd[6938]: Accepted keyboard-interactive/pam for mnash from 127.0.0.1 port 57907 ssh2
Sep 16 16:22:21 cyclops sshd[6938]: pam_unix(sshd:session): session opened for user mnash by (uid=0)

Contributor

ThomasHabets commented Oct 10, 2014

Comment #19 originally posted by TheSp1d3r on 2011-11-14T01:18:02.000Z:

Ref: Comment 16
A blank verification code will bypass this patch

Contributor

ThomasHabets commented Oct 10, 2014

Comment #20 originally posted by markus@google.com on 2011-12-16T00:59:30.000Z:

I believe the main complaint from this issue is now addressed in the head of tree. Administrators can now set the "nullok" option to allow having a non-existent secret file.

Contributor

ThomasHabets commented Oct 10, 2014

Comment #21 originally posted by lwj0012 on 2013-01-04T05:00:39.000Z:

patch made by Comment 16 can solve this problem, but the patch file need to be update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment