449984 - Login cookies should be created as SSL-only on installations that require SSL

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P1)

Description

[Michael Morgan [:morgamic]]

We don't have the secure flag on our Bugzilla cookies, which means that if someone
did a man-in-the-middle attack they could steal Bugzilla sessions.

When people type in the domain it has to redirect from http->https.  That scenario is the one of concern.

We need to:
* patch cookie creation to create cookies with the secure bit
* nuke all existing bugzilla sessions

We could also consider:
* disabling http->https redirects

Comment 1

[timeless]

don't bother w/ the disabling. it'll just **** people off, and it won't protect anyone.

Comment 2

[Max Kanat-Alexander]

Yes, I've long been aware of this. It happens in nearly every web app out there, it's not just us.

Do all the browsers we support support the secure bit?

Comment 3

[Michael Morgan [:morgamic]]

We'd probably have to enumerate which browsers support it, but in the meantime we should enable this for BMO, at least.  If someone hijacked a trusted user's session they could disclose security bugs -- fixed or not.

It doesn't have to be enabled by default in the distribution version of Bugzilla.

Comment 4

[Max Kanat-Alexander]

As long as it doesn't actually *break* cookies in the browsers that don't support it, I don't see anything wrong with implementing it upstream. Our cookie code is pretty modularized, and not to hard to fix up.

Comment 5

[Max Kanat-Alexander]

Also, you don't have to MITM, you just have to be on an unswitched network with the cookie sender (like an open wireless).

Comment 6

[Michael Morgan [:morgamic]]

There was actually a bug filed already a couple of hours ago.

Comment 7

[Dave Miller [:justdave]]

Not the same bug.  Wrong product. :)  (Well, yes, it's the same bug, but comment 0 in that bug asks that this one remain separate so it can be potentially fixed in bmo without waiting for it to land upstream).

Comment 8

[Max Kanat-Alexander]

I won't block 3.2 on this, but I'd take it in the 3.2 final or 3.2.1 timeframe, definitely. It won't make 3.2rc1. or 3.0.5.

Comment 9

[Bradley Baetz (:bbaetz)]

This should actually be fairly simple. Browser compat isn't an issue - if we set it as secure and the client doesn't support that attribute it'll still work, just get sent for all sessions (barring browser bugs I guess, but this feature is used in enough sites that that shouldn't happen...) It should be on where appropriate and keyed off the ssl param not something else.

Just need to be a bit careful about the transition code - ie not getting left with stale non-secure cookies that aren't replaced with the new ones.

Comment 10

[Dave Miller [:justdave]]

Release notes should advise people to truncate the logincookies table after picking this up.  I can't think of a way to automate this in checksetup without doing it every time you run it, which wouldn't be very nice.

Comment 11

[Bradley Baetz (:bbaetz)]

Attachment: Patch

Something like this. Adds a few sanity checks such as clearing any insecure cookies that are sent to us. Only changes login cookies to be secure; for the 'always' case I guess all the cookies could be made secure.

The RFC says that overwriting cookies happens based on name/path/etc only so the secure cookie should just overwrite the non-secure one. Tested on FF only.

I made the ssl param depend on sslbase being set. I know why the other https checks in the code check sslbase (to avoid the admin locking themselves out) but I don't know why we don't just enforce that in the param setting code - anyone?

Its had light testing on firefox 3 on linux, but nothing else.

Comment 12

[Bradley Baetz (:bbaetz)]

This doesn't clear existing sessions on upgrade. The code I added to clear doesn't trigger for 'always' settings because we redirect before checking in such cases - it only triggers when using ssl for authenticated sessions only.

Comment 13

[Frédéric Buclin]

Before you all become paranoid, but I guess the damage is already done, I would just note here that this bug has been reported for over a year as bug 381569, as an enhancement with priority P4, and nobody really paid any attention to it. That bug has no security flag set, so if someone wanted to try this attack, it probably didn't need to wait till today to try it. I just restricted it to the security group right now, to hide it while we have this discussion and fix this issue.

Also, before this patch is reviewed, I first want to see bug 428659 being committed, to be sure we don't conflict or introduce anything wrong here. Also, make sure we don't regress anything which has been discussed in bug 428659 comment 44.

Comment 14

[Reed Loden [:reed]]

Note, I bitrotted this patch in bug 368502.

Comment 15

[Max Kanat-Alexander]

Comment on attachment 333140 [details] [diff] [review]
Patch

No, no, too complex.

Just set the secure bit on the cookies if necessary, and in Install::DB we can do a TRUNCATE TABLE logincookies based on the very last schema change, or we can just add a note for upgraders in the release notes, like we did in 2.22 or so.

Comment 16

[Max Kanat-Alexander]

Comment on attachment 333140 [details] [diff] [review]
Patch

  Actually, I take that back. I see why you did what you did. I do have some comments though:

>Index: Bugzilla/Auth/Login/Cookie.pm
>+        # If we got a cookie and we're not over SSL then the cookie
>+        # was sent plain-text. Invalidate it.

  You don't need the comments to explain what we're doing, only why. But it would probably be good to say something like, "If we get a plain-text cookie and we require SSL for authenticated sessions, then delete and refuse the cookie."

  However, that will totally screw up browsers that don't support the secure attribute, when they are doing redirects. So I think we should probably avoid it (unless we do the redirect before this point?).

>+        if (Bugzilla->params->{'ssl'} ne 'never'
>+            && $cgi->protocol ne 'https') {

  Nit: Brace on next line.

>+            $dbh->do("DELETE FROM logincookies WHERE userid = ? AND cookie = ?",
>+                     undef,
>+                     $user_id, $login_cookie);

  Nit: No need to have the vars on a separate line from undef.

>Index: Bugzilla/Auth/Persist/Cookie.pm

  This of course needs a bitrot fix.

>+    my @cookieargs = ();

  You could make that a hash, too--probably better.

>+        # Sanity check
>+        if ($cgi->protocol ne 'https') {
>+            ThrowCodeError("cookie_insecure");
>+        }

  Mm, I'm not sure we need that sanity check.

>Index: Bugzilla/Config/Common.pm
>+    if ($val ne 'never' && !Bugzilla->params->{'sslbase'}) {
>+        return "Using SSL requires setting the sslbase parameter";
>+    }

  You can't do that, Bugzilla->params hasn't been updated yet (if we set sslbase and ssl at the same time).

>+    if ($val ne 'never' && Bugzilla->params->{'ssl'} eq 'never') {

  Hmm, our checkers don't get both the old and new values?

>+        Bugzilla->dbh->do("DELETE FROM logincookies");

  You'll need to display a message to the admin explaining that all users have been logged out. You should maintain the admin's cookie, though, or he won't even be able to see such a message, right? Particularly if another validator fails after this one.

Comment 17

[Frédéric Buclin]

Attachment: minimal fix

Based on how we now redirect to SSL in Bugzilla->login, I think the fix is as trivial as this. We don't care if $cgi->protocol eq 'https' or not; we are sure we will be redirected if necessary. And if the cookie existed before SSL has been turned on, then the admin is free to "TRUNCATE TABLE logincookies".

Note that I didn't include the message displayed to the admin suggesting him to truncate the table.

Comment 18

[Bradley Baetz (:bbaetz)]

(In reply to comment #16)
> (From update of attachment 333140 [details] [diff] [review])
>   Actually, I take that back. I see why you did what you did. I do have some
> comments though:
> 
> >Index: Bugzilla/Auth/Login/Cookie.pm
> >+        # If we got a cookie and we're not over SSL then the cookie
> >+        # was sent plain-text. Invalidate it.

>   However, that will totally screw up browsers that don't support the secure
> attribute, when they are doing redirects. So I think we should probably avoid
> it (unless we do the redirect before this point?).

I don't believe that there are any browsers not supporting the secure attribute, although I suppose there could be. We do redirect before here, though.

> >+        # Sanity check
> >+        if ($cgi->protocol ne 'https') {
> >+            ThrowCodeError("cookie_insecure");
> >+        }
> 
>   Mm, I'm not sure we need that sanity check.

I'm paranoid? ;)

> 
> >Index: Bugzilla/Config/Common.pm
> >+    if ($val ne 'never' && !Bugzilla->params->{'sslbase'}) {
> >+        return "Using SSL requires setting the sslbase parameter";
> >+    }
> 
>   You can't do that, Bugzilla->params hasn't been updated yet (if we set
> sslbase and ssl at the same time).

Oh, bleh. Is there a way to do that?

> 
> >+    if ($val ne 'never' && Bugzilla->params->{'ssl'} eq 'never') {
> 
>   Hmm, our checkers don't get both the old and new values?
> 
> >+        Bugzilla->dbh->do("DELETE FROM logincookies");
> 
>   You'll need to display a message to the admin explaining that all users have
> been logged out. You should maintain the admin's cookie, though, or he won't
> even be able to see such a message, right? Particularly if another validator
> fails after this one.

No, at this point we're processing the page so the login checks have been done. This page will work but any subsequent access will fail. Or do we not apply any params changes if one fails?

Comment 19

[Max Kanat-Alexander]

Comment on attachment 335140 [details] [diff] [review]
minimal fix

Looks good to me, as long as the "authenticated sessions" SSL mode keeps working.

Comment 20

[Max Kanat-Alexander]

I'm fine with unlocking this bug, if you are, LpSolit.

Because we don't properly do SSL redirects on 3.0 (particularly not for XML-RPC), I don't think this can go any earlier than 3.2.

Comment 21

[Max Kanat-Alexander]

We should add a Note For Upgraders in the release notes that mentions you should ideally do TRUNCATE TABLE logincookies if you use SSL.

Comment 22

[Reed Loden [:reed]]

Comment on attachment 335140 [details] [diff] [review]
minimal fix

>+    # Prevent JavaScript to use login cookies.

s/to use/from accessing/

Comment 23

[Frédéric Buclin]

(In reply to comment #20)
> I'm fine with unlocking this bug, if you are, LpSolit.

Yes, I'm fine with that. Especially because bug 381569 (which is now a dupe of this bug) was publicly visible for over a year.

Comment 25

[Frédéric Buclin]

tip:

Checking in Bugzilla/Auth/Persist/Cookie.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Persist/Cookie.pm,v  <--  Cookie.pm
new revision: 1.7; previous revision: 1.6
done

3.2rc1:

Checking in Bugzilla/Auth/Persist/Cookie.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Persist/Cookie.pm,v  <--  Cookie.pm
new revision: 1.5.4.2; previous revision: 1.5.4.1
done

Comment 26

[Max Kanat-Alexander]

Added to release notes for 3.2 in bug 463244.