1377555 - Running from a symlinked network drive will fail with restricting SIDs.

Status: VERIFIED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

The check added in bug 1323188 needs to resolve symlinks and junction points before checking if Firefox is installed on a network drive.

Comment 1

[Bob Owen (:bobowen)]

Here's a try push to fix this, I've also reworked the test for the network drive and moved the setting to not use restricting SIDs properly into the policy.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dab1d5955b3b594e12dc0518b2f6962b4354b64

handyman - for info rather that need info - this is the changeset for this bug and it should make the NPAPI change fairly straight forward.
https://hg.mozilla.org/try/rev/5f2b457218a8836f20f61b8127f1e4f04e0e83ed

Comment 2

[Bob Owen (:bobowen)]

I'll split the patch into a backout and new patch.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 1: Back out changesets 04edb03fb817 and d17ac655cc51

This backouts the previous change to detect and change the sandbox policy
when running from a network drive.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 2: Add option to Windows chromium sandbox policy to not use restricting SIDs

Comment 5

[Bob Owen (:bobowen)]

Attachment: Part 3: Don't use restricting SIDs when running from a network drive

Comment 6

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42c93cf7363be923b855aa850d18242314a022a4

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8884924 [details] [diff] [review]
Part 2: Add option to Windows chromium sandbox policy to not use restricting SIDs

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

::: security/sandbox/chromium/sandbox/win/src/restricted_token_utils.cc
@@ +48,5 @@
>  
> +      if (use_restricting_sids) {
> +        unsigned err_code = restricted_token.AddRestrictingSidAllSids();
> +        if (ERROR_SUCCESS != err_code)
> +          return err_code;

personally I'd prefer brackets here to avoid confusion with the closing outer bracket.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8884926 [details] [diff] [review]
Part 3: Don't use restricting SIDs when running from a network drive

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

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +29,5 @@
>  namespace mozilla
>  {
>  
>  sandbox::BrokerServices *SandboxBroker::sBrokerService = nullptr;
> +bool SandboxBroker::sRunningFromNetworkDrive = false;

comment me please

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.h
@@ +64,5 @@
>    void ApplyLoggingPolicy();
>  
>  private:
>    static sandbox::BrokerServices *sBrokerService;
> +  static bool sRunningFromNetworkDrive;

or here.

Comment 9

[Bob Owen (:bobowen)]

(In reply to Jim Mathies [:jimm] from comment #7)
...
> > +      if (use_restricting_sids) {
> > +        unsigned err_code = restricted_token.AddRestrictingSidAllSids();
> > +        if (ERROR_SUCCESS != err_code)
> > +          return err_code;
> 
> personally I'd prefer brackets here to avoid confusion with the closing
> outer bracket.

Yes, so do I, fixed locally. That's existing style we don't want to keep, thanks.

(In reply to Jim Mathies [:jimm] from comment #8)
...
> > +bool SandboxBroker::sRunningFromNetworkDrive = false;
> 
> comment me please

I've added the comment here locally, so I can explain why we default to false.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58aa65eab7ce
Part 1: Back out changesets 04edb03fb817 and d17ac655cc51. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/14374cd9497a
Part 2: Add option to Windows chromium sandbox policy to not use restricting SIDs. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/872b3ecea103
Part 3: Don't use restricting SIDs when running from a network drive. r=jimm

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58aa65eab7ce
https://hg.mozilla.org/mozilla-central/rev/14374cd9497a
https://hg.mozilla.org/mozilla-central/rev/872b3ecea103

Comment 12

[Bob Owen (:bobowen)]

Comment on attachment 8884922 [details] [diff] [review]
Part 1: Back out changesets 04edb03fb817 and d17ac655cc51

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1323188

[User impact if declined]:
Bug 1377249 will not be able to be uplifted.

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Verified in local build

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes, running from a symlinked network drive on Nightly will fail.
* Copy firefox to network drive.
* As admin create local symlink to dir: mklink /D <path_link_to_create> <path_to_network_folder>
* Run firefox from symlink

[List of other uplifts needed for the feature/fix]:
Bug 1378061 should be uplifted before this and for a separate issue bug 1377249 afterwards.

[Is the change risky?]:
Low

[Why is the change risky/not risky?]:
Fairly simple additions of a new policy member that then controls whether we use restricting SIDs.

[String changes made/needed]:
None

Comment 13

[Bob Owen (:bobowen)]

Comment on attachment 8884924 [details] [diff] [review]
Part 2: Add option to Windows chromium sandbox policy to not use restricting SIDs

See comment 12

Comment 14

[Bob Owen (:bobowen)]

Comment on attachment 8884926 [details] [diff] [review]
Part 3: Don't use restricting SIDs when running from a network drive

See comment 12

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 8884922 [details] [diff] [review]
Part 1: Back out changesets 04edb03fb817 and d17ac655cc51

windows sandbox fix, beta55+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/60fdd29667ad
https://hg.mozilla.org/releases/mozilla-beta/rev/4d917a319e6f
https://hg.mozilla.org/releases/mozilla-beta/rev/a15ebd1c523c

Comment 17

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce the issue on Firefox 56.0a1 (2017-06-30), under Windows 7x64 and using the STR from Comment 12.
The issue is no longer reproducible on Firefox 56.0a1 (2017-07-18), or on Firefox 55.0b10, using the same STR as above.
Tests were performed under Windows 7x64.