Closed
Bug 998396
Opened 10 years ago
Closed 10 years ago
Fix gethash completions in Aurora and Nightly
Categories
(Toolkit :: Safe Browsing, defect)
Tracking
()
RESOLVED
FIXED
Firefox 31
Tracking | Status | |
---|---|---|
firefox29 | --- | unaffected |
firefox30 | + | fixed |
firefox31 | + | fixed |
People
(Reporter: mmc, Assigned: mmc)
References
(Blocks 1 open bug)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 3 obsolete files)
All safebrowsing table preferences may be comma-separated strings: See https://bugzilla.mozilla.org/show_bug.cgi?id=998370 http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#72 1090 mGethashTables.AppendElement(Preferences::GetCString(PHISH_TABLE_PREF)); 1091 mGethashTables.AppendElement(Preferences::GetCString(MALWARE_TABLE_PREF)); 1092 mGethashTables.AppendElement(Preferences::GetCString( 1093 DOWNLOAD_BLOCK_TABLE_PREF)); 1094 mGethashTables.AppendElement(Preferences::GetCString( 1095 DOWNLOAD_ALLOW_TABLE_PREF));
Assignee | ||
Updated•10 years ago
|
status-firefox30:
--- → affected
tracking-firefox30:
--- → ?
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8409273 [details] [diff] [review] Fix gethash completions, urlclassifier.malware_table and urlclassifier.phish_table may be comma-separated lists ( Review of attachment 8409273 [details] [diff] [review]: ----------------------------------------------------------------- Tested by waiting for updates and visiting some phishtank URLs. These are showing up for me right now. http://quartexindustries.com/paypal-server-secure-server-update/ http://colunavip.net/patchID09c5dez3355dwsa25d43ax25937sae1a1x38e/confirmationform/eb0bdd10b554b392f71182cc8dcb78c0/ http://takinbless.nanotec-industries.com/doc/docs/ mmc@arkham:~/mozilla-central$ ls -al ~/.cache/mozilla/firefox/lsoq0afb.fix-gethash/safebrowsing/ total 1608 drwxr-xr-x 2 mmc mmc 4096 Apr 18 20:38 . drwx------ 6 mmc mmc 4096 Apr 18 20:38 .. -rw-rw-r-- 1 mmc mmc 12 Apr 18 20:38 goog-badbinurl-shavar.cache -rw-r--r-- 1 mmc mmc 243864 Apr 18 20:38 goog-badbinurl-shavar.pset -rw-rw-r-- 1 mmc mmc 259465 Apr 18 20:38 goog-badbinurl-shavar.sbstore -rw-rw-r-- 1 mmc mmc 12 Apr 18 20:38 goog-malware-shavar.cache -rw-r--r-- 1 mmc mmc 283394 Apr 18 20:38 goog-malware-shavar.pset -rw-rw-r-- 1 mmc mmc 384530 Apr 18 20:38 goog-malware-shavar.sbstore -rw-rw-r-- 1 mmc mmc 140 Apr 18 20:38 goog-phish-shavar.cache -rw-r--r-- 1 mmc mmc 280904 Apr 18 20:38 goog-phish-shavar.pset -rw-rw-r-- 1 mmc mmc 135681 Apr 18 20:38 goog-phish-shavar.sbstore -rw-rw-r-- 1 mmc mmc 44 Apr 18 20:38 test-malware-simple.cache -rw-r--r-- 1 mmc mmc 16 Apr 18 20:38 test-malware-simple.pset -rw-rw-r-- 1 mmc mmc 232 Apr 18 20:38 test-malware-simple.sbstore -rw-rw-r-- 1 mmc mmc 44 Apr 18 20:38 test-phish-simple.cache -rw-r--r-- 1 mmc mmc 16 Apr 18 20:38 test-phish-simple.pset -rw-rw-r-- 1 mmc mmc 232 Apr 18 20:38 test-phish-simple.sbstore
Attachment #8409273 -
Flags: review?(gpascutto)
Assignee | ||
Comment 3•10 years ago
|
||
I also see gethash requests in the logs. GET http://paypal.112.2o7.net/b/ss/paypalglobal/1/H.19.3/s35628504627318 [HTTP/1.1 200 OK 208ms] GET http://quartexindustries.com/paypal-server-secure-server-update/ [388ms] POST https://safebrowsing.google.com/safebrowsing/gethash [HTTP/1.1 200 OK 65ms]
Comment 4•10 years ago
|
||
Comment on attachment 8409273 [details] [diff] [review] Fix gethash completions, urlclassifier.malware_table and urlclassifier.phish_table may be comma-separated lists ( Review of attachment 8409273 [details] [diff] [review]: ----------------------------------------------------------------- I wonder if there's any other places we missed :-P ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp @@ +1071,5 @@ > sUrlClassifierDBService = nullptr; > } > > nsresult > +nsUrlClassifierDBService::ReadTables() Rename this to something more informative. ReadTablesFromPrefs (at least...)
Attachment #8409273 -
Flags: review?(gpascutto) → review+
Comment 5•10 years ago
|
||
Also make sure we don't complete on the download/test lists. That code might not really have been tested before due to this bug.
Assignee | ||
Comment 6•10 years ago
|
||
Thanks, will rename before checking in. I did an MXR search and these are the only places where these prefs are used.
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Gian-Carlo Pascutto [:gcp] from comment #5) > Also make sure we don't complete on the download/test lists. That code might > not really have been tested before due to this bug. I added this to GetCompleter: // We should never fetch hash completions for test tables. MOZ_ASSERT(!StringBeginsWith(tableName, "test-")); remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/96e274a243ff
Assignee | ||
Updated•10 years ago
|
Blocks: downloadprotection
Comment 8•10 years ago
|
||
Backed out for failing to compile: https://tbpl.mozilla.org/php/getParsedLog.php?id=38252431&tree=Mozilla-Inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f20e885fa3a
Assignee | ||
Comment 9•10 years ago
|
||
Sorry Ed, I failed to qref.
Assignee | ||
Comment 10•10 years ago
|
||
Argh, a difference between unified and non-unified builds. 0:08.99 from /home/mmc/mozilla-central/obj-x86_64-unknown-linux-gnu/toolkit/components/url-classifier/Unified_cpp_url-classifier0.cpp:67: 0:08.99 ../../../dist/include/nsStringAPI.h:12:2: error: #error nsStringAPI.h is only usable from non-MOZILLA_INTERNAL_API code! Guess I'm not supposed to use StringBeginsWith.
Comment 11•10 years ago
|
||
(In reply to Monica Chew [:mmc] (please use needinfo) from comment #9) > Sorry Ed, I failed to qref. No worries :-)
Comment 12•10 years ago
|
||
>Guess I'm not supposed to use StringBeginsWith.
It's absolutely usable form within url-classifier. I think you just got the wrong include? nsString.h instead of nsStringAPI.h or something like that?
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8409273 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8410325 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8410329 [details] [diff] [review] Fix gethash completions, urlclassifier.malware_table and urlclassifier.phish_table may be comma-separated lists ( Review of attachment 8410329 [details] [diff] [review]: ----------------------------------------------------------------- gcp, you are right, I shouldn't include headers where the function is actually declared ;) Carrying over r+, will check in when tree is open.
Attachment #8410329 -
Flags: review+
Assignee | ||
Comment 16•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/73bb0c592bf1
Comment 17•10 years ago
|
||
Comment on attachment 8410329 [details] [diff] [review] Fix gethash completions, urlclassifier.malware_table and urlclassifier.phish_table may be comma-separated lists ( >@@ -1405,16 +1433,19 @@ nsUrlClassifierDBService::GetCompleter(c >+ // We should never fetch hash completions for test tables. >+ MOZ_ASSERT(!StringBeginsWith(tableName, "test-")); This seems to have busted inbound: https://tbpl.mozilla.org/php/getParsedLog.php?id=38255700&tree=Mozilla-Inbound StringBeginsWith takes nsAString / nsACString, not a char*. MXR suggests you probably want to call it with NS_LITERAL_STRING(
Comment 18•10 years ago
|
||
Backed out following our IRC conversation :-) remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/22e57c335fc5
Comment 19•10 years ago
|
||
While you're fixing that MOZ_ASSERT, I'd suggest also moving the comment into the MOZ_ASSERT, to make it more meaningful and to provide a bit more information if/when it fails. e.g.: MOZ_ASSERT(!StringBeginsWith(tableName, NS_LITERAL_STRING"test-")), "We should never fetch hash completions for test tables");
Assignee | ||
Comment 20•10 years ago
|
||
Thanks dholbert, I didn't know that MOZ_ASSERT took a second argument! Try: https://tbpl.mozilla.org/?tree=Try&rev=a19d26c400a5
Assignee | ||
Comment 21•10 years ago
|
||
Try was green, one more time: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f78e4ede49
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c8f78e4ede49
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8410329 -
Attachment is obsolete: true
Assignee | ||
Comment 24•10 years ago
|
||
Comment on attachment 8411167 [details] [diff] [review] Fix gethash completions, urlclassifier.malware_table and urlclassifier.phish_table may be comma-separated lists ( [Approval Request Comment] Bug caused by (feature/regressing bug #): 998370, see https://bugzilla.mozilla.org/show_bug.cgi?id=998370#c5 User impact if declined: safebrowsing phishing and malware interstitials will not appear in FF 30 Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): repeating the same backouts as in 998370 String or IDL/UUID changes made by this patch: none
Attachment #8411167 -
Flags: review+
Attachment #8411167 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8411167 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•