1264517 - Remove query string parameters from debug output

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[François Marier [:francois]]

We should make sure that the output of browser.safebrowsing.debug as well as the various Safe Browsing MOZ_LOG_MODULES don't include the query string parameters that are part of the URLs.

Comment 1

[Henry Chang [:hchang]]

Note:

Turn on "browser.safebrowsing.debug" and touch "browser.safebrowsing.provider.google.lastupdatetime" to trigger an list update from google server.

Comment 2

[Henry Chang [:hchang]]

An example that reveals the api-key

listmanager: 10:04:54 GMT+0800 (CST): needsUpdate: {
  "https://safebrowsing.google.com/safebrowsing/downloads?client=Firefox&appver=48.0a1&pver=2.2&key=no-google-api-key": {
    "goog-phish-shavar": true,
    "goog-malware-shavar": true,
    "goog-unwanted-shavar": true,
    "goog-badbinurl-shavar": true
  },
  "https://shavar.services.mozilla.com/downloads?client=Firefox&appver=48.0a1&pver=2.2": {
    "mozstd-track-digest256": true,
    "mozstd-trackwhite-digest256": true,
    "mozplugin-block-digest256": false
  }
}

Comment 3

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1264517-Part-1-Add-interface-for-removing-sensit.patch

Comment 4

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1264517-Part-2-Dump-update-URL-w-o-API-key-in-th.patch

Comment 5

[Henry Chang [:hchang]]

Hi Francois,

I just attached two patches for removing API key from the debugging message. I haven't confirmed if I catch all the cases where the API key is revealed. Besides, instead of removing entire query string, the patch only removes the API key since other query info might be helpful for debugging. Requesting for your initial feedback and will have a thorough scan of the all relevant debug message. Thanks :)

Comment 6

[François Marier [:francois]]

Comment on attachment 8742225 [details] [diff] [review]
0001-Bug-1264517-Part-1-Add-interface-for-removing-sensit.patch

Review of attachment 8742225 [details] [diff] [review]:
-----------------------------------------------------------------

I think that's a great approach.

::: toolkit/components/url-classifier/nsIUrlListManager.idl
@@ +65,5 @@
>      void safeLookup(in nsIPrincipal key,
>                      in nsIUrlListManagerCallback cb);
> +
> +    /**
> +     * Remove all the sensitive query string from the given URL. Usually

nit: I would say "Remove all of the sensitive query string parameters from the given URL."

::: toolkit/components/url-classifier/tests/unit/test_listmanager.js
@@ +1,1 @@
> +let listManager = Cc["@mozilla.org/url-classifier/listmanager;1"]

Thanks for including a test :)

Comment 7

[François Marier [:francois]]

Comment on attachment 8742284 [details] [diff] [review]
0001-Bug-1264517-Part-2-Dump-update-URL-w-o-API-key-in-th.patch

Review of attachment 8742284 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/content/listmanager.js
@@ +208,5 @@
>    // If the user has never downloaded tables, do the check now.
> +  Object.keys(this.needsUpdate_).forEach((url) => {
> +    // The key of |this.needsUpdate_| is the "updateUrl".
> +    let tables = this.needsUpdate_[url];
> +    log(this.removeSensitiveQuery(url) + ": \n" + JSON.stringify(tables, undefined, 2));

Maybe we should keep the string "needsUpdate" somewhere in there to help identify what these log lines are about?

I'm thinking something like: "[needsUpdate] " + removeSensitiveQuery(url) + ": \n" + ...

Comment 8

[Henry Chang [:hchang]]

Attachment: Part 1: Add function to remove sensitive info from URL

Comment 9

[Henry Chang [:hchang]]

Attachment: Part 2: Dump URL with no API key

Comment 10

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6603a34de31

Comment 11

[François Marier [:francois]]

Comment on attachment 8742703 [details] [diff] [review]
Part 2: Dump URL with no API key

Review of attachment 8742703 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, I forgot about the hash completer. I suspect these messages also leak the API key:

https://dxr.mozilla.org/mozilla-central/rev/67ac40fb8f680ea5e03805552187ba1b5e8392a1/toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js#316
https://dxr.mozilla.org/mozilla-central/rev/67ac40fb8f680ea5e03805552187ba1b5e8392a1/toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js#340

Comment 12

[François Marier [:francois]]

Another place where we could leak the full URL is inside the MOZ_LOG messages.

If you export these:

NSPR_LOG_MODULES="UrlClassifierDbService:5,nsChannelClassifier:5,UrlClassifierProtocolParser:5,UrlClassifierStreamUpdater:5,UrlClassifierPrefixSet:5"

then look at:

1. list updates
2. classification of URLs (i.e. you open any webpage)
3. hash completion

you might see API keys in the debug output.

You can force list updates (#1) by setting `browser.safebrowsing.provider.google.nextupdatetime` to "1" and then restarting the browser. To force a hash completion (#3), the easiest would be to visit a real phishing site by following links on https://www.phishtank.com/.

Comment 13

[Henry Chang [:hchang]]

Attachment: exposure_spots.txt

Comment 14

[Henry Chang [:hchang]]

Attachment: exposed_keys.txt

Comment 15

[Henry Chang [:hchang]]

Attachment: Part 2: Dump URL with no API key (updated)

Comment 16

[Henry Chang [:hchang]]

Attachment: Part 2: Dump URL with no API key (updated)

Comment 17

[Henry Chang [:hchang]]

Attachment: Part 2: Dump URL with no API key (updated)

Comment 18

[Henry Chang [:hchang]]

Comment on attachment 8743193 [details] [diff] [review]
Part 2: Dump URL with no API key (updated)

Hi Francois,

I've followed your instruction to catch more google api key revealing spots. Besides, I also reviewed the entire "nsUrlClassifierStreamUpdater.cpp" to trim the update URLs. 

By the way, the gethash URL contains no API key [1]. However, I still update the hash completer to avoid API key being added in the future.

Thanks :)

[1] https://dxr.mozilla.org/mozilla-central/search?q=browser.safebrowsing.provider.google.gethashURL&redirect=false&case=false

Comment 19

[François Marier [:francois]]

Comment on attachment 8743193 [details] [diff] [review]
Part 2: Dump URL with no API key (updated)

Review of attachment 8743193 [details] [diff] [review]:
-----------------------------------------------------------------

That looks fine to me. Flagging gcp to make sure he's okay with that approach.

Comment 20

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8743193 [details] [diff] [review]
Part 2: Dump URL with no API key (updated)

Review of attachment 8743193 [details] [diff] [review]:
-----------------------------------------------------------------

Ok for me, but I think you could've made this simpler, because you can know at the moment the code runs what the sensitive information (i.e. the actual key) is, so instead of having the exfiltrate all the URLs from the logging and do more string fiddling to find the query parameters, you could've just piped all the logging through a function that does a s/WHATEVERTHEAPIKEYIS/THEKEYWASHERE/g. There's also less risk of missing anything, and it's handier for debugging to see that they key parameter was actually there.

What do you think?

Comment 21

[François Marier [:francois]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #20)
> you could've just
> piped all the logging through a function that does a
> s/WHATEVERTHEAPIKEYIS/THEKEYWASHERE/g. There's also less risk of missing
> anything, and it's handier for debugging to see that they key parameter was
> actually there.

The downside of that would be that the API key would be very easily visible in dxr.mozilla.org (and possibly even discoverable) and that we would probably forget to update that cleanup function if we ever need to change API keys. I think I prefer Henry's approach, even though it's a bit more complicated.

Comment 22

[Gian-Carlo Pascutto [:gcp]]

(In reply to François Marier [:francois] from comment #21)
> The downside of that would be that the API key would be very easily visible
> in dxr.mozilla.org (and possibly even discoverable) and that we would
> probably forget to update that cleanup function if we ever need to change
> API keys. I think I prefer Henry's approach, even though it's a bit more
> complicated.

What? No, not at all. The API key shouldn't ever be *hardcoded* in the source. Just *retrieve* it from the builders in the same way that SafeBrowsing already does:

browser.safebrowsing.provider.google.updateURL=
"https://safebrowsing.google.com/safebrowsing/downloads?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2&key=%GOOGLE_API_KEY%"

I'm suggesting the filtering function use the %GOOGLE_API_KEY% lookup and s/%GOOGLE_API_KEY%/API_KEY_WAS_HERE/g. Then you can just pipe all the log text through it.

Comment 23

[François Marier [:francois]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #22)
> I'm suggesting the filtering function use the %GOOGLE_API_KEY% lookup and
> s/%GOOGLE_API_KEY%/API_KEY_WAS_HERE/g. Then you can just pipe all the log
> text through it.

Ah, OK that makes more sense :)

Comment 24

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8743193 [details] [diff] [review]
Part 2: Dump URL with no API key (updated)

Review of attachment 8743193 [details] [diff] [review]:
-----------------------------------------------------------------

I'm pulling my r+ because the more I think about it, the more I think the alternate approach suggested would make the resulting code a lot cleaner (you can fix it in log() once for all the JS!). Henry, can you investigate whether you can get it working?

Comment 25

[Henry Chang [:hchang]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #24)
> Comment on attachment 8743193 [details] [diff] [review]
> Part 2: Dump URL with no API key (updated)
> 
> Review of attachment 8743193 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm pulling my r+ because the more I think about it, the more I think the
> alternate approach suggested would make the resulting code a lot cleaner
> (you can fix it in log() once for all the JS!). Henry, can you investigate
> whether you can get it working?

Definitely! BTW, do you think it's fine to fix it in log() (I guess you meant dump()) for all the JS? If in |log| (which calls dump() internally), we still have to fix two JS files (listmanager/hashcompleter).

Thanks :)

Comment 26

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1264517-Replace-sensitive-info-from-debugging-me.patch

Comment 27

[Henry Chang [:hchang]]

Comment on attachment 8744840 [details] [diff] [review]
0001-Bug-1264517-Replace-sensitive-info-from-debugging-me.patch

Hi gcp,

Could you have a review for this patch? It's implemented by following comment 22.  What I'd like to point out in the patch are:

1) To make the api key replacement consistent, nsIURLFormatter::trimSensitiveURL is added as a utility function to trim sensitive info from the given string even though the replacement is super easy to code. (actually preprocessing the JS file like listmanager.js is a hassle.)

2) HashCompleter doesn't always use |log()| to output debug message so that some uses of |dump| needs to be changed to |log|

Thanks!

Comment 28

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8744840 [details] [diff] [review]
0001-Bug-1264517-Replace-sensitive-info-from-debugging-me.patch

Review of attachment 8744840 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like feedback on the following before r+.

::: toolkit/components/url-classifier/SafeBrowsing.jsm
@@ +23,5 @@
>    }
>  
>    var d = new Date();
>    let msg = "SafeBrowsing: " + d.toTimeString() + ": " + stuff.join(" ");
> +  dump(Services.urlFormatter.trimSensitiveURL(msg) + "\n");

The "msg" might contain multiple URLs, so plural (trimSensitiveURLs) is more consistent.

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
@@ +32,5 @@
>  #undef LOG
>  
>  // NSPR_LOG_MODULES=UrlClassifierStreamUpdater:5
>  static mozilla::LazyLogModule gUrlClassifierStreamUpdaterLog("UrlClassifierStreamUpdater");
> +#define LOG(args) TrimAndLog args

Does this work? I'm baffled.

::: toolkit/components/urlformatter/nsURLFormatter.js
@@ +160,5 @@
> +
> +  trimSensitiveURL: function uf_trimSensitiveURL(aMsg) {
> +    // Only the google API key is sensitive for now.
> +    return aMsg.replace("key=@MOZ_GOOGLE_API_KEY@",
> +                        "key=[trimmed-google-api-key]", "g");

You might as well scan for @MOZ_GOOGLE_API_KEY@ without they key= prefix. This will keep things working even if the query parameter format changes, as the key is (essentially guaranteed) to be unique.

On the other hand, you might want to safeguard against the key being empty then.

Comment 29

[Gian-Carlo Pascutto [:gcp]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #28)
> > +#define LOG(args) TrimAndLog args
> 
> Does this work? I'm baffled.

I'm pretty sure it should be

LOG(...) TrimAndLog(__VA_ARGS__)

or something similar.

Comment 30

[Henry Chang [:hchang]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #28)
> Comment on attachment 8744840 [details] [diff] [review]
> 0001-Bug-1264517-Replace-sensitive-info-from-debugging-me.patch
> 
> Review of attachment 8744840 [details] [diff] [review]:
> -----------------------------------------------------------------
> > +#define LOG(args) TrimAndLog args
> 
> Does this work? I'm baffled.
>

Yes it does work since all the call sites of "LOG" has double () like (("this is %p", this)).
So, 

LOG(("this is %p, this")) 

will be expanded to

TrimAndLog("this is %p, this")
 
> ::: toolkit/components/urlformatter/nsURLFormatter.js
> @@ +160,5 @@
> > +
> > +  trimSensitiveURL: function uf_trimSensitiveURL(aMsg) {
> > +    // Only the google API key is sensitive for now.
> > +    return aMsg.replace("key=@MOZ_GOOGLE_API_KEY@",
> > +                        "key=[trimmed-google-api-key]", "g");
> 
> You might as well scan for @MOZ_GOOGLE_API_KEY@ without they key= prefix.
> This will keep things working even if the query parameter format changes, as
> the key is (essentially guaranteed) to be unique.
> 

My argument is @MOZ_GOOGLE_API_KEY@ might be too general (in nightly build it's like google-no-api-key).
If we assume it's always unique, it makes sense on replacing the key only :)


> On the other hand, you might want to safeguard against the key being empty
> then.

Thanks for the review!

Comment 31

[Henry Chang [:hchang]]

Attachment: Patch v2

Comment 32

[Henry Chang [:hchang]]

Comment on attachment 8746417 [details] [diff] [review]
Patch v2

Hi gcp,

The new patch addresses all your comments in the previous review (except the #define since it does work). Could you review again? Thanks :)

Comment 33

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fec363de681d

Comment 34

[Henry Chang [:hchang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e84e43dd84fde45411583422464ff83b1334012
Bug 1264517 - Replace sensitive info from debugging message. r=gcp.

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e84e43dd84f

Comment 36

[Frank-Rainer Grahl (:frg)]

You may want to revise the patch.

>> Timestamp: 4/30/2016 9:20:15 PM
>> Warning: flags argument of String.prototype.{search,match,replace} is no longer supported
>> Source File: resource://gre/components/nsURLFormatter.js
>> Line: 161

Comment 37

[Henry Chang [:hchang]]

Attachment: Remove warning

Comment 38

[Henry Chang [:hchang]]

Hi Francois, gcp,

I just revised the patch but didn't have a test case since I haven't figured out if it's a good idea to have preprocessing @MOZ_GOOGLE_API_KEY@ in the test case. I didn't find any test cases in tree having preprocessing string. Directly exposing nsURLFormatterService._defaults.GOOGLE_API_KEY might be also solution. What do you think of? Thanks :)

Comment 39

[Ethan Tseng [:ethan]]

Reopen this bug due to the warning addressed in comment 36.

Comment 40

[Gian-Carlo Pascutto [:gcp]]

I'm not sure exposing the key through JS is a good idea (but I'm not sure about any security boundaries there). No idea about preprocessing and tests.

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d032f77a55f3

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d032f77a55f3

Comment 43

[Frank-Rainer Grahl (:frg)]

Again me. Not sure if this intentional but the listmanager still happily prints the keys in clear in the web console using logStringMessage:

>>  Services.console.logStringMessage(msg);
>>  dump(Services.urlFormatter.trimSensitiveURLs(msg) + "\n");

Comment 44

[Henry Chang [:hchang]]

(In reply to Frank-Rainer Grahl from comment #43)
> Again me. Not sure if this intentional but the listmanager still happily
> prints the keys in clear in the web console using logStringMessage:
> 
> >>  Services.console.logStringMessage(msg);
> >>  dump(Services.urlFormatter.trimSensitiveURLs(msg) + "\n");

I would feel it's not intentional :(

Comment 45

[François Marier [:francois]]

(In reply to Frank-Rainer Grahl from comment #43)
> Again me. Not sure if this intentional but the listmanager still happily
> prints the keys in clear in the web console using logStringMessage:
> 
> >>  Services.console.logStringMessage(msg);
> >>  dump(Services.urlFormatter.trimSensitiveURLs(msg) + "\n");

Oops, looks like we all missed that one :(

Henry, do you mind doing a follow-up commit? We can probably just remove the "Services.console.logStringMessage()" line since we're already outputting the message using "dump()".

Comment 46

[Frank-Rainer Grahl (:frg)]

This would rip out a useful message if you do want a quick look what's going on. There are other messages in the log which do not make much sense without this one in it.

I would change it to 

Services.console.logStringMessage(Services.urlFormatter.trimSensitiveURLs(msg));

Comment 47

[Henry Chang [:hchang]]

Attachment: Remove API key from console log

Comment 48

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e3dc7d38d95

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e3dc7d38d95