Closed Bug 1041775 Opened 10 years ago Closed 9 years ago

Update security/sandbox to upstream chromium sandbox code as of 25th July 2014

Categories

(Core :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: TimAbraldes, Assigned: bobowen)

References

Details

Attachments

(3 files, 2 obsolete files)

It includes changes that allow `SetIntegrityLevel` to work with the alternate desktop [1]

[1] bug 1027907 comment 1
Assignee: nobody → bobowencode
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b0821552da2f

I think I'm fairly close on this merge, which takes us to Chromium commit 9522fad406dd161400daa518075828e47bd47f60

This was on 25/7/14 and so after the fix for SetIntegrityLevel / alternate desktop, but before some of the changes that were causing me problems on Linux.

Jed, that try push is with |ac_add_options --enable-content-sandbox| specified on all platforms.
Everything builds, but we get a whole host of problems running tests on Linux and Android ... I assume we'd expect that?
Flags: needinfo?(jld)
I'm going to take a flyer on the answer to that needinfo.

This brings all directories up to Chromium commit 9522fad406dd161400daa518075828e47bd47f60 except security/sandbox/linux.

Some of the files in this directory have been modified to change include paths and linux_seccomp.h has had more significant changes.
There is only a small change to android_ucontext.h that I don't think is relevant to us (for android x86_64).
There were bigger changes to linux_seccomp.h, but it's difficult to tell because of our own changes.

Hopefully we'll be able to remove most of those changes once we move these files to match the Chromium structure.

I've also added a file to hopefully keep track of the commit that each directory (or file) was taken from.
security/sandbox/chromium-commit-status.txt

I'm not sure how these things are normally tracked, so I'm open to better suggestions.

Anyway, most of this is just modifications to our existing Chromium files.

Here are some of my notes about the additions, deletions and shim changes.

* shim logging.cpp - there were various changes needed to this and I've changed it to be a stripped down version of the Chromium logging.cc file to make it easier to merge changes in the future.  It does mean the diff with the current version is a bit of a mess, sorry.  It'll be better in the long run.  One notable thing is that I left the deletes in the LogMessage functions as any callers will presumably be expecting that. 

* shim thread_local_storage.h - added as an empty header in shim because a change for android which included it didn't #if the include (Chromium issue raised).

* shim registry.h - while I was trying to merge to a more recent version registry.h was pulling in new things and I noticed that it is only used in windows_version.cc OSInfo::processor_model_name, which we don't use.  So we could move to a stripped down shim that just exposed the bits that needed to compile.  We didn't stricly need it for this merge, but I thought I might as well make the change now.

* string_split.cc/h - this is now needed for named_pipe_dispatcher.cc

* string_util.cc - this replaces string_util_stripped.cc which was our own version.  string_split needed quite a few of the utility functions and having the original file didn't need anything extra.  I'm guessing they've cleaned up the dependencies a bit here.

* platform_file.h - gone from Chromium base.

* event_trace_provider.cc/h - this was failing to link with "unresolved external symbol _GUID_NULL" and did't seem to be used anywhere in our copies.  Again this was while attempting a more recent merge, but I thought it might as well go anyway.

* sandbox_export.h - moved from sandbox/linux to sandbox.

* Took new files in sandbox/win/src as needed by other files and we may well want to use them anyway.

* Took new files in sandbox/linux/seccomp-bpf as the original patch to add these stated it was taking the whole directory.


I'm going to file a tracker with dependencies (one of which Jed has already filed) to get all the Chromium code into the same structure and move the shim code out.  This should put us in good shape for when we are able to move to a more up to date version.
Attachment #8523849 - Flags: review?(jld)
Attachment #8523849 - Flags: review?(aklotz)
(In reply to Bob Owen (:bobowen) from comment #1)
> Jed, that try push is with |ac_add_options --enable-content-sandbox|
> specified on all platforms.
> Everything builds, but we get a whole host of problems running tests on
> Linux and Android ... I assume we'd expect that?

Yes, that's expected.  (Or, at least, it's not currently expected to work; I don't know if I'd have expected these particular failure modes.)
Flags: needinfo?(jld)
(In reply to Bob Owen (:bobowen) from comment #2)
> * shim logging.cpp - there were various changes needed to this and I've
> changed it to be a stripped down version of the Chromium logging.cc file to
> make it easier to merge changes in the future.  It does mean the diff with
> the current version is a bit of a mess, sorry.  It'll be better in the long
> run.

In particular, it might make the diff for bug 1055227 less messy.
Comment on attachment 8523849 [details] [diff] [review]
Part 1: Update to Chromium sandbox code to commit 9522fad406dd161400daa518075828e47bd47f60. v1

The non-Chromium parts look okay, and I've skimmed the Linux changes in the Chromium diffs.

Also, passes Linux desktop GMP tests on try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b9e936427f4b
Attachment #8523849 - Flags: review?(jld) → review+
Attachment #8523849 - Flags: review?(aklotz) → review+
r=jld - for Linux side of things from comment 5
r=aklotz - for Windows side of things from history item 7

I'd forgotten to qref on the new commit status file:
security/sandbox/chromium-commit-status.txt

I'd also somehow not stripped out the warn only sandbox changes from:
filesystem_interception.cc
registry_interception.cc

which I realised when I tried to re-apply them in the next patch (which I'm about to upload).

This wouldn't have caused any actual problems, but we want any changes we apply to be in their own patch.

Doing this merge has also made me realise why it is so important to keep our changes to an absolute minimum.
Even these few changes add a fair bit of hassle.
Attachment #8523849 - Attachment is obsolete: true
Attachment #8524716 - Flags: review+
Hi Tim,

This is just re-applying the warn only sandbox patches that you reviewed before.
The only ones that were slightly different were for sync_interception.cc, as they've changed to intercept NtCreateEvent and NtOpenEvent from CreateEventW and OpenEventW.

It means I'm going to have to rebase those ones for bug 928044, which are removing and re-adding the same changes for that bug as these are going to land first.
Such is life. :)
Attachment #8524722 - Flags: review?(tabraldes)
... and this is just re-applying the stdout/err workaround for Win XP.
Attachment #8524724 - Flags: review?(tabraldes)
Try push with these patches (this time without |ac_add_options --enable-content-sandbox|):
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bccfa8d02fc2

Chris - don't know if you want to test anything locally with these patches?
This is just an update of the Chromium sandboxing code, it shouldn't change the actual policy at all.
I won't try and land them until tomorrow.
This enables us to fix bug 1027902.
I've tried the patch for that on top of these and it works, so I'll get that uploaded tomorrow.
Flags: needinfo?(cpearce)
I tested these patches and they did not break WMF decoding, so f+ from me.
Flags: needinfo?(cpearce)
Attachment #8524722 - Flags: review?(tabraldes) → review+
Attachment #8524724 - Flags: review?(tabraldes) → review+
Blocks: 1102195
sorry had to back this out since this somehow hit windows 8 PGO Builds like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4075879&repo=mozilla-inbound
Attachment #8524716 - Attachment is obsolete: true
Comment on attachment 8526867 [details] [diff] [review]
Part 1: Update to Chromium sandbox code to commit 9522fad406dd161400daa518075828e47bd47f60. v3

Whoa, that just decided to post half way through adding my attachment.

r=jld - for Linux side of things from comment 5

The interdiff between v3 and v2 seems to be working OK.

Mike - I isolated the PGO bustage to one file and only on Windows x64.

Here's a PGO try push (I realised that I didn't need the debug ones):
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=56fe3e48e5d5

Aaron - while I was making these changes I realised that I hadn't tried this on MSVC2010.
Turns out Chromium had dropped support by this commit, so I've had to go back to a previous version of chromium/base/compiler_specific.h.
This was before they moved from sealed to final.
Blocks: 928044
Comment on attachment 8526867 [details] [diff] [review]
Part 1: Update to Chromium sandbox code to commit 9522fad406dd161400daa518075828e47bd47f60. v3

Review of attachment 8526867 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/sandbox/moz.build
@@ +29,5 @@
>      SOURCES += security_sandbox_cppsrcs
>  
> +    # Bug 1102853 tracks looking at removing this.
> +    if CONFIG['CPU_ARCH'] == 'x86_64':
> +        SOURCES['%s/security/sandbox/win/src/sharedmem_ipc_client.cc' % TOPSRCDIR].no_pgo = True

It's unfortunate that this needs to be duplicated, but meh.
Attachment #8526867 - Flags: review?(mh+mozilla) → review+
Attachment #8526867 - Flags: review?(aklotz) → review+
Summary: Update security/sandbox to latest upstream chromium sandbox code → Update security/sandbox to upstream chromium sandbox code as of 25th July 2014
Comment on attachment 8526867 [details] [diff] [review]
Part 1: Update to Chromium sandbox code to commit 9522fad406dd161400daa518075828e47bd47f60. v3

>+#include <immintrin.h>  // For _xgetbv()
Note that VC2010 doesn't actually provide _xgetbv. (I couldn't find it in MSDN either, so I don't know where it's documented that VC2013 provides it.)
(In reply to neil@parkwaycc.co.uk from comment #18)
> Comment on attachment 8526867 [details] [diff] [review]
> Part 1: Update to Chromium sandbox code to commit
> 9522fad406dd161400daa518075828e47bd47f60. v3
> 
> >+#include <immintrin.h>  // For _xgetbv()
> Note that VC2010 doesn't actually provide _xgetbv. (I couldn't find it in
> MSDN either, so I don't know where it's documented that VC2013 provides it.)

It appears to be included in VS2010 SP1, bug 1105729 is dealing with the Thunderbird fallout from this issue.

I have a possible replacement for VS2010 without SP1.
You need to log in before you can comment on or make changes to this bug.