292544 - [SECURITY] Can see a security-sensitive bug in buglist.cgi for a short time when there are certain performance problems

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Matthias Versen [:Matti]]

I'm not in the security group for bugzilla.mozilla.org but I saw bug 292541 
in the query for todays new bugs for a few (2-3) minutes until it disappeared.

292541     cri    --    PC    [e-mail]    UNCO       Firefox/Xorg
crash when site loads

I could not access the bug itself because i got a security error :
"You are not authorized to access bug #292541."

<justdave> the security flag was set by the reporter according to the bug history.
<justdave> if you don't have security access and you saw it at all, please file
a security-flagged bug against Bugzilla on it.

Comment 1

[Dave Miller [:justdave]]

15:53 <@justdave> I suspect the flags are set after the bug is created (which is
necessary in order to know the bug number to set the flag on) and whatever
happened that caused replication to lag happened to hit right between the two
insert queries.
15:54 <@justdave> man, that's a good case for requiring transactions right there.

Comment 2

[Dave Miller [:justdave]]

Outside of getting transaction support working during post_bug, I don't think
there's a damn thing we can do about this aside from rel-noting it.  It's a
side-effect of using a shadow database without transactions.  This would only
affect sites using a shadow database, and only when replication is lagging and
happens to lag at *exactly* the right spot.  Freaking race condition.

Comment 3

[Frédéric Buclin]

If we put "INSERT INTO bugs..." and "INSERT INTO bug_group_map..." between "LOCK
TABLES bugs WRITE, bug_group_map WRITE" and "UNLOCK TABLES", does it prevent the
bug data from being copied to the shadow DB before all security bits are set?

Comment 4

[Matthias Versen [:Matti]]

BTW: This bug is  not that important.
You have to hit the race condition in bugzilla and also you have to get usefull
informations from the summary (and that isn't the case in most security bugs)

Comment 5

[Jacob Steenhagen]

(In reply to comment #4)
> BTW: This bug is  not that important.
> You have to hit the race condition in bugzilla and also you have to get usefull
> informations from the summary (and that isn't the case in most security bugs)

While hitting the race condition would certainly be difficult, I suspect you
could get all the information you want on the bug if you ran the query in "Long
Format" during the window. Though, truthfully, this really isn't all that
different than a user submitting a security bug w/out setting the security flag,
except in that case, the information would be public until there was some manual
intervention.

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> While hitting the race condition would certainly be difficult, I suspect you
> could get all the information you want on the bug if you ran the query in "Long
> Format" during the window.

I don't think so as the bug description (and any additional comments) is written
to the DB after the security settings are. So if the shadow DB has access to
comments, it also has access to the security settings set on this bug and should
not display any confidential information.

Comment 7

[Dave Miller [:justdave]]

This does not affect 2.16.x because the security flags are actually stored in
the bugs table (groupset column) with the bug in 2.16.

The only way I can think of to fix this without transactions is to add some sort
of flag to a bug that says "there aren't any other flags set" or something to
that effect.

Comment 8

[Frédéric Buclin]

Attachment: patch, v1

Lock tables while adding the new bug to the DB. Even if that does not fix the
problem, at least it sounds good to me to lock tables anyway! ;)

Comment 9

[Frédéric Buclin]

Per discussion with Therion and justdave on #mysql, we came to the following
conclusion:

- we change the schema of the 'bug_group_map' table to allow NULL group_id;
- *all* bugs must have an entry in this table. If the bug is public, set
group_id to NULL;
- if a bug has no entry in 'bug_group_map', the bug is not accessible,
independently of your privs.

This requires to change all join conditions to INNER JOIN. This way, when a new
bug is created, we cannot access it even if replication occurs between its entry
in the 'bugs' table and its entry in 'bug_group_map'. Moreover, this solution is
more reliable than to see if a comment has already been inserted into longdescs
and there is no need to add an additional field into the 'bugs' table. :)

Comment 10

[Frédéric Buclin]

Comment on attachment 182888 [details] [diff] [review]
patch, v1

Not useless, but not complete, per my previous comment. ;)

