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 email@example.com 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.
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.
Created attachment 182888 [details] [diff] [review] 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! ;)
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 on attachment 182888 [details] [diff] [review] patch, v1 Not useless, but not complete, per my previous comment. ;)
comment 9 sounds like a high-risk solution to a low-risk bug, to me.
Created attachment 184052 [details] [diff] [review] 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...
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.
Created attachment 185479 [details] [diff] [review] 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 on attachment 185479 [details] [diff] [review] patch, v2 looks right. I'll r+ it after I test it.
Created attachment 185499 [details] [diff] [review] patch, v2.1 oops, I forgot to remove a comment which I used for tests.
Created attachment 185585 [details] [diff] [review] 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.
Created attachment 185594 [details] [diff] [review] 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 on attachment 185585 [details] [diff] [review] patch for the trunk, v3 Max, Is the creation_ts > 0 portable across databases?
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 on attachment 185585 [details] [diff] [review] patch for the trunk, v3 (In reply to comment #21) > (From update of attachment 185585 [details] [diff] [review] ) > 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.
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.
Created attachment 186143 [details] [diff] [review] 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.
Created attachment 186150 [details] [diff] [review] patch for 2.18.1, v2
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 on attachment 186150 [details] [diff] [review] patch for 2.18.1, v2 r=joel if you've tested it
(In reply to comment #31) > r=joel if you've tested it Yes I have.
Transactions rule ;)
Created attachment 188607 [details] [diff] [review] 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+
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: 188.8.131.52; previous revision: 184.108.40.206 done Checking in Bugzilla/Search.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v <-- Search.pm new revision: 220.127.116.11; previous revision: 18.104.22.168 done
OK, security advisory posted and release complete.