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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bart Jansens’s picture

Version: 5.9 » 7.x-dev
FileSize
785 bytes

This 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:

if ($user->uid || $value || count($_COOKIE)) {

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.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

The 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.

egfrith’s picture

There's now a patch for a test for this bug at: #302075: drupalPost() needs option to POST with empty cookie jar

egfrith’s picture

I noticed the patch no longer applied, so I've rerolled against the latest HEAD.

lilou’s picture

Patch reroll from Drupal root.

webchick’s picture

Version: 7.x-dev » 6.x-dev

Committed to HEAD. Thanks!

Marking back for consideration in 6.x.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Patch fails to apply to Drupal 6. Which is not surprising given the above discussion. We need a backport.

egfrith’s picture

Status: Patch (to be ported) » Reviewed & tested by the community
FileSize
794 bytes

Here 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.

egfrith’s picture

Just realised I rolled the patch in a subdirectory... here is the correct patch.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Great, I've committed this to Drupal 6, thanks!

egfrith’s picture

Version: 6.x-dev » 5.x-dev
FileSize
741 bytes

Smashing! 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.

Gábor Hojtsy’s picture

Status: Fixed » Reviewed & tested by the community

Mark it RTBC for that as well then.

ivanSB@drupal.org’s picture

I tested it with:

function user_authenticate_xmlrpc($name, $pass) {
        $user=user_authenticate($name, $pass);
        return $user->uid;
}
function catalog_test_xmlrpc($string) {
        if(user_access('buy items')) {
                return "You can";
        } else {
                return "You can't\n";
        }
}

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.

egfrith’s picture

@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.

ivanSB@drupal.org’s picture

If 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.

egfrith’s picture

@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.

drumm’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 5.x.

egfrith’s picture

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.