Closed Bug 292544 Opened 19 years ago Closed 19 years ago

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

Categories

(Bugzilla :: Bugzilla-General, defect)

2.17
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: Matti, Assigned: LpSolit)

References

Details

(Whiteboard: [does not affect 2.16.x] [ready for 2.18.2] [ready for 2.20])

Attachments

(2 files, 7 obsolete files)

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    nobody@mozilla.org    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.
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.
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.
OS: Windows Server 2003 → All
Hardware: PC → All
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?
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)
(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.
(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.
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.
Flags: blocking2.20+
Flags: blocking2.18.2+
Whiteboard: [does not affect 2.16.x] [wanted for 2.18.2] [wanted for 2.20]
Target Milestone: --- → Bugzilla 2.18
Attached patch patch, v1 (obsolete) — Splinter Review
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! ;)
Attachment #182888 - Flags: review?(mkanat)
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. :)
Assignee: general → LpSolit
Comment on attachment 182888 [details] [diff] [review]
patch, v1

Not useless, but not complete, per my previous comment. ;)
Attachment #182888 - Flags: review?(mkanat)
comment 9 sounds like a high-risk solution to a low-risk bug, to me.
Attached patch non-working not finished patch (obsolete) — Splinter Review
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...
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.
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.
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.
Attached patch patch, v2 (obsolete) — Splinter Review
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.
Attachment #182888 - Attachment is obsolete: true
Attachment #184052 - Attachment is obsolete: true
Attachment #185479 - Flags: review?(bugreport)
Comment on attachment 185479 [details] [diff] [review]
patch, v2

looks right.  I'll r+ it after I test it.
Attached patch patch, v2.1 (obsolete) — Splinter Review
oops, I forgot to remove a comment which I used for tests.
Attachment #185479 - Attachment is obsolete: true
Attachment #185499 - Flags: review?(bugreport)
Attachment #185479 - Flags: review?(bugreport)
Attachment #185499 - Flags: review?(bugreport) → review+
Flags: approval?
Flags: approval2.18?
Attached patch patch for the trunk, v3 (obsolete) — Splinter Review
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.
Attachment #185499 - Attachment is obsolete: true
Attachment #185585 - Flags: review?(bugreport)
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval2.18?
Attached patch patch for 2.18.1, v1 (obsolete) — Splinter Review
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.
Attachment #185594 - Flags: review?(bugreport)
Comment on attachment 185585 [details] [diff] [review]
patch for the trunk, v3

Max,

   Is the creation_ts > 0 portable across databases?
Attachment #185585 - Flags: review?(mkanat)
Attachment #185585 - Flags: review?(bugreport)
Attachment #185585 - Flags: review+
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.
Attachment #185594 - Flags: review?(bugreport) → review+
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.
Attachment #185585 - Flags: review?(mkanat) → review-
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???
> What's the problem???

ms-sql's epoch is Jan 1, 1753
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?
(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.
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.
Attachment #185585 - Attachment is obsolete: true
Attachment #186143 - Flags: review?(bugreport)
Attached patch patch for 2.18.1, v2 (obsolete) — Splinter Review
Attachment #185594 - Attachment is obsolete: true
Attachment #186150 - Flags: review?(bugreport)
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.
Attachment #186143 - Flags: review?(bugreport) → review+
Comment on attachment 186150 [details] [diff] [review]
patch for 2.18.1, v2

r=joel if you've tested it
Attachment #186150 - Flags: review?(bugreport) → review+
(In reply to comment #31)
> r=joel if you've tested it

Yes I have.
Flags: approval?
Flags: approval2.18?
Whiteboard: [does not affect 2.16.x] [wanted for 2.18.2] [wanted for 2.20] → [does not affect 2.16.x] [ready for 2.18.2] [ready for 2.20]
Blocks: 297749
Blocks: 299156
Transactions rule ;)
"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+
Attachment #186150 - Attachment is obsolete: true
Attachment #188607 - Flags: review+
Blocks: 299653
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
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
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Summary: I could see a new security-sensitive bug in the query for a short time → [SECURITY] Can see a security-sensitive bug in buglist.cgi for a short time when there are certain performance problems
Version: 2.19.1 → 2.17
OK, security advisory posted and release complete.
Group: webtools-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: