Remove query string parameters from debug output

RESOLVED FIXED in Firefox 49

Status

()

Toolkit
Safe Browsing
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: francois, Assigned: hchang)

Tracking

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: tpe-seceng)

Attachments

(3 attachments, 10 obsolete attachments)

9.33 KB, patch
gcp
: review+
Details | Diff | Splinter Review
2.45 KB, patch
gcp
: review+
Details | Diff | Splinter Review
1.28 KB, patch
gcp
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → hchang
(Assignee)

Comment 1

2 years ago
Note:

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

Comment 2

2 years ago
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
  }
}
(Assignee)

Comment 3

2 years ago
Created attachment 8742225 [details] [diff] [review]
0001-Bug-1264517-Part-1-Add-interface-for-removing-sensit.patch
(Assignee)

Comment 4

2 years ago
Created attachment 8742284 [details] [diff] [review]
0001-Bug-1264517-Part-2-Dump-update-URL-w-o-API-key-in-th.patch
(Assignee)

Comment 5

2 years ago
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 :)
Flags: needinfo?(francois)
(Reporter)

Comment 6

2 years ago
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 :)
Attachment #8742225 - Flags: feedback+
(Reporter)

Comment 7

2 years ago
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" + ...
Attachment #8742284 - Flags: feedback+
(Reporter)

Updated

2 years ago
Flags: needinfo?(francois)
(Assignee)

Comment 8

2 years ago
Created attachment 8742702 [details] [diff] [review]
Part 1: Add function to remove sensitive info from URL
Attachment #8742225 - Attachment is obsolete: true
Attachment #8742284 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
Created attachment 8742703 [details] [diff] [review]
Part 2: Dump URL with no API key
Attachment #8742702 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8742702 - Attachment is obsolete: false
(Assignee)

Updated

2 years ago
Attachment #8742702 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8742703 - Flags: review?(francois)
(Reporter)

Updated

2 years ago
Attachment #8742702 - Flags: review?(francois) → review+
(Reporter)

Updated

2 years ago
Attachment #8742703 - Flags: review?(francois) → review+
(Reporter)

Comment 12

2 years ago
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/.
(Assignee)

Comment 13

2 years ago
Created attachment 8743118 [details]
exposure_spots.txt
(Assignee)

Comment 14

2 years ago
Created attachment 8743161 [details]
exposed_keys.txt
Attachment #8743118 - Attachment is obsolete: true
(Assignee)

Comment 15

2 years ago
Created attachment 8743185 [details] [diff] [review]
Part 2: Dump URL with no API key (updated)
(Assignee)

Comment 16

2 years ago
Created attachment 8743186 [details] [diff] [review]
Part 2: Dump URL with no API key (updated)
Attachment #8742703 - Attachment is obsolete: true
Attachment #8743185 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
Created attachment 8743193 [details] [diff] [review]
Part 2: Dump URL with no API key (updated)
Attachment #8743186 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
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
Attachment #8743193 - Flags: review?(francois)
(Reporter)

Comment 19

2 years ago
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.
Attachment #8743193 - Flags: review?(gpascutto)
Attachment #8743193 - Flags: review?(francois)
Attachment #8743193 - Flags: review+
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?
Attachment #8743193 - Flags: review?(gpascutto) → review+
(Reporter)

Comment 21

2 years ago
(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.
(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.
(Reporter)

Comment 23

2 years ago
(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 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?
Attachment #8743193 - Flags: review+
(Assignee)

Comment 25

2 years ago
(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 :)
Flags: needinfo?(gpascutto)
(Assignee)

Comment 26

2 years ago
Created attachment 8744840 [details] [diff] [review]
0001-Bug-1264517-Replace-sensitive-info-from-debugging-me.patch
Attachment #8742702 - Attachment is obsolete: true
Attachment #8743161 - Attachment is obsolete: true
Attachment #8743193 - Attachment is obsolete: true
(Assignee)

Comment 27

2 years ago
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!
Flags: needinfo?(gpascutto)
Attachment #8744840 - Flags: review?(gpascutto)
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.
Attachment #8744840 - Flags: review?(gpascutto) → review-
(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.
(Assignee)

Comment 30

2 years ago
(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!
(Assignee)

Comment 31

2 years ago
Created attachment 8746417 [details] [diff] [review]
Patch v2
Attachment #8744840 - Attachment is obsolete: true
(Assignee)

Comment 32

2 years ago
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 :)
Attachment #8746417 - Flags: review?(gpascutto)
Attachment #8746417 - Flags: review?(gpascutto) → review+

Comment 35

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e84e43dd84f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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
(Assignee)

Comment 37

2 years ago
Created attachment 8747511 [details] [diff] [review]
Remove warning
(Assignee)

Comment 38

2 years ago
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 :)
Flags: needinfo?(gpascutto)
Flags: needinfo?(francois)
Reopen this bug due to the warning addressed in comment 36.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8747511 - Flags: review+
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.
Flags: needinfo?(gpascutto)
(Reporter)

Updated

2 years ago
Flags: needinfo?(francois)

Comment 42

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d032f77a55f3
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
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");
(Assignee)

Comment 44

2 years ago
(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 :(
(Reporter)

Comment 45

2 years ago
(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()".
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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));
(Assignee)

Comment 47

2 years ago
Created attachment 8750301 [details] [diff] [review]
Remove API key from console log
(Assignee)

Updated

2 years ago
Attachment #8750301 - Flags: review?(francois)
Attachment #8750301 - Flags: review?(francois) → review+

Comment 49

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1e3dc7d38d95
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.