Closed Bug 1399392 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Security: Process Sandboxing, enhancement, P1)

x86_64
Linux
enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: gcp, Assigned: gcp)

Details

(Whiteboard: sb+)

Attachments

(1 file)

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

We should check the FreeDesktop environment vars instead of whitelisting those paths.
Whiteboard: sb?
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+
Pushed by gpascutto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5dc76a14828
Don't hardcode .config, use XDG_* environment vars. r=jld
Assignee: nobody → gpascutto
Priority: -- → P1
Whiteboard: sb? → sb+
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.
(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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.