Comment 11

[Max Kanat-Alexander]

comment 9 sounds like a high-risk solution to a low-risk bug, to me.

Comment 12

[Frédéric Buclin]

Attachment: non-working not finished patch

I decided to abandon the idea mentioned in comment 9. Definitely too risky and
too ugly. Moreover, we are not consistent anymore between the way we treat
bug_group_map and other *_group_map tables, which is bad also.

We need another suggestion...

Comment 13

[Joel Peshkin]

I suggest you stay away from the solution in comment 9.  Right now, we can
extremely efficiently figure out if there is any reason barring you from seeing
a bug.  That would make that cease to be true.  

If you really want something along those lines, then, there needs to be
something like a group -1 (that everyone is effecivelty in) and we would then do
an additional join to ensure that the bug is in group -1 or else it is invisible.

In other words...

change
LEFT JOIN bug_group_map ON bugs.id = bug_group_map.bug_id AND
bug_group_map.group_id NOT IN (1,2,3,4,5,...)
WHERE bug_group_map.group_id IS NULL

to

LEFT JOIN bug_group_map ON bugs.id = bug_group_map.bug_id AND
bug_group_map.group_id NOT IN (1,2,3,4,5,...)
INNER JOIN bug_group_map AS bgm2 ON bugs.id = bgm2.bug_id AND
bgm2.group_id = -1
WHERE bug_group_map.group_id IS NULL

but I don't think this is worth it.

Comment 14

[Frédéric Buclin]

Another suggestion is to:
1) lock tables per my initial patch (patch, v1);
2) get the next bug ID using $next_bug_id = "SELECT MAX(id) FROM bugs" + 1;
3) insert group stuff into bug_group_map using bug_id = $next_bug_id;
4) insert bug stuff into bugs using bug_id = $next_bug_id;
5) insert remaining data in their respective tables.

Another suggestion is to:
1) insert an empty bug into the bugs table: INSERT INTO bugs () VALUES ();
2) get its ID using $dbh->last_insert_id();
3) - 5) same as above


Question: why tables are locked when updating bugs in process_bug.cgi, but not
when creating a new bug in post_bug.cgi? For installations which are not using
replication, I would say that my patch, v1 would prevent data leakage, even
without changing the actual sequence order. Please comment.

Comment 15

[Joel Peshkin]

Definitely, the empty bug approach.  
You may as well add the rest of the bug record last, then.  That will ensure
that the bug is actually complete before anyone fetches it.  Most of the rest of
the code joins reporter and assigned_to to profiles, so the empty bug should not
show up in odd places until it is complete.

Comment 16

[Frédéric Buclin]

Attachment: patch, v2

post_bug.cgi now delays the moment when it sets bugs.creation_ts. It waits till
all tables are updated. User::can_see_bug() and Search.pm are updated
accordingly, per discussion with joel on IRC.

Comment 17

[Joel Peshkin]

Comment on attachment 185479 [details] [diff] [review]
patch, v2

looks right.  I'll r+ it after I test it.

Comment 18

[Frédéric Buclin]

Attachment: patch, v2.1

oops, I forgot to remove a comment which I used for tests.

Comment 19

[Frédéric Buclin]

Attachment: patch for the trunk, v3

I missed the most trivial case: when the user is not logged in. In this case,
can_see_bug() returns 'true' because ($owner == $userid) is true: neither the
owner nor the user is defined. :(

Now the test done by can_see_bug() is of the form ($ready && (...)) which is
the best test we can expect. The bug is not ready? Return 'false'. Period!

Note that this patch does not apply cleanly to 2.18.1. Backport coming soon.

Comment 20

[Frédéric Buclin]

Attachment: patch for 2.18.1, v1

Same patch as for the trunk, backported to 2.18.1.

Note that Bugzilla::User::can_see_bug() does not exist yet. It was previously
known as CanSeeBug() in globals.pl, which had a $found_id variable defined but
not used.

Comment 21

[Joel Peshkin]

Comment on attachment 185585 [details] [diff] [review]
patch for the trunk, v3

Max,

   Is the creation_ts > 0 portable across databases?

Comment 22

[Joel Peshkin]

Comment on attachment 185594 [details] [diff] [review]
patch for 2.18.1, v1

Looks good.  I don't have a 2.18 to test it.  Someone needs to test this before
it lands.

Comment 23

[Max Kanat-Alexander]

Comment on attachment 185585 [details] [diff] [review]
patch for the trunk, v3

(In reply to comment #21)
> (From update of attachment 185585 [details] [diff] [review] [edit])
> Max,
> 
>    Is the creation_ts > 0 portable across databases?

No. In fact, neither is 00-00-00 00:00:00. You just need to pick some date in
the far past. I think we should have a constant in Bugzilla::Constants called
BEGINNING_OF_TIME to handle situations like this, since it keeps coming up.

Comment 24

[Frédéric Buclin]

Here is what I found at the official PostgreSQL website:

http://www.postgresql.org/docs/8.0/static/datatype-datetime.html

Name:        timestamp [(p)] [without time zone]
Description: both date and time
Low Value:   4713 BC
High Value:  5874897 AD

0 is between the lowest and highest date allowed, right? What's the problem???

Comment 25

[:glob ✱]

> What's the problem???

ms-sql's epoch is Jan 1, 1753

Comment 26

[Frédéric Buclin]

Reading the docs again, I see where the problem could be, even using MySQL. If
the NO_ZERO_DATE SQL mode is enabled, '0' would generate errors. OK, forget this
solution.

Then I see two other alternatives:

1) change the schema to accept creation_ts to be NULL;
2) set its default to EPOCH or BEGINNING_OF_TIME (a per DB constant).

Any preference?

Comment 27

[Max Kanat-Alexander]

(In reply to comment #26)
> 1) change the schema to accept creation_ts to be NULL;
> 2) set its default to EPOCH or BEGINNING_OF_TIME (a per DB constant).

  For now, just create the constant BEGINNING_OF_TIME and use it in your INSERT
statements. Both of those solutions are very complex. The second one is complex
due to the way that Bugzilla::DB::Schema is structured.

Comment 28

[Frédéric Buclin]

Attachment: patch for the trunk, v4

Instead of creating some artifical 'zero time', I prefer to set the creation
time of the bug as NULL till all other bug fields are defined. This is cleaner
and safer IMO.

Comment 29

[Frédéric Buclin]

Attachment: patch for 2.18.1, v2

Comment 30

[Joel Peshkin]

Comment on attachment 186143 [details] [diff] [review]
patch for the trunk, v4

r=joel
Create another bug to enhance sanitycheck.cgi to find these bugs if creation_ts
never gets set.

Comment 31

[Joel Peshkin]

Comment on attachment 186150 [details] [diff] [review]
patch for 2.18.1, v2

r=joel if you've tested it

Comment 32

[Frédéric Buclin]

(In reply to comment #31)
> r=joel if you've tested it

Yes I have.

Comment 33

[Bradley Baetz (:bbaetz)]

Transactions rule ;)

Comment 34

[Frédéric Buclin]

Attachment: patch for 2.18.1, v2.0.1 (unbitrot)

"patch for 2.18.1, v2" is bitrotten due to the checkin of bug 289382. Here is
an updated one.

carrying forward joel's r+

Comment 35

[Max Kanat-Alexander]

Tip:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.412; previous revision: 1.411
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.118; previous revision: 1.117
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.99; previous revision: 1.98
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.61; previous revision: 1.60
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.32; previous revision: 1.31
done

2.18:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.289.2.33; previous revision: 1.289.2.32
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.270.2.12; previous revision: 1.270.2.11
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.88.2.9; previous revision: 1.88.2.8
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.57.2.8; previous revision: 1.57.2.7
done

Comment 36

[Max Kanat-Alexander]

OK, security advisory posted and release complete.