bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Don't hardcode .config etc, use XDG_* environment vars.

RESOLVED FIXED in Firefox 57

Status

()

Core
Security: Process Sandboxing
P1
enhancement
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: gcp, Assigned: gcp)

Tracking

Trunk
mozilla57
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: sb+)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

10 months ago
https://bugzilla.mozilla.org/show_bug.cgi?id=1396542#c50

We should check the FreeDesktop environment vars instead of whitelisting those paths.
(Assignee)

Updated

10 months ago
Whiteboard: sb?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

10 months ago
mozreview-review
Comment on attachment 8907606 [details]
Bug 1399392 - Don't hardcode .config, use XDG_* environment vars.

https://reviewboard.mozilla.org/r/179292/#review184748
Attachment #8907606 - Flags: review?(jld) → review+

Comment 4

10 months ago
Pushed by gpascutto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5dc76a14828
Don't hardcode .config, use XDG_* environment vars. r=jld
(Assignee)

Updated

10 months ago
Assignee: nobody → gpascutto
Priority: -- → P1
Whiteboard: sb? → sb+

Comment 5

10 months ago
mozreview-review
Comment on attachment 8907606 [details]
Bug 1399392 - Don't hardcode .config, use XDG_* environment vars.

https://reviewboard.mozilla.org/r/179292/#review185060

::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:140
(Diff revision 2)
> +    policy->AddDir(rdonly, xdgConfigPath);
> +  }
> +
> +  nsAutoCString xdgConfigDirs(PR_GetEnv("XDG_CONFIG_DIRS"));
> +  for (const auto& path : xdgConfigDirs.Split(':')) {
> +    policy->AddDir(rdonly, PromiseFlatCString(path).get());

It's not a bug, but this one is a little odd because it will treat an unset variable the same as an empty one.  `AddDir(_, "")` won't do anything because `stat("", _)` will fail with `ENOENT`, and we'd wind up in that case anyway if one of these env vars wound up set to the empty string somehow, but it's something to keep in mind if we ever change the policy object's `Add*` methods.
(Assignee)

Comment 6

10 months ago
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #5)
> > +  nsAutoCString xdgConfigDirs(PR_GetEnv("XDG_CONFIG_DIRS"));
> > +  for (const auto& path : xdgConfigDirs.Split(':')) {
> > +    policy->AddDir(rdonly, PromiseFlatCString(path).get());
> 
> It's not a bug, but this one is a little odd because it will treat an unset
> variable the same as an empty one.  `AddDir(_, "")` won't do anything
> because `stat("", _)` will fail with `ENOENT`, and we'd wind up in that case
> anyway if one of these env vars wound up set to the empty string somehow,
> but it's something to keep in mind if we ever change the policy object's
> `Add*` methods.

We have defense in depth.

AddDir explicitly checks for and ignores empty strings:
https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/security/sandbox/linux/broker/SandboxBroker.cpp#217

nsTSubStringSplitter handles the case where it's given an empty string:
http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/xpcom/string/nsTSubstring.cpp#1230

I wrote both checks! Do I get a cookie? I guess Bug 1339112 says I don't :-(
https://hg.mozilla.org/mozilla-central/rev/d5dc76a14828
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.