Closed Bug 998396 Opened 6 years ago Closed 6 years ago

Fix gethash completions in Aurora and Nightly

Categories

(Toolkit :: Safe Browsing, defect)

x86_64
Linux
defect
Not set

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: nobody → mmc
Status: NEW → ASSIGNED
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)
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+
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.
Thanks, will rename before checking in. I did an MXR search and these are the only places where these prefs are used.
(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
Sorry Ed, I failed to qref.
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.
(In reply to Monica Chew [:mmc] (please use needinfo) from comment #9)
> Sorry Ed, I failed to qref.

No worries :-)
>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?
Attachment #8409273 - Attachment is obsolete: true
Attachment #8410325 - Attachment is obsolete: true
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+
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(
Backed out following our IRC conversation :-)
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/22e57c335fc5
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");
Thanks dholbert, I didn't know that MOZ_ASSERT took a second argument!

Try: https://tbpl.mozilla.org/?tree=Try&rev=a19d26c400a5
https://hg.mozilla.org/mozilla-central/rev/c8f78e4ede49
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Attachment #8410329 - Attachment is obsolete: true
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?
Attachment #8411167 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → Toolkit
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.