At present, user_authenticate() fails when $_COOKIE is empty, because sess_write() does not write to the session table when $_COOKIE is empty. Most of the time this isn't a problem because drupal has given the browser a cookie by the time the login form is submitted.
However, this is a problem for modules such as Image Publishing, which allows clients other than web browsers to authenticate with drupal (see issue #204209: User authentification does not work under Gallery Remote). In this case user_authenticate() is called on the first request, before $_COOKIE is set. Therefore, although the $user variable is returned correctly from user_authenticate(), it is not saved to the session table, because $_COOKIE is not set.
The attached patch addresses this issue by getting sess_write() to write to the session table if $user->uid is nonzero, even if $_COOKIE is empty. I have confirmed that it does cause writing to the session table in the scenario above.
It shouldn't let any more crawlers through, unless they happen to have valid login information -- which if they are valid crawlers, they won't have.
Comment | File | Size | Author |
---|---|---|---|
#11 | 20081016_session_login_without_cookie_5.patch | 741 bytes | egfrith |
#9 | 20081016_session_login_without_cookie_6(2).patch | 821 bytes | egfrith |
#8 | 20081016_session_login_without_cookie_6.patch | 794 bytes | egfrith |
#4 | 20080920.session_login_without_cookie.patch | 759 bytes | egfrith |
#1 | 20080812.session_login_without_cookie.patch | 785 bytes | Bart Jansens |
Comments
Comment #1
Bart Jansens CreditAttribution: Bart Jansens commentedThis is pretty easy to reproduce. Go to yourdrupalsite.tld/user, clear your cookies and click login. The login will fail.
This patch seems correct to me. In the D6 code, you can still see the old check that we were using:
Looks like the uid check was forgotten in the new test, which was added by #76931: Don't SELECT user uid = 0
Moving to D7 because the issue still exists there. We can backport it if required.
D7 patch attached.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe patch looks good and solve the issue.
The second check that you mention in D6 is actually useless (logically speaking, it always matches), and was removed from D7 while discussing #213699: Race condition in sess_write() causes duplicate entry error in {sessions} table.
That last issue has been committed to D7 but not yet to D6, so we should probably wait for it before fixing D6.
Comment #3
egfrith CreditAttribution: egfrith commentedThere's now a patch for a test for this bug at: #302075: drupalPost() needs option to POST with empty cookie jar
Comment #4
egfrith CreditAttribution: egfrith commentedI noticed the patch no longer applied, so I've rerolled against the latest HEAD.
Comment #5
lilou CreditAttribution: lilou commentedPatch reroll from Drupal root.
Comment #6
webchickCommitted to HEAD. Thanks!
Marking back for consideration in 6.x.
Comment #7
Gábor HojtsyPatch fails to apply to Drupal 6. Which is not surprising given the above discussion. We need a backport.
Comment #8
egfrith CreditAttribution: egfrith commentedHere is a patch that applies to D6. It passes the manual test at comment #1, so, given that it has been accepted into D7, I'm marking the issue as RTBC - hope this is the Right Thing to do.
Comment #9
egfrith CreditAttribution: egfrith commentedJust realised I rolled the patch in a subdirectory... here is the correct patch.
Comment #10
Gábor HojtsyGreat, I've committed this to Drupal 6, thanks!
Comment #11
egfrith CreditAttribution: egfrith commentedSmashing! I'm marking back for consideration in 5.x, and am attaching a patch rolled against 5.11. Hope this is the right thing to do.
Comment #12
Gábor HojtsyMark it RTBC for that as well then.
Comment #13
ivanSB@drupal.orgI tested it with:
and it works.
Anyway user_authenticate does too much and too few.
It changes the global $user but it doesn't prevent session fixation nor it logs into {users} and {watchdog}.
It would be better to add a note to D5 and D6 about the security implications of using user_authenticate and separate un/pw checking from the rest in D7 especially since pw checking in D7 became more complicate.
Comment #14
egfrith CreditAttribution: egfrith commented@ivanSB: Thanks for testing at this. Re the point you make about what user_authenticate() should do, perhaps search the issue queue to see if this is being worked on and if it's not being worked on, raise a separate issue. It's possible that #280934: Use httponly cookie support when available is related.
Comment #15
ivanSB@drupal.orgIf I knew how to do I'd merge the 2 bugs since I think that separating credential check and token management will make the code cleaner, improve good use, let people decide if they want or don't want a cookie (checking user auth for administrative purposes, rpc etc...) and make it easier to modularize pass check.
Comment #16
egfrith CreditAttribution: egfrith commented@ivanSB :Your proposal sounds interesting and useful, but I think it would be much more productive to post a separate issue in the D7 queue rather than trying to add to discussion here.
The patch that fixes the bug that is the subject of this thread has been applied in D7 and D6, and may be accepted into D5, which is why it is now in the D5 queue, awaiting the consideration of the D5 maintainer. The patch makes a very minor change to the way drupal works, which is why might have a chance of being accepted into D5. (@drumm: From my point of view this would be great, as it would mean that users of the image_pub module I maintain would not have to patch drupal in order for it to work correctly).
Your proposal would require a more fundamental change to the code, I think. It is really a different bug. It will get no attention in the D5 queue, because D5 is not the place for active development.
Comment #17
drummCommitted to 5.x.
Comment #18
egfrith CreditAttribution: egfrith commentedThanks!