Closed
Bug 1399392
Opened 7 years ago
Closed 7 years ago
Don't hardcode .config etc, use XDG_* environment vars.
Categories
(Core :: Security: Process Sandboxing, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: gcp, Assigned: gcp)
References
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.
Assignee | ||
Updated•7 years ago
|
Whiteboard: sb?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years 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+
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•7 years ago
|
Assignee: nobody → gpascutto
Priority: -- → P1
Whiteboard: sb? → sb+
Comment 5•7 years 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•7 years 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 :-(
Comment 7•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•