Closed Bug 1330326 Opened 7 years ago Closed 7 years ago

Make sandboxing policy more configurable via preferences

Categories

(Core :: Security: Process Sandboxing, defect)

Product:
Core
Component:
Security: Process Sandboxing
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: gcp, Assigned: gcp)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: sblc2)

Attachments

(2 files, 1 obsolete file)

Bug 1330326 - Add Split() function on String classes.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1330326 - Make sandboxing policy more configurable via preferences.
59 bytes, text/x-review-board-request
jld
: review+
Details 
      No description provided.
Assignee: nobody → gpascutto
Whiteboard: sblc2
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-
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 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+
(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.
(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.
Attachment #8827681 - Attachment is obsolete: true
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 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+
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
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)
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)
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
https://hg.mozilla.org/mozilla-central/rev/e7768fa69da5
https://hg.mozilla.org/mozilla-central/rev/50ff055b70fe
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1335323
Blocks: 1337162
Depends on: 1339112
(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...
Depends on: 1346099
Depends on: 1346100
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: