Closed Bug 1354633 Opened 2 years ago Closed 2 years ago

blank MediaError.message when resisting fingerprinting

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: simon.mainey, Assigned: cfu)

References

(Blocks 1 open bug)

Details

(Keywords: privacy, Whiteboard: [tor 21792][fingerprinting][fp:m3])

Attachments

(2 files, 2 obsolete files)

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?
Blocks: 1322606
Component: Untriaged → Audio/Video: Playback
Keywords: privacy
Product: Firefox → Core
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.
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
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
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
Summary: reinstate dom.MediaError.message.enabled → blank MediaError.message when resisting fingerprinting
Whiteboard: [tor 21792]
Version: 52 Branch → unspecified
Priority: -- → P2
Whiteboard: [tor 21792] → [tor 21792][fingerprinting][fp:m3]
Attachment #8893752 - Flags: review?(jwwang)
Attachment #8893752 - Flags: review?(arthuredelstein)
Will there be any concern about blanking MediaError.message?
If this patch is fine, I will add some tests.
Assignee: nobody → cfu
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.
Attachment #8893752 - Flags: review?(jwwang) → review-
(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 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.
Attachment #8893752 - Flags: review?(arthuredelstein)
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.
(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?
Flags: needinfo?(jwwang)
(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.
Flags: needinfo?(jwwang)
Maybe we could consider showing something like "Error message is blocked due to fingerprinting" when the pref of Fingerprinting resistance is on.
(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?
Flags: needinfo?(jwwang)
I prefer the whitelist approach. Check https://bugzilla.mozilla.org/show_bug.cgi?id=1376004#c10 for the format.
Flags: needinfo?(jwwang)
(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.
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.
Flags: needinfo?(jwwang)
Flags: needinfo?(arthuredelstein)
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.
(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.
Flags: needinfo?(arthuredelstein)
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.
(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.
Flags: needinfo?(jwwang)
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.
Flags: needinfo?(jwwang)
Flags: needinfo?(arthuredelstein)
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.".
Attachment #8897709 - Flags: review+
(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.
Flags: needinfo?(arthuredelstein)
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?
Flags: needinfo?(arthuredelstein)
(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.
Flags: needinfo?(arthuredelstein)
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?
Attachment #8893752 - Flags: review?(jwwang)
Attachment #8893752 - Flags: review?(arthuredelstein)
Attachment #8899337 - Flags: review?(jwwang)
Attachment #8899337 - Flags: review?(arthuredelstein)
Attachment #8897709 - Attachment is obsolete: true
Attachment #8898233 - Attachment is obsolete: true
Attachment #8893752 - Flags: review?(jwwang)
Attachment #8893752 - Flags: review?(arthuredelstein)
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(...)
Attachment #8899337 - Flags: review?(arthuredelstein) → review+
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!
Attachment #8893752 - Flags: review?(arthuredelstein) → review+
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
Attachment #8893752 - Flags: review?(jwwang) → review+
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.
Flags: needinfo?(jwwang)
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!
Flags: needinfo?(jwwang)
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.
Attachment #8899337 - Flags: review?(jwwang) → review+
Flags: needinfo?(jwwang)
(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.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
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)
Flags: needinfo?(cfu)
Flags: needinfo?(cfu)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/120bb4c9f8aa
https://hg.mozilla.org/mozilla-central/rev/bdd7fa60afcf
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.