Opened 14 months ago

Closed 13 months ago

Last modified 12 months ago

#31730 closed defect (fixed)

Revert aarch64 fixup for ESR 60-based bundles with Tor Browser 9

Reported by: gk Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, tbb-parity, TorBrowserTeam201910R tbb-9.0-must
Cc: sysrqb Actual Points: 0.5
Parent ID: Points: 0.5
Reviewer: Sponsor:


We solved #31616 and #31140 by making sure the JIT prefs are not enabled via our security slider on aarch64.

We need to revert that for Tor Browser 9 and make sure that the security level is actually reset to low for those users whose level got set to custom due to the workaround we deployed. (Doing the latter might actually not be trivial).

Child Tickets

Change History (12)

comment:1 Changed 14 months ago by cypherpunks

The security level is set automatically both back and forth.

comment:2 Changed 13 months ago by gk

Cc: sysrqb added
Points: 0.5

comment:3 Changed 13 months ago by acat

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: newneeds_review

comment:4 Changed 13 months ago by acat

Actually, I think ​ is a bit more precise. This will preserve the user custom state if they did not change any of the javascript.options.* prefs (but some of the others).

comment:5 Changed 13 months ago by sysrqb

Keywords: tbb-mobile, tbb-parity, TorBrowserTeam201910R, tbb-9.0-musttbb-mobile, tbb-parity, TorBrowserTeam201910R tbb-9.0-must
Status: needs_reviewneeds_revision

If I was smart, I would've saved the current prefs somehow before we overwrote them, but I didn't think about that. Unfortunately, I'm worried about changing these. On the one hand, custom prefs aren't recommended but they are allowed. If we change this without notifying the user then we could be putting them at risk. We should probably include a warning about this in the release announcement, at a minimum.

I think we should set kCustomPref as false if all the prefs are set as level 4. What do you think? Can you add a function isSecurityLevel(index) that takes an index and iterates through kSecuritySettings, returns true if they match the expected value and false otherwise? Or something like this?

comment:6 Changed 13 months ago by gk

Keywords: TorBrowserTeam201910 added; TorBrowserTeam201910R removed

comment:7 Changed 13 months ago by acat

In the first version which I didn't push I was doing a similar thing, reusing the function read_setting_from_prefs. I think I switched to just checking the security level because I thought in any case it would be possible to set these prefs to true without user wanting, and that it would not be very likely that a user which did not change the security level slider would have set ion, baselinejit and native_regexp manually to false.

But I think the one you suggest is probably better here, it's on the safer side. It's true that there could be the case that a user kept level 4 and just flipped some of media.webaudio.enabled, mathml.disabled, gfx.font_rendering.opentype_svg.enabled or svg.disabled, in which case with the this fix we would wrongly keep ion, baselinejit or native_regexp to false (with an unnecessary performance hit). But given that no solution is perfect, I think it's better to prioritize security over performance.

For both fixes, there's always the case that for a user who kept security level to 4 and disabled ion, baselinejit or native_regexp we will be wrongly enabling these. So I think in any case having some warning it's good, not sure where. Probably somewhere in release notes and/or blog post is enough.

So here is the revised fix:

I'm also checking that the slider value is 4, to rule out cases where user moved the security slider to < 3 but flipped some prefs in a way that those have the same value as level 4 (very unlikely, but...).

comment:8 Changed 13 months ago by acat

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: needs_revisionneeds_review

comment:9 Changed 13 months ago by acat

Actual Points: 0.5

comment:10 Changed 13 months ago by sysrqb

Status: needs_reviewmerge_ready

Okay, I think this'll work. Let's use it.

comment:11 Changed 13 months ago by gk

Resolution: fixed
Status: merge_readyclosed

This looks good to me, too. Applied to master (commit 9d744c6adc5ee3608e519c5db49528cd6ee6fe54).

comment:12 Changed 12 months ago by sysrqb

It looks like this worked. I tested it on my device.

Note: See TracTickets for help on using tickets.