Closed Bug 1369327 Opened 7 years ago Closed 7 years ago

Making reader view users uniform when 'privacy.resistFingerprinting' is true

Categories

(Toolkit :: Reader Mode, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: timhuang, Assigned: jhao)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fingerprinting][tor][fp:m2])

Attachments

(1 file, 1 obsolete file)

For fingerprinting resistance, we'd like to make users of Reader View all look like the same when 'privacy.resistFingerprinting' is true.

We want to provide the same behavior as 'reader.parse-on-load.enabled' = false. 

If having 'reader.parse-on-load.enabled' set to true would discriminate between users with low memory computers (probably only some mobile ones) and those who have Reader View capable ones.

See [1] for further information

[1] https://trac.torproject.org/projects/tor/ticket/18950
Priority: -- → P2
Priority: P2 → P1
Assignee: nobody → jhao
Attachment #8879845 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8879845 [details]
Bug 1369327 - Making reader view users uniform when 'privacy.resistFingerprinting' is true

https://reviewboard.mozilla.org/r/151252/#review156086

(In reply to Tim Huang[:timhuang] from comment #0)
> For fingerprinting resistance, we'd like to make users of Reader View all
> look like the same when 'privacy.resistFingerprinting' is true.

parse-on-load.enabled defaults to true and has no user-visible toggle (besides about:config, which lets you change *any* preference).

Why is this necessary? Conversely, why do we not need a fingerprinting override for every single pref we ship, which clearly doesn't scale?

> We want to provide the same behavior as 'reader.parse-on-load.enabled' =
> false.

Defaulting to off seems like a bad idea. Why do we need to cripple the browser when people enable fingerprinting resistance? No concrete threat model enabled by reader mode seems to be present either in this bug or in the tor ticket. It's not even clear how you think an adversary would check whether reader mode was available or turned on/off. As noted on the tor ticket, about:reader is governed by security restrictions, and those are the same irrespective of whether parse-on-load enabled is turned on or off (and irrespective of what `_getStateForParseOnLoad` returns).

> If having 'reader.parse-on-load.enabled' set to true would discriminate
> between users with low memory computers (probably only some mobile ones) and
> those who have Reader View capable ones.

Not "probably", literally only android - see https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsMemoryImpl.cpp#37-65

And in fact, that threshold is defined as 384mb, which is currently listed as the minimum system requirement anyway ( https://www.mozilla.org/en-US/firefox/android/52.0/system-requirements/ ). I don't know if this is enforced, but if anything, it sounds like we should just rip this code out (but you should check with a mobile peer  like :sebastian).


So, r-, because:
1) outright removing features when the user toggles fingerprinting resistance is questionable
2) it's not clear there's any fingerprinting risk in the sense that the feature is enabled for everyone anyway
3) it's not clear that even if the feature isn't enabled for everyone, an adversary could detect whether or not it is enabled in some way.
Attachment #8879845 - Flags: review?(gijskruitbosch+bugs) → review-
Status: NEW → ASSIGNED
Hi Georg,

Is there a way that the content can know whether the reader mode is on or off?
Flags: needinfo?(gk)
We discussed this today. The original fingerprinting concern was that onload parsing may or may not occur based on user hardware. But if content cannot detect whether onload parsing occured, we may not need to care about this at all!
(In reply to Tom Ritter [:tjr] from comment #4)
> We discussed this today. The original fingerprinting concern was that onload
> parsing may or may not occur based on user hardware. But if content cannot
> detect whether onload parsing occured, we may not need to care about this at
> all!

Yes. It seems comment 2 is indicating that this is indeed a non-issue. At least for desktop builds. Now, if we could rip out the code that discriminates users based on available memory that would be a good thing. I'd be happy to see that solution to the problem instead of disabling this feature fwiw.
Flags: needinfo?(gk)
Sebastian, can you comment on whether the low memory check here is still useful? Do we enforce the minimum system requirements on android, and are all devices we run on guaranteed to have enough RAM anyway?
Flags: needinfo?(s.kaspari)
Good question. We removed the mirrored code in Java some time ago because it was unused (bug 1256922). I'm okay with removing it here too. But I'll flag snorp additionally.
Flags: needinfo?(s.kaspari) → needinfo?(snorp)
Seems fine to me
Flags: needinfo?(snorp)
Attachment #8879845 - Attachment is obsolete: true
Thanks to Georg, Gijs, Sebastian and snorp.

The above patch removes the low memory check in reader mode.
Comment on attachment 8887347 [details]
Bug 1369327 - Remove low memory platform checks in reader mode.

https://reviewboard.mozilla.org/r/158176/#review163396

Might be worth a followup to remove the infrastructure on the XPCOM side here.
Attachment #8887347 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by jhao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c74072efc06
Remove low memory platform checks in reader mode. r=Gijs
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.
https://hg.mozilla.org/mozilla-central/rev/0c74072efc06
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: