Closed Bug 1848615 Opened 6 months ago Closed 6 months ago

SetDefaultTimeZoneFromHostTimeZone uses wrong timezone in content process

Categories

(Core :: Security: Process Sandboxing, defect, P2)

Desktop
Linux
defect

Tracking

()

RESOLVED FIXED
119 Branch
Tracking Status
relnote-firefox --- 117+
firefox-esr102 --- unaffected
firefox-esr115 --- fixed
firefox116 --- wontfix
firefox117 --- fixed
firefox118 --- fixed
firefox119 --- fixed

People

(Reporter: tschuster, Assigned: jld)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

I noticed that new Intl.DateTimeFormat().resolvedOptions().timeZone returns Europe/Oslo on websites such as jsconsole.com, instead of (correctly) Europe/Berlin like in the browser console.

This seems to be caused by icu::TimeZone::detectHostTimeZone returning a different timezone in content processes.

In the parent process detectHostTimeZone() returns Europe/Berlin, but in the content process it's Arctic/Longyearbyen, which is later canonicalized to Europe/Oslo by tryCanonicalizeTimeZoneConsistentWithIANA.

Does it work correctly when the content sandbox is disabled via MOZ_DISABLE_CONTENT_SANDBOX=1? Bug 1839287 mentions that the content sandbox can cause issues when trying to resolve the host time zone.

Thanks. I does seem like disabling sandboxing makes it work.

See Also: → 1839287
Component: JavaScript: Internationalization API → Security: Process Sandboxing
OS: Unspecified → Linux
Hardware: Unspecified → Desktop
Attached file MOZ_SANDBOX_LOGGING
Assignee: nobody → tschuster
Status: NEW → ASSIGNED

For reference I am running "Linux Mint 21.2 Cinnamon", and not nix.

/usr/share and everything below it are already allowed in the sandbox, so just adding that shouldn't change anything. The log shows:

[89518] Sandbox: Failed errno -22 op readlink flags 00 path /etc
[89248] Sandbox: Recording mapping /usr/share/zoneinfo/Europe/Berlin -> /etc/localtime
[89248] Sandbox: SandboxBroker: denied op=readlink rflags=0 perms=0 path=/usr for pid=89518

There's something about the way that the links are laid out here that our code considers insecure, but it's not so obvious what. Unlike the Nix example in the other bug, this is the same setup as there currently is on Ubuntu LTS, so all files are in normal paths (no /nix/store).

Assignee: tschuster → jld
Severity: -- → S3
Priority: -- → P2
Attachment #9348833 - Attachment is obsolete: true

Just speculating, but maybe we need to add "/usr" considering the path=/usr in the error? Edit: Yeah that works. Not sure if there is anything important in there though.

Yes, it looks like we could do AddPath("/usr", MAY_READ), and that should work. It would also allow listing the directory, but that should be relatively low-impact.

Slightly outside the scope of this bug, but maybe readlink ought to require only MAY_ACCESS and not MAY_READ, in light of how Linux itself doesn't recognize symlink permissions (i.e., in the absence of our sandboxing, if you can lstat a path then you can also readlink it), so that we don't have to give out readdir access when we really just want realpath to work. However, that change could have a lot of side effects, so I'd prefer not to block this bug on that.

For reference, here's an example of what realpath is doing:

$ strace -fereadlink,readlinkat realpath /etc/localtime
readlink("/etc", 0x7ffe89c6da20, 1023)  = -1 EINVAL (Invalid argument)
readlink("/etc/localtime", "/usr/share/zoneinfo/US/Pacific", 1023) = 30
readlink("/usr", 0x7ffe89c6da20, 1023)  = -1 EINVAL (Invalid argument)
readlink("/usr/share", 0x7ffe89c6da20, 1023) = -1 EINVAL (Invalid argument)
readlink("/usr/share/zoneinfo", 0x7ffe89c6da20, 1023) = -1 EINVAL (Invalid argument)
readlink("/usr/share/zoneinfo/US", 0x7ffe89c6da20, 1023) = -1 EINVAL (Invalid argument)
readlink("/usr/share/zoneinfo/US/Pacific", "../America/Los_Angeles", 1023) = 22
readlink("/usr/share/zoneinfo/America", 0x7ffe89c6da20, 1023) = -1 EINVAL (Invalid argument)
readlink("/usr/share/zoneinfo/America/Los_Angeles", 0x7ffe89c6da20, 1023) = -1 EINVAL (Invalid argument)
/usr/share/zoneinfo/America/Los_Angeles
+++ exited with 0 +++

On NixOS (see bug 1839287), it looks like it's /etc/localtime/etc/zoneinfo/<timezone>, then /etc/zoneinfo/etc/static/zoneinfo, /etc/static/nix/store/<hash>-etc/etc, then /nix/store/<hash>-etc/etc/zoneinfo/nix/store/<hash>-tzdata-<version>/share/zoneinfo. So, we'd also want to add /nix similary to /usr. (We already allow read access to /etc and anything under it.)

We have the affected ICU version; marking bug 1824744 as the regressor so uplifts can be tracked.

Keywords: regression
Regressed by: 1824744

Set release status flags based on info from the regressing bug 1824744

ICU version 73 (bug 1824744) has a change to call realpath
rather than just readlink on /etc/localtime, meaning that it needs to
be able to readlink every directory involved in path resolution. In
particular, for a symlink into /usr/share/zoneinfo, this includes
/usr, which is blocked by the content sandbox policy.

Currently, the file broker requires MAY_READ permission to allow
readlink, so we grant that on /usr and /nix (there will be a
similar issue with symlinks to /nix/store/...). Note that this
applies only to those directories themselves, not files within them.

This also means that the process can open those directories for reading
(i.e., readdir), but that should be relatively low-impact compared to
the information that's already exposed.

Duplicate of this bug: 1839287

However, that change could have a lot of side effects, so I'd prefer not to block this bug on that.

Fair enough. I had similar observations about MAY_ACCESS vs MAY_READ in the patch review, though.

Set release status flags based on info from the regressing bug 1824744

Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab673eef69bf
Adjust Linux content sandbox policy so that ICU can get the canonical time zone. r=gcp
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 119 Branch

The patch landed in nightly and beta is affected.
:jld, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox118 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(jld)

Comment on attachment 9349651 [details]
Bug 1848615 - Adjust Linux content sandbox policy so that ICU can get the canonical time zone.

Beta/Release Uplift Approval Request

  • User impact if declined: Incorrect information from JS internationalization APIs (depending on the user's locale / time zone settings) on Linux; see the initial bug report for details.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: I don't need manual testing (the automated test I added should be enough), but if it matters, I was able to reproduce the bug by changing my system time zone to Europe/Berlin and evaluating the JS expression in the bug report.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This makes the sandbox policy more permissive, so it's unlikely to cause any functional regressions; and the addition to the sandbox policy is fairly small, so it's unlikely to unexpectedly regress the effectiveness of sandboxing.
  • String changes made/needed: none
  • Is Android affected?: No

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a regression from earlier versions, it's a change in Web API behavior exposed to content, and the risk of uplift should be low.
  • User impact if declined: (See Beta uplift request.)
  • Fix Landed on Version: 119
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): (See beta uplift request.)
Flags: needinfo?(jld)
Attachment #9349651 - Flags: approval-mozilla-esr115?
Attachment #9349651 - Flags: approval-mozilla-beta?

Comment on attachment 9349651 [details]
Bug 1848615 - Adjust Linux content sandbox policy so that ICU can get the canonical time zone.

Approved for 118.0b3

Attachment #9349651 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9349651 - Flags: approval-mozilla-esr115? → approval-mozilla-esr115+
Attachment #9349651 - Flags: approval-mozilla-release?

Comment on attachment 9349651 [details]
Bug 1848615 - Adjust Linux content sandbox policy so that ICU can get the canonical time zone.

Approved for 117.0.1

Attachment #9349651 - Flags: approval-mozilla-release? → approval-mozilla-release+

Added to the 117.0.1 release notes:

Fixed an issue causing incorrect time zones to be detected on some sites

Duplicate of this bug: 1852355
Duplicate of this bug: 1843421
You need to log in before you can comment on or make changes to this bug.