Closed
Bug 1330326
Opened 8 years ago
Closed 8 years ago
Make sandboxing policy more configurable via preferences
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: gcp, Assigned: gcp)
References
(Depends on 1 open bug)
Details
(Keywords: dev-doc-needed, Whiteboard: sblc2)
Attachments
(2 files, 1 obsolete file)
No description provided.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gpascutto
Updated•8 years ago
|
Whiteboard: sblc2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•8 years ago
|
||
mozreview-review |
Comment on attachment 8827681 [details]
Bug 1330326 - Add Split() function on String classes.
https://reviewboard.mozilla.org/r/105306/#review106334
A few little things below. This also needs tests in `xpcom/tests/gtest/TestStrings.cpp` before it can land.
Thanks for tackling this!
::: dom/ipc/ContentChild.cpp:1505
(Diff revision 1)
> Preferences::GetCString("security.sandbox.content.syscall_whitelist");
> if (extraSyscalls) {
> - auto callNumbers = StringSplit(extraSyscalls);
> + for (const nsCSubstring& callNrString : extraSyscalls.Split(',')) {
> - for (const nsCString& callNrString : callNumbers) {
> nsresult rv;
> - int callNr = callNrString.ToInteger(&rv);
> + int callNr = PromiseFlatCString(callNrString).ToInteger(&rv);
`ToInteger` being on the wrong string type makes me sad; we shouldn't need to allocate a copy here.
::: xpcom/string/nsTSubstring.h:928
(Diff revision 1)
> protected:
>
> friend class nsTObsoleteAStringThunk_CharT;
> friend class nsTSubstringTuple_CharT;
> -
> - // XXX GCC 3.4 needs this :-(
> + friend class nsTSubstringSplitter_CharT;
> + // XXX GCC 3.4 till 5.4 need this :-(
Please remove this comment, as discussed on IRC.
::: xpcom/string/nsTSubstring.h:1221
(Diff revision 1)
> + nsTSubstring_CharT::size_type mPos;
> + };
> +
> +private:
> + const nsTSubstring_CharT* mStr;
> + nsTSubstring_CharT* mArray;
Please use `mozilla::UniquePtr<nsTSubstring_CharT[]> mArray` for this.
I'm pretty sure you can write this without any dynamic memory allocation, but we can leave that for a followup bug.
::: xpcom/string/nsTSubstring.h:1271
(Diff revision 1)
> + return nsTSubstringSplit_Iter(*this, mArraySize);
> + }
> +
> + const nsTSubstring_CharT& Get(const nsTSubstring_CharT::size_type index) const
> + {
> + NS_ASSERTION(index < mArraySize, "Split index out of bounds");
`MOZ_ASSERT`, please.
Attachment #8827681 -
Flags: review?(nfroyd) → review-
Comment 5•8 years ago
|
||
Something else I was thinking of today: I think we want nsDependent*Subtring as the iterator element type, as that won't cause allocations unless the caller asks for it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8827681 [details]
Bug 1330326 - Add Split() function on String classes.
https://reviewboard.mozilla.org/r/105306/#review107638
I think this looks reasonable. I have comments, but nothing that should be blocking, assuming the answers to questions below are all satisfactory.
::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:203
(Diff revision 2)
> Preferences::GetCString("security.sandbox.content.write_path_whitelist");
> -
> if (extraPathString) {
> - auto extraPaths = StringSplit(extraPathString);
> - for (const nsCString& path : extraPaths) {
> - policy->AddDynamic(rdwr, path.get());
> + for (const nsCSubstring& path : extraPathString.Split(',')) {
> + nsCString stripPath(path);
> + stripPath.StripWhitespace();
Uh. Why is this necessary now?
::: xpcom/string/nsTSubstring.h:1175
(Diff revision 2)
> const nsTSubstring_CharT::base_string_type& aRhs)
> {
> return Compare(aLhs, aRhs) > 0;
> }
> +
> +class nsTSubstringSplitter_CharT
We should have a comment here: "You should not need to instantiate this class directly. Use nsTSubstring::Split instead."
::: xpcom/string/nsTSubstring.h:1225
(Diff revision 2)
> + return;
> + }
> +
> + nsTSubstring_CharT::size_type delimCount = mStr->CountChar(aDelim);
> + mArraySize = delimCount + 1;
> + mArray.reset(new nsTSubstring_CharT[mArraySize]);
Does `mArray = mozilla::MakeUnique<nsTSubstring_CharT[]>(mArraySize);` not work? Or was it just too lengthy for your taste?
::: xpcom/string/nsTSubstring.h:1231
(Diff revision 2)
> + int32_t offset = mStr->FindChar(aDelim, start);
> + if (offset != -1) {
Maybe:
```
MOZ_ASSERT(seenParts < mArraySize);
```
would be appropriate here prior to testing `offset`.
::: xpcom/tests/gtest/TestStrings.cpp:983
(Diff revision 2)
> + nsCString one("one"),
> + two("one;two"),
> + three("one--three");
We should check the following cases, just for completeness:
* an empty string;
* delimeter at the beginning;
* delimiter at the end.
Attachment #8827681 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #9)
> ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:203
> (Diff revision 2)
> > Preferences::GetCString("security.sandbox.content.write_path_whitelist");
> > -
> > if (extraPathString) {
> > - auto extraPaths = StringSplit(extraPathString);
> > - for (const nsCString& path : extraPaths) {
> > - policy->AddDynamic(rdwr, path.get());
> > + for (const nsCSubstring& path : extraPathString.Split(',')) {
> > + nsCString stripPath(path);
> > + stripPath.StripWhitespace();
>
> Uh. Why is this necessary now?
I was thinking that if the path pref looks like "/usr/local, /usr/share" we'd want to get rid of the extra space. But StripWhitespace() does too much, right. I want "TrimWhitespace()" or equivalent.
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #9)
> Does `mArray = mozilla::MakeUnique<nsTSubstring_CharT[]>(mArraySize);` not
> work? Or was it just too lengthy for your taste?
There's an added friend declaration in this patch because the default constructor of nsTSubstring is protected. With the proposed change, some incantation of mozilla::MakeUnique/mozilla::UniquePtr needs to be friended as well (I'd tell you which if I could decode the compiler error message). I don't think it is a net gain in clarity.
Assignee | ||
Comment 12•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8827681 -
Attachment is obsolete: true
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8827679 [details]
Bug 1330326 - Add Split() function on String classes.
https://reviewboard.mozilla.org/r/105302/#review107838
Thank you!
Attachment #8827679 -
Flags: review?(nfroyd) → review+
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8827680 [details]
Bug 1330326 - Make sandboxing policy more configurable via preferences.
https://reviewboard.mozilla.org/r/105304/#review108348
Mostly looks good, with some nits about passing down the syscall vector.
(And, for the record, I can't think of a better way to get the pref data into libmozsandbox, given that it's used exactly once and only on one code path. Using a preference observer would work, but in this case it would just be extra code and more complicated security-sensitive data flow for no real benefit, in my opinion.)
::: security/sandbox/linux/SandboxFilter.h:26
(Diff revision 3)
>
> #ifdef MOZ_CONTENT_SANDBOX
> class SandboxBrokerClient;
>
> -UniquePtr<sandbox::bpf_dsl::Policy> GetContentSandboxPolicy(SandboxBrokerClient* aMaybeBroker);
> +UniquePtr<sandbox::bpf_dsl::Policy> GetContentSandboxPolicy(SandboxBrokerClient* aMaybeBroker,
> + std::vector<int>& aSyscallWhitelist);
Couldn't this be a `const vector<int>&` or `vector<int>&&`?
::: security/sandbox/linux/SandboxFilter.cpp:505
(Diff revision 3)
> }
>
> public:
> explicit ContentSandboxPolicy(SandboxBrokerClient* aBroker):mBroker(aBroker) { }
> virtual ~ContentSandboxPolicy() { }
> + void WhiteListSyscall(int aSyscall) { mSyscallWhitelist.push_back(aSyscall); }
Couldn't you just pass the vector to the constructor instead of iterating over it and using this method?
::: security/sandbox/linux/SandboxFilter.cpp:852
(Diff revision 3)
>
> UniquePtr<sandbox::bpf_dsl::Policy>
> -GetContentSandboxPolicy(SandboxBrokerClient* aMaybeBroker)
> +GetContentSandboxPolicy(SandboxBrokerClient* aMaybeBroker,
> + std::vector<int>& aSyscallWhitelist)
> {
> - return UniquePtr<sandbox::bpf_dsl::Policy>(new ContentSandboxPolicy(aMaybeBroker));
> + auto policyPtr = new ContentSandboxPolicy(aMaybeBroker);
I'd suggest `MakeUnique<ContentSandboxPolicy>(...)` and returning it with `Move`; or, if the WhitelistSyscall loop goes away, `return MakeUnique<ContentSandboxPolicy>(...)` should work. `UniquePtr` has an implicit constructor that takes a `UniquePtr&&` for a different but compatible type, which I didn't realize when I originally wrote this, and that should take care of the upcasting.
Attachment #8827680 -
Flags: review?(jld) → review+
Comment hidden (mozreview-request) |
Comment 18•8 years ago
|
||
Pushed by gpascutto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/558774589f3e
Add Split() function on String classes. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/e87ae43ca443
Make sandboxing policy more configurable via preferences. r=jld
Comment 19•8 years ago
|
||
Backed out for Windows build bustage:
https://hg.mozilla.org/integration/autoland/rev/0766f63202b5fd05043163f0f47ba7d735d61e99
https://hg.mozilla.org/integration/autoland/rev/2633df8bf5d3969230f0627eda9c01e239f1091d
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e87ae43ca44332a0bf30a4151b57cbb9b8e369ac
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=72644575&repo=autoland
> 11:46:33 INFO - c:\builds\moz2_slave\autoland-w32-00000000000000000\build\src\obj-firefox\dist\include\nsTSubstring.h(1234): error C2220: warning treated as error - no 'object' file generated
Flags: needinfo?(gpascutto)
Assignee | ||
Comment 20•8 years ago
|
||
Ugh right, the Sandboxing changes don't get compiled on Windows at all, the the change to the string header seems to generate a warning with MSVC.
Flags: needinfo?(gpascutto)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•8 years ago
|
||
Pushed by gpascutto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e7768fa69da5
Add Split() function on String classes. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/50ff055b70fe
Make sandboxing policy more configurable via preferences. r=jld
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e7768fa69da5
https://hg.mozilla.org/mozilla-central/rev/50ff055b70fe
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 25•8 years ago
|
||
The new Split function should be added to this doc: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings
Keywords: dev-doc-needed
Assignee | ||
Comment 26•8 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #25)
> The new Split function should be added to this doc:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/
> Internal_strings
See the comment directly above yours...
You need to log in
before you can comment on or make changes to this bug.
Description
•