Update security/sandbox/chromium/ to Chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5.

RESOLVED FIXED in Firefox 59

Status

()

Core
Security: Process Sandboxing
P1
enhancement
RESOLVED FIXED
7 months ago
6 days ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

(Blocks: 1 bug)

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: sb+)

Attachments

(26 attachments)

575.38 KB, patch
aklotz
: review+
jld
: review+
Details | Diff | Splinter Review
109.80 KB, patch
aklotz
: review+
jld
: review+
Details | Diff | Splinter Review
250.07 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
8.23 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
6.00 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
7.20 KB, patch
jld
: review+
Details | Diff | Splinter Review
6.25 KB, patch
jld
: review+
Details | Diff | Splinter Review
10.39 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
2.01 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
10.61 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
28.81 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
1.78 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
1.75 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
12.23 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
1.17 KB, patch
jld
: review+
Details | Diff | Splinter Review
16.05 KB, patch
jld
: review+
Details | Diff | Splinter Review
11.97 KB, patch
jld
: review+
Details | Diff | Splinter Review
1.77 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
6.12 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
1.54 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
27.25 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
6.39 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
1.46 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
6.39 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
6.15 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
7.10 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
(Assignee)

Description

7 months ago
Update security/sandbox/chromium/ to latest Chromium stable channel version once Fx56 is on m-c.
This should be chromium 59.
The way the dates work here mean that we might need to take some minor release patches as well.

Dropping this into milestone sbwc3, because I don't think we should block on this for sbwc2, but we'll want to do it soon afterwards if it doesn't make it into 56 for some reason.
(Assignee)

Comment 1

6 months ago
Working on updating to chromium commit: 9f4b44b898b326679817ee5a327256f8fac6ee75
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED

Updated

4 months ago
Priority: -- → P2
Whiteboard: sbwc3 → sb+
(Assignee)

Updated

a month ago
Priority: P2 → P1
Summary: Update security/sandbox/chromium/ to latest Chromium stable channel version in Fx56. → Update security/sandbox/chromium/ to Chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5.
(Assignee)

Comment 2

a month ago
Try pushes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cec1c8de9b63211128201a7a5ede94558a72868f
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dee8fd7d04bd2b6d5384f66099bf7ec947597065
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a68d2376cfe868cabe73978e0789508adf23684
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc19136484c675320c98ffe458259f6ac2aa10b7
(Assignee)

Comment 3

a month ago
I'm going to upload the patches in broken down form to hopefully aid review.

I'm not going to land until Fx59, so there isn't a massive rush for these.

Then I'll land as two rolled-up patches.
The first with the chromium update and any patches needed to get a compiling and mostly working Firefox.
The second with the rest of our changes.

I'm changing the way we track our own patches away from the modifications-to-chromium-to-reapply-after-upstream-merge.txt file.
Now there will be two directories in security/sandbox/chromium-shim/patches/.
* with_update/ - for patches that need to be applied to get a compiling and mostly working Firefox.
* after_update/ - for the rest.

Each dir will have a patch_order.txt file, as it matters in a couple of cases.

I won't upload all the separate patches that add in the patch files themselves as it will just create more noise.
(Assignee)

Comment 4

a month ago
Created attachment 8926372 [details] [diff] [review]
Update existing security/sandbox/chromium/base files to chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5

This includes removed files.
Attachment #8926372 - Flags: review?(jld)
Attachment #8926372 - Flags: review?(aklotz)
(Assignee)

Comment 5

a month ago
Created attachment 8926373 [details] [diff] [review]
Add new security/sandbox/chromium/base dependencies from chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5
Attachment #8926373 - Flags: review?(jld)
Attachment #8926373 - Flags: review?(aklotz)
(Assignee)

Comment 6

a month ago
Created attachment 8926374 [details] [diff] [review]
Update existing security/sandbox/chromium/sandbox/win files to chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5

This includes removed files.
Attachment #8926374 - Flags: review?(aklotz)
(Assignee)

Comment 7

a month ago
Created attachment 8926375 [details] [diff] [review]
[PATCH] Correct two bugs in Windows sandboxing alternate desktops:

These were fixes to our upstream patch that didn't make it into release Chromium.


From db4c64b63d6098294ed255e962700fd2d465575e Mon Sep 17 00:00:00 2001
When a parent has two alternate desktops, one on a local winstation, the
other on an alternate winstation:

1) Ensure the desktops have different names.
2) Ensure both desktops have their integrity level set correctly.

Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1229829#c11
Cq-Include-Trybots: master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Change-Id: I2d17779e389c9f74146a83d97d62babffa903184
Reviewed-on: https://chromium-review.googlesource.com/638872
Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499280}
---
Attachment #8926375 - Flags: review?(aklotz)
(Assignee)

Comment 8

a month ago
Created attachment 8926377 [details] [diff] [review]
Add new security/sandbox/chromium/sandbox/win dependencies from chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5
Attachment #8926377 - Flags: review?(aklotz)
(Assignee)

Comment 9

a month ago
Created attachment 8926378 [details] [diff] [review]
Update existing security/sandbox/chromium/sandbox/linux files to chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5
Attachment #8926378 - Flags: review?(jld)
(Assignee)

Comment 10

a month ago
Created attachment 8926379 [details] [diff] [review]
Add new security/sandbox/chromium/sandbox/linux dependencies from chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5
Attachment #8926379 - Flags: review?(jld)
(Assignee)

Comment 11

a month ago
Created attachment 8926380 [details] [diff] [review]
Changes to chromium-shim and mozilla code required for chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5
Attachment #8926380 - Flags: review?(aklotz)
(Assignee)

Comment 12

a month ago
Created attachment 8926381 [details] [diff] [review]
Update chromium's list of linux-x86-32 syscalls

Originally landed as changset:
https://hg.mozilla.org/mozilla-central/rev/adb1d2a92e0d
(Assignee)

Comment 13

a month ago
Comment on attachment 8926381 [details] [diff] [review]
Update chromium's list of linux-x86-32 syscalls

Carrying r=jld from original landing.
Attachment #8926381 - Flags: review+
(Assignee)

Comment 14

a month ago
Created attachment 8926383 [details] [diff] [review]
Reinstate sandbox::BrokerServices::AddTargetPeer

This is basically a revert of chromium commit 996b42db5296bd3d11b3d7fde1a4602bbcefed2c.

We still use AddTargetPeer, required when duplicating handles to non-sandboxed children.
Attachment #8926383 - Flags: review?(aklotz)
(Assignee)

Comment 15

a month ago
Created attachment 8926385 [details] [diff] [review]
Reinstate sandbox::TargetServices::BrokerDuplicateHandle

This basically reverts chromium commit 569193665184525ca366e65d0735f5c851106e43.

We still use BrokerDuplicateHandle, Chromium does this all explicitly from the parent now and so removed this.
Attachment #8926385 - Flags: review?(aklotz)
(Assignee)

Comment 16

a month ago
Created attachment 8926386 [details] [diff] [review]
Don't compile sandbox::ApplyMitigationsToCurrentThread

This brings in new dependencies via FilePath and we don't currently use it.
As far as I can tell Chromium doesn't use it either.
Attachment #8926386 - Flags: review?(aklotz)
(Assignee)

Comment 17

a month ago
Created attachment 8926388 [details] [diff] [review]
Don't compile base::Time::FromStringInternal

This has a dependency on nspr, which causes issues.

Originally landed in changeset:
https://hg.mozilla.org/mozilla-central/rev/477b991bf6fa7b4511768649c9bf37c7275d30d9
(Assignee)

Comment 18

a month ago
Comment on attachment 8926388 [details] [diff] [review]
Don't compile base::Time::FromStringInternal

Carrying r=aklotz from original landing.
Attachment #8926388 - Flags: review+
(Assignee)

Comment 19

a month ago
Created attachment 8926389 [details] [diff] [review]
Add option to Windows chromium sandbox policy to not use restricting SIDs

This originally landed in changeset:
https://hg.mozilla.org/mozilla-central/rev/14374cd9497a
(Assignee)

Comment 20

a month ago
Comment on attachment 8926389 [details] [diff] [review]
Add option to Windows chromium sandbox policy to not use restricting SIDs

Carrying r=jimm from original landing.
Attachment #8926389 - Flags: review+
(Assignee)

Comment 21

a month ago
Created attachment 8926391 [details] [diff] [review]
This removes sequence checking on RefCountedBase in DEBUG builds

We don't currently make use of it and it brings in many dependencies.
Attachment #8926391 - Flags: review?(jld)
(Assignee)

Comment 22

a month ago
Created attachment 8926400 [details] [diff] [review]
Revert usage of c++14 alias versions of standard type traits

This basically reverts chromium commit 35a31743fdc9006d5424fd2e1e15f6aba0a3c30c.
Attachment #8926400 - Flags: review?(jld)
(Assignee)

Comment 23

a month ago
Created attachment 8926402 [details] [diff] [review]
Revert usage of c++14 std::index_sequence

This basically reverts chromium commit 84956fa86786f73fa41ebf99e2f6b8d549688c91.
Attachment #8926402 - Flags: review?(jld)
(Assignee)

Comment 24

a month ago
Created attachment 8926404 [details] [diff] [review]
Add a void cast to silence clang Wcomma build warning, in sandbox's snapshot of chromium header

The build warning is for "possible misuse of comma operator".

The comma operator is a bit of a footgun becasue its first operand's result
just gets dropped on the floor (in this case, the result of the DCHECK
expression).  It appears that Chromium's use of the comma operator here is
intentional, though -- so we might as well accept clang's suggestion and "cast
expression to void to silence warning".

This is also filed upstream as:
 https://bugs.chromium.org/p/chromium/issues/detail?id=729123
(Assignee)

Comment 25

a month ago
Comment on attachment 8926404 [details] [diff] [review]
Add a void cast to silence clang Wcomma build warning, in sandbox's snapshot of chromium header

Carrying r=bobowen from original landing.
Attachment #8926404 - Flags: review+
(Assignee)

Comment 26

a month ago
Created attachment 8926405 [details] [diff] [review]
Allow a special all paths rule in the Windows process sandbox when using semantics FILES_ALLOW_READONLY

This also changes the read only related status checks in filesystem_interception.cc to include STATUS_NETWORK_OPEN_RESTRICTION (0xC0000201), which gets returned in some cases and fails because we never ask the broker.
(Assignee)

Comment 27

a month ago
Comment on attachment 8926405 [details] [diff] [review]
Allow a special all paths rule in the Windows process sandbox when using semantics FILES_ALLOW_READONLY

Carrying r=jimm from original landing.
Attachment #8926405 - Flags: review+
(Assignee)

Comment 28

a month ago
Created attachment 8926407 [details] [diff] [review]
Revert commit f7540af7428f4b146136ec19b781886693f8c03f changes to policy_target.cc for causing issues with CoInitializeSecurity
(Assignee)

Comment 29

a month ago
Comment on attachment 8926407 [details] [diff] [review]
Revert commit f7540af7428f4b146136ec19b781886693f8c03f changes to policy_target.cc for causing issues with CoInitializeSecurity

Carrying r=aklotz from original landing.
Attachment #8926407 - Flags: review+
(Assignee)

Comment 30

a month ago
Created attachment 8926408 [details] [diff] [review]
Re-apply - Logging changes to the Chromium interception code

Originally landed as changset:
https://hg.mozilla.org/mozilla-central/rev/0f763c186855
(Assignee)

Comment 31

a month ago
Comment on attachment 8926408 [details] [diff] [review]
Re-apply - Logging changes to the Chromium interception code

Carrying r=tabraldes from the original landing.
Attachment #8926408 - Flags: review+
(Assignee)

Comment 32

a month ago
Created attachment 8926409 [details] [diff] [review]
Change to allow network drives in sandbox rules with non-file device fix

Originally landed in changeset:
https://hg.mozilla.org/mozilla-central/rev/c70d06fa5302
(Assignee)

Comment 33

a month ago
Comment on attachment 8926409 [details] [diff] [review]
Change to allow network drives in sandbox rules with non-file device fix

Carrying r=aklotz from original landing.
Attachment #8926409 - Flags: review+
(Assignee)

Comment 34

a month ago
Created attachment 8926410 [details] [diff] [review]
Add KEY_WOW64_64Key and KEY_WOW64_32KEY to the Chromium sandbox allowed registry read flags

Originally landed as changeset:
https://hg.mozilla.org/mozilla-central/rev/d24db55deb85
(Assignee)

Comment 35

a month ago
Comment on attachment 8926410 [details] [diff] [review]
Add KEY_WOW64_64Key and KEY_WOW64_32KEY to the Chromium sandbox allowed registry read flags

Carrying r=aklotz from original landing.
Attachment #8926410 - Flags: review+
(Assignee)

Comment 36

a month ago
Created attachment 8926412 [details] [diff] [review]
Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs

Originally landed in changeset:
https://hg.mozilla.org/mozilla-central/rev/0e6bf137521e
(Assignee)

Comment 37

a month ago
Comment on attachment 8926412 [details] [diff] [review]
Change USER_NON_ADMIN access token level from whitelist to blacklist containing Admin SIDs

Carrying r=jimm from original landing.
Attachment #8926412 - Flags: review+
(Assignee)

Comment 38

a month ago
Created attachment 8926413 [details] [diff] [review]
Add mechanism to libsandbox_s to track names of files that have been given special sandbox access permissions (PermissionsService)

Hook this into the browser via the XREAppData. This patch contains only the changes to Chromium source code.

Originally landed in changeset:
https://hg.mozilla.org/mozilla-central/rev/6ecd19d25822
(Assignee)

Comment 39

a month ago
Comment on attachment 8926413 [details] [diff] [review]
Add mechanism to libsandbox_s to track names of files that have been given special sandbox access permissions (PermissionsService)

Carrying r=bobowen from original landing.
Attachment #8926413 - Flags: review+
(Assignee)

Comment 40

a month ago
Created attachment 8926414 [details] [diff] [review]
Permit sandboxed processes to access Flash temporary files

Allows the creation/use of temp files when the user has already green-lit
the use of a file for write purposes in that folder.

Originally landed in changeset:
https://hg.mozilla.org/mozilla-central/rev/0f64b24c40c4
(Assignee)

Comment 41

a month ago
Comment on attachment 8926414 [details] [diff] [review]
Permit sandboxed processes to access Flash temporary files

Carrying r=bobowen from original landing.
Attachment #8926414 - Flags: review+
(Assignee)

Updated

a month ago
Depends on: 1415569
(Assignee)

Updated

a month ago
Blocks: 1378417
Comment on attachment 8926372 [details] [diff] [review]
Update existing security/sandbox/chromium/base files to chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5

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

rs=me
Attachment #8926372 - Flags: review?(aklotz) → review+

Updated

28 days ago
Attachment #8926373 - Flags: review?(aklotz) → review+
Comment on attachment 8926374 [details] [diff] [review]
Update existing security/sandbox/chromium/sandbox/win files to chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5

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

rs=me
Attachment #8926374 - Flags: review?(aklotz) → review+

Updated

28 days ago
Attachment #8926375 - Flags: review?(aklotz) → review+

Updated

28 days ago
Attachment #8926377 - Flags: review?(aklotz) → review+

Updated

28 days ago
Attachment #8926380 - Flags: review?(aklotz) → review+

Updated

28 days ago
Attachment #8926383 - Flags: review?(aklotz) → review+

Updated

28 days ago
Attachment #8926385 - Flags: review?(aklotz) → review+

Updated

28 days ago
Attachment #8926386 - Flags: review?(aklotz) → review+
Comment on attachment 8926372 [details] [diff] [review]
Update existing security/sandbox/chromium/base files to chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5

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

I've either read or at least skimmed through everything here that isn't Windows-specific.
Attachment #8926372 - Flags: review?(jld) → review+
Attachment #8926373 - Flags: review?(jld) → review+
Attachment #8926378 - Flags: review?(jld) → review+
Attachment #8926379 - Flags: review?(jld) → review+
Attachment #8926391 - Flags: review?(jld) → review+
Attachment #8926400 - Flags: review?(jld) → review+
Attachment #8926402 - Flags: review?(jld) → review+
(Assignee)

Comment 45

21 days ago
Thanks for all the reviews:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=620898799dee8450582f4b718b2f175b4ee4d34c

Comment 46

20 days ago
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d87028e56a05
Part 1: Roll-up of chromium sandbox update and mozilla patches to get a running browser. r=jld,aklotz,jimm,bobowen
https://hg.mozilla.org/integration/mozilla-inbound/rev/387f44470903
Part 2: Roll-up patch to apply remaining mozilla changes to chromium sandbox. r=tabraldes,aklotz,jimm,bobowen

Comment 47

20 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d87028e56a05
https://hg.mozilla.org/mozilla-central/rev/387f44470903
Status: ASSIGNED → RESOLVED
Last Resolved: 20 days ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
https://hg.mozilla.org/projects/oak/rev/d87028e56a054954aabd4936d787095d79a7babc
Bug 1366701 Part 1: Roll-up of chromium sandbox update and mozilla patches to get a running browser. r=jld,aklotz,jimm,bobowen

https://hg.mozilla.org/projects/oak/rev/387f44470903a99137251849adf382b82e66c159
Bug 1366701 Part 2: Roll-up patch to apply remaining mozilla changes to chromium sandbox. r=tabraldes,aklotz,jimm,bobowen
You need to log in before you can comment on or make changes to this bug.