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

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P1)

Description

[Bob Owen (:bobowen)]

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.

Comment 1

[Bob Owen (:bobowen)]

Working on updating to chromium commit: 9f4b44b898b326679817ee5a327256f8fac6ee75

Comment 2

[Bob Owen (:bobowen)]

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

Comment 3

[Bob Owen (:bobowen)]

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.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Update existing security/sandbox/chromium/base files to chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5

This includes removed files.

Comment 5

[Bob Owen (:bobowen)]

Attachment: Add new security/sandbox/chromium/base dependencies from chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5

Comment 6

[Bob Owen (:bobowen)]

Attachment: Update existing security/sandbox/chromium/sandbox/win files to chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5

This includes removed files.

Comment 7

[Bob Owen (:bobowen)]

Attachment: [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 <[e-mail]>
Commit-Queue: Will Harris <[e-mail]>
Cr-Commit-Position: refs/heads/master@{#499280}
---

Comment 8

[Bob Owen (:bobowen)]

Attachment: Add new security/sandbox/chromium/sandbox/win dependencies from chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5

Comment 9

[Bob Owen (:bobowen)]

Attachment: Update existing security/sandbox/chromium/sandbox/linux files to chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5

Comment 10

[Bob Owen (:bobowen)]

Attachment: Add new security/sandbox/chromium/sandbox/linux dependencies from chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5

Comment 11

[Bob Owen (:bobowen)]

Attachment: Changes to chromium-shim and mozilla code required for chromium commit ba7a0041073a5e9928d277806bfe24c325d113e5

Comment 12

[Bob Owen (:bobowen)]

Attachment: Update chromium's list of linux-x86-32 syscalls

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

Comment 13

[Bob Owen (:bobowen)]

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

Carrying r=jld from original landing.

Comment 14

[Bob Owen (:bobowen)]

Attachment: 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.

Comment 15

[Bob Owen (:bobowen)]

Attachment: 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.

Comment 16

[Bob Owen (:bobowen)]

Attachment: 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.

Comment 17

[Bob Owen (:bobowen)]

Attachment: 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

Comment 18

[Bob Owen (:bobowen)]

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

Carrying r=aklotz from original landing.

Comment 19

[Bob Owen (:bobowen)]

Attachment: 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

Comment 20

[Bob Owen (:bobowen)]

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.

Comment 21

[Bob Owen (:bobowen)]

Attachment: This removes sequence checking on RefCountedBase in DEBUG builds

We don't currently make use of it and it brings in many dependencies.

Comment 22

[Bob Owen (:bobowen)]

Attachment: Revert usage of c++14 alias versions of standard type traits

This basically reverts chromium commit 35a31743fdc9006d5424fd2e1e15f6aba0a3c30c.

Comment 23

[Bob Owen (:bobowen)]

Attachment: Revert usage of c++14 std::index_sequence

This basically reverts chromium commit 84956fa86786f73fa41ebf99e2f6b8d549688c91.

Comment 24

[Bob Owen (:bobowen)]

Attachment: 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

Comment 25

[Bob Owen (:bobowen)]

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.

Comment 26

[Bob Owen (:bobowen)]

Attachment: 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.

Comment 27

[Bob Owen (:bobowen)]

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.

Comment 28

[Bob Owen (:bobowen)]

Attachment: Revert commit f7540af7428f4b146136ec19b781886693f8c03f changes to policy_target.cc for causing issues with CoInitializeSecurity

Comment 29

[Bob Owen (:bobowen)]

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.

Comment 30

[Bob Owen (:bobowen)]

Attachment: Re-apply - Logging changes to the Chromium interception code

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

Comment 31

[Bob Owen (:bobowen)]

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

Carrying r=tabraldes from the original landing.

Comment 32

[Bob Owen (:bobowen)]

Attachment: 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

Comment 33

[Bob Owen (:bobowen)]

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.

Comment 34

[Bob Owen (:bobowen)]

Attachment: 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

Comment 35

[Bob Owen (:bobowen)]

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.

Comment 36

[Bob Owen (:bobowen)]

Attachment: 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

Comment 37

[Bob Owen (:bobowen)]

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.

Comment 38

[Bob Owen (:bobowen)]

Attachment: 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

Comment 39

[Bob Owen (:bobowen)]

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.

Comment 40

[Bob Owen (:bobowen)]

Attachment: 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

Comment 41

[Bob Owen (:bobowen)]

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

Carrying r=bobowen from original landing.

Comment 42

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 43

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 44

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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.

Comment 45

[Bob Owen (:bobowen)]

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

Comment 46

[Pulsebot]

Pushed by [e-mail]:
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

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/d87028e56a05
https://hg.mozilla.org/mozilla-central/rev/387f44470903

Comment 48

[Sylvestre Ledru [:Sylvestre]]

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