1354633 - blank MediaError.message when resisting fingerprinting

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Thorin [:thorin]]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170316213829

Steps to reproduce:

MediaError.message ( https://developer.mozilla.org/en-US/docs/Web/API/MediaError/message ) landed in FF52 (https://bugzilla.mozilla.org/show_bug.cgi?id=1322606)


Actual results:

FF52 also removed dom.MediaError.message.enabled which had been used previously in Aurora etc (see: https://bugzilla.mozilla.org/show_bug.cgi?id=1322606#c9 and https://reviewboard.mozilla.org/r/97800/diff/2#index_header )


Expected results:

Relevant TOR ticket: https://trac.torproject.org/projects/tor/ticket/21792
Relevant API: https://developer.mozilla.org/en-US/docs/Web/API/MediaError

Allow MediaError.code, but re-enable blocking of MediaError.message which is a (potential?/real?) privacy/tracking concern.

Should this be part of the Tor Uplift? Or tied to privacy.resistFingerprinting?

Comment 1

[Jean-Yves Avenard [:jya]]

MediaError.message is now part of the HTML5 spec, there's never any private information added in there.

It only ever shows the path of the function that caused the problem and this never include user's information.. this can't be used to track someone any more than the user agent would.

Comment 2

[Arthur Edelstein [:arthur]]

It looked to me that fingerprintable information might be inadvertently exposed in the future. So, to be extra safe, in Tor Browser we return a blank MediaError.message when privacy.resistFingerprinting is true. We'd like to propose uplifting this patch.

Current patch: https://torpat.ch/21792
Tor ticket: https://bugs.torproject.org/21792

Comment 3

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1354633 - When privacy.resistFingerprinting = true, MediaError.message can only get whitelisted messages

Review commit: https://reviewboard.mozilla.org/r/164856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164856/

Comment 4

[Chung-Sheng Fu [:cfu]]

Will there be any concern about blanking MediaError.message?
If this patch is fine, I will add some tests.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8893752 [details]
Bug 1354633 - When privacy.resistFingerprinting = true, MediaError.message can only get whitelisted messages

https://reviewboard.mozilla.org/r/164856/#review170634

As said in comment 1, I don't believe we have put private information in the error message. Please provide analysis or reports that show fingerprinting is done through MediaError.message.

Comment 6

[Tom Ritter [:tjr]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #5)
> As said in comment 1, I don't believe we have put private information in the
> error message. Please provide analysis or reports that show fingerprinting
> is done through MediaError.message.

I agree that currently this doesn't seem to provide any fingerprinting mechanism. However, we are concerned that it could in the future. Some error message may be passed in directly that leaks a path name, or a codec error might reveal that a user doesn't have enough memory to process a video.

I considered trying to reverse direction and write tests that ensure that no new error messages could leak information, but that seems impossible to be able to enumerate all future error conditions. The future concern is not without precedent - while they aren't perfectly analogous, there are a few examples of this happening in the past. The battery status API and the resource:// URI schemes both become significant fingerprinting vectors well after their introduction.

So we'd like to future proof this with the small patch to ensure that this doesn't become a fingerprinting vector in the future either by accident or as attached to a feature.

Comment 7

[Arthur Edelstein [:arthur]]

Comment on attachment 8893752 [details]
Bug 1354633 - When privacy.resistFingerprinting = true, MediaError.message can only get whitelisted messages

https://reviewboard.mozilla.org/r/164856/#review171942

Code looks good, but happy to continue the discussion on whether this patch is desirable.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

I don't like to cripple the user experience in the name of privacy. Actually, privacy is a bonus after you have a decent/usable browser.

Bug 1374510 is an example where Firefox used to show "not supported" instead of "404 not found" when the requested file doesn't exist at all. This gives the user a wrong impression that this video format is not supported by Firefox and he should find luck in other browsers.

https://groups.google.com/forum/#!searchin/mozilla.dev.platform/data$20review|sort:relevance/mozilla.dev.platform/KiW8MMQVhHs/_RPNA0cDBwAJ
As we require data review when using the MOZ_CRASH_UNSAFE_* macros, we should do the same when adding logs to MediaError.message.

Comment 9

[Ethan Tseng [:ethan]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #8)
> I don't like to cripple the user experience in the name of privacy.
> Actually, privacy is a bonus after you have a decent/usable browser.

Hi JW,

I agree with you on this point.
However, I believe this patch is not a concern to the user experience.

1. Fingerprinting resistance is behind a pref, and it's off by default.
   The message will be blanked only when the user opts in to turn on the feature,
   and he/she will understand it's a trade-off between full functionality and privacy.

2. MediaError.message is a debugging aid for developers.
   Regular users will not notice the message is blank or not.
   This patch does not affect usability or web compatibility.

Again, we understand there is no fingerprinting surface from MediaError.message for now.
But we want to prevent the potential risk when the message is changed in the future.

Could you reconsider the patch?

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Ethan Tseng [:ethan] from comment #9)
> 1. Fingerprinting resistance is behind a pref, and it's off by default.
>    The message will be blanked only when the user opts in to turn on the feature,
>    and he/she will understand it's a trade-off between full functionality and privacy.
I am afraid that the user doesn't have the full knowledge to tell whether malfunction comes from anti-fingerprinting or a bug in the browser. I believe most of them will just assume the later.

> 2. MediaError.message is a debugging aid for developers.
>    Regular users will not notice the message is blank or not.
>    This patch does not affect usability or web compatibility.
See bug 1381375. The field is essential for the UI to show the correct error message to the user.

Comment 11

[Blake Wu [:bwu][:blakewu]]

Maybe we could consider showing something like "Error message is blocked due to fingerprinting" when the pref of Fingerprinting resistance is on.

Comment 12

[Ethan Tseng [:ethan]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #11)
> Maybe we could consider showing something like "Error message is blocked due
> to fingerprinting" when the pref of Fingerprinting resistance is on.

We can consider this option. :)


(In reply to JW Wang [:jwwang] [:jw_wang] from comment #10)
> > 2. MediaError.message is a debugging aid for developers.
> >    Regular users will not notice the message is blank or not.
> >    This patch does not affect usability or web compatibility.
> See bug 1381375. The field is essential for the UI to show the correct error
> message to the user.

Another compromise is we whitelist only the errors that we want to expose in bug 1381375,
and block all others.

What do you think, JW?

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

I prefer the whitelist approach. Check https://bugzilla.mozilla.org/show_bug.cgi?id=1376004#c10 for the format.

Comment 14

[Ethan Tseng [:ethan]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #13)
> I prefer the whitelist approach. Check
> https://bugzilla.mozilla.org/show_bug.cgi?id=1376004#c10 for the format.
Thanks, JW!
Let's move forward using this solution.

Comment 15

[Chung-Sheng Fu [:cfu]]

I'm going to add a filter.

When privacy.resistFingerprinting = true, if the error message starts with 3 digits and a colon, it will be whitelisted; otherwise, blank the message.

It will look like

>  void
>  MediaError::GetMessage(nsAString& aResult) const
>  {
> +  if (!nsContentUtils::IsCallerChrome() &&
> +      nsContentUtils::ShouldResistFingerprinting() &&
> +      !std::regex_search(mMessage.get(), std::regex("^\\d{3}: "))) {
> +    aResult.Truncate();
> +    return;
> +  }
> +
>    CopyUTF8toUTF16(mMessage, aResult);
>  }

Here are some questions:

1. Should I further filter valid HTTP status codes?

2. Will it be safer to return only the HTTP status code, without the response text?
   However, it seems that front-end has not determined how to parse the error message format yet.
   If we would like to do this, I will ask their opinion in bug 1381375.

Comment 16

[Georg Koppen]

I wonder whether we could write a test making sure that the error message is non-localized (i.e. always english) and non-platform dependent. Because if anti-fingerprinting mode is enabled then we don't want to leak those bits.

The reasoning in comment 10 makes me think that e.g. localization will be needed/wanted at some point. I mean if users *really* believe that the fingerprinting defense they enabled breaks the browser because there is no error message visible, they will surely believe the browser is buggy when this is the only non-localized message they see (especially when they are not capable of understanding english).

If the localization is happening at a level which is beyond the reach of content then that's fine. But I want to be sure that neither localized nor platform-dependent error messages get introduced in the future without notice.

Comment 17

[Arthur Edelstein [:arthur]]

(In reply to Chung-Sheng Fu [:cfu] from comment #15)

> 2. Will it be safer to return only the HTTP status code, without the
> response text?

Yes. The response text might still be fingerprintable if we only filter by HTTP status code.

I think a better solution would be to validate the whole message. That would require examining the codebase for all possible error message strings, and collecting them in a whitelist.

Comment 18

[Chung-Sheng Fu [:cfu]]

At this point, only decode error and load error have values in MediaError.message.

Both kinds of error message are not localized.

Decode error messages are hard-coded and there are no localization strings for them.

Currently decode error messages are for developers' debugging use so it looks fine to me to just blank them.

Load error messages are now required by UI to recognize the reason, in order for UI to correctly show not supported or not found (bug 1376004).

When the browser fails to load some media source through a HTTP channel, MediaError.message will be assigned with

> <status code>: <response text>
http://searchfox.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#547

I believe the response text has no chance to be localized because it is either

1. hard-coded
   http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpResponseHead.cpp#381

2. gathered from the HTTP response
   http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpResponseHead.cpp#578

As I understand, we only have to allow this kind of message.

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Arthur Edelstein [:arthuredelstein] from comment #17)
> I think a better solution would be to validate the whole message. That would
> require examining the codebase for all possible error message strings, and
> collecting them in a whitelist.
I share the same opinion that we should verity the whole message instead of matching patterns. As said in comment 6, if it is possible to leak user data through the Batter API, it is also possible for HTTP response text to leak user data in the future.

Comment 20

[Chung-Sheng Fu [:cfu]]

Attachment: 0001-Bug-1354633-Blank-MediaError.message-when-resisting-.patch

Does this approach look fine to you?

I really don't think we should allow decode error messages, so now only load error messages are listed.

It seems that currently UI only cares about the HTTP 404 case, right?

We can add more messages to the list if UI wants to handle other cases.

Comment 21

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8897709 [details] [diff] [review]
0001-Bug-1354633-Blank-MediaError.message-when-resisting-.patch

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

Please add a test to ensure the whitelist is valid and up-to-date even when its dependency (e.g. nsHttpResponseHead::AssignDefaultStatusText()) changes. E.g. someone accidentally change "Not Found" to "Not Found.".

Comment 22

[Arthur Edelstein [:arthur]]

(In reply to Chung-Sheng Fu [:cfu] from comment #20)
> Created attachment 8897709 [details] [diff] [review]
> 0001-Bug-1354633-Blank-MediaError.message-when-resisting-.patch
> 
> Does this approach look fine to you?

Hi Chung-Sheng -- this basic approach looks good to me. But if a developer wants to add a new error message somewhere in the codebase, they may not be aware that the message will be blocked by this code when "privacy.resistFingerprinting" is enabled.

I can think of some alternative possible solutions:
  1. Include a debug assert, independent of the state of privacy.resistFingerprinting that alerts developers of a non-whitelisted error message.
  2. Change this patch to always filter error messages, independent of privacy.resistFingerprinting. (Requires that all existing messages be whitelisted.)
  3. Do both 1 and 2.

Comment 23

[Chung-Sheng Fu [:cfu]]

Attachment: 0001-Alert-developers-of-non-whitelisted-error-messages.patch

When content script accesses a non-whitelisted MediaError.message, a warning will be print to JavaScript console (and our debug log, if possible) to alert developers.

Developers going to change the messages or add new messages will notice the warning when they run tests.

I think this can help with the whitelist consistency in some respects.

What do you think of it?

Comment 24

[Arthur Edelstein [:arthur]]

(In reply to Chung-Sheng Fu [:cfu] from comment #23)
> Created attachment 8898233 [details] [diff] [review]
> 0001-Alert-developers-of-non-whitelisted-error-messages.patch
> 
> When content script accesses a non-whitelisted MediaError.message, a warning
> will be print to JavaScript console (and our debug log, if possible) to
> alert developers.
> 
> Developers going to change the messages or add new messages will notice the
> warning when they run tests.
> 
> I think this can help with the whitelist consistency in some respects.
> 
> What do you think of it?

That looks good. In the warning, I would somehow mention that the MediaError.message needs to be added to the whitelist, so developers know how to solve the problem.

Comment 25

[Chung-Sheng Fu [:cfu]]

It is exactly important.  Sorry I missed it.

The warning will look like:

"This error message will be blank when privacy.resistFingerprinting = true.  If it is really necessary, please add it to the whitelist in MediaError::GetMessage: " + mMessage

Is it better?

Comment 26

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1354633 - Add test cases

Review commit: https://reviewboard.mozilla.org/r/170554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170554/

Comment 27

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8893752 [details]
Bug 1354633 - When privacy.resistFingerprinting = true, MediaError.message can only get whitelisted messages

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164856/diff/1-2/

Comment 28

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8893752 [details]
Bug 1354633 - When privacy.resistFingerprinting = true, MediaError.message can only get whitelisted messages

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164856/diff/2-3/

Comment 29

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8899337 [details]
Bug 1354633 - Add test cases

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170554/diff/1-2/

Comment 30

[Arthur Edelstein [:arthur]]

Comment on attachment 8899337 [details]
Bug 1354633 - Add test cases

https://reviewboard.mozilla.org/r/170554/#review177130

Looks good to me. One nit is it might look a little cleaner if you use await/async instead of .then(...)

Comment 31

[Arthur Edelstein [:arthur]]

Comment on attachment 8893752 [details]
Bug 1354633 - When privacy.resistFingerprinting = true, MediaError.message can only get whitelisted messages

https://reviewboard.mozilla.org/r/164856/#review177134

r+, pending a completed whitelist.

Thanks, Chung-Sheng!

Comment 32

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8893752 [details]
Bug 1354633 - When privacy.resistFingerprinting = true, MediaError.message can only get whitelisted messages

https://reviewboard.mozilla.org/r/164856/#review178376

Comment 33

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8899337 [details]
Bug 1354633 - Add test cases

https://reviewboard.mozilla.org/r/170554/#review178380

I fail to understand the purpose of this test case. There should be SimpleTest.ok() calls to assert whether the test is success or failure.

::: browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html:13
(Diff revision 2)
> +
> +let testPromise = (resistFingerprinting, src, whitelist) => new Promise(resolve => {
> +  let video = document.createElement("video");
> +  video.src = src;
> +  video.controls = "true";
> +  video.onload = () => {

Remove this block since a media element will never fire 'load' events.

Comment 34

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8893752 [details]
Bug 1354633 - When privacy.resistFingerprinting = true, MediaError.message can only get whitelisted messages

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164856/diff/3-4/

Comment 35

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8899337 [details]
Bug 1354633 - Add test cases

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170554/diff/2-3/

Comment 36

[Chung-Sheng Fu [:cfu]]

As we talked, the test checks if content script can get whitelisted error messages, and if non-whitelisted error messages are blank, when privacy.resistFingerprinting = true.

I also followed your instruction, adding a check that when privacy.resistFingerprinting = false, the error messages should not be blank.

Could you please take a look again?  Thanks a lot!

Comment 37

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8899337 [details]
Bug 1354633 - Add test cases

https://reviewboard.mozilla.org/r/170554/#review178452

::: browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html:48
(Diff revision 3)
> +    false // whitelist
> +  );
> +};
> +
> +SimpleTest.waitForExplicitFinish();
> +document.addEventListener("DOMContentLoaded", async () => {

'async' is not needed for this event handler.

Comment 38

[Chung-Sheng Fu [:cfu]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #37)
> Comment on attachment 8899337 [details]
> Bug 1354633 - Add test cases
> 
> https://reviewboard.mozilla.org/r/170554/#review178452
> 
> :::
> browser/components/resistfingerprinting/test/mochitest/
> test_bug1354633_media_error.html:48
> (Diff revision 3)
> > +    false // whitelist
> > +  );
> > +};
> > +
> > +SimpleTest.waitForExplicitFinish();
> > +document.addEventListener("DOMContentLoaded", async () => {
> 
> 'async' is not needed for this event handler.

It seems to me that we still need to declare the handler async because we have to await for testBody inside it.

Comment 39

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8893752 [details]
Bug 1354633 - When privacy.resistFingerprinting = true, MediaError.message can only get whitelisted messages

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164856/diff/4-5/

Comment 40

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8899337 [details]
Bug 1354633 - Add test cases

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170554/diff/3-4/

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f7d662de266
When privacy.resistFingerprinting = true, MediaError.message can only get whitelisted messages r=arthuredelstein,jwwang
https://hg.mozilla.org/integration/autoland/rev/84021c7d6b39
Add test cases r=arthuredelstein,jwwang

Comment 42

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failure: unnecessary semicolon at browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html:45:

https://hg.mozilla.org/integration/autoland/rev/10c3e8bf658fc41744760c5c48c8610812557644
https://hg.mozilla.org/integration/autoland/rev/b3ac3b6c7775b1403247dd97123b476007ac3519

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=84021c7d6b3924cfc907c75bdf3608c3e2980a04&filter-resultStatus=testfailed&filter-resultStatus=runnable&filter-resultStatus=busted&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=127433834&repo=autoland
> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/resistfingerprinting/test/mochitest/test_bug1354633_media_error.html:45:2 | Unnecessary semicolon. (no-extra-semi)

Comment 43

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8899337 [details]
Bug 1354633 - Add test cases

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170554/diff/4-5/

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/120bb4c9f8aa
When privacy.resistFingerprinting = true, MediaError.message can only get whitelisted messages r=arthuredelstein,jwwang
https://hg.mozilla.org/integration/autoland/rev/bdd7fa60afcf
Add test cases r=arthuredelstein,jwwang

Comment 45

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/120bb4c9f8aa
https://hg.mozilla.org/mozilla-central/rev/bdd7fa60afcf