Closed Bug 1377555 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Security: Process Sandboxing, defect)

56 Branch
All
Windows
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox55 --- verified
firefox56 --- verified

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

(Whiteboard: sbwc2)

Attachments

(3 files)

The check added in bug 1323188 needs to resolve symlinks and junction points before checking if Firefox is installed on a network drive.
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
Flags: needinfo?(davidp99)
I'll split the patch into a backout and new patch.
This backouts the previous change to detect and change the sandbox policy
when running from a network drive.
Attachment #8884922 - Flags: review?(jmathies)
Blocks: 1314801
Attachment #8884922 - Flags: review?(jmathies) → review+
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.
Attachment #8884924 - Flags: review?(jmathies) → review+
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.
Attachment #8884926 - Flags: review?(jmathies) → review+
Blocks: 1377249
Flags: needinfo?(davidp99)
(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.
Pushed by bobowencode@gmail.com:
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 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
Attachment #8884922 - Flags: approval-mozilla-beta?
Comment on attachment 8884924 [details] [diff] [review]
Part 2: Add option to Windows chromium sandbox policy to not use restricting SIDs

See comment 12
Attachment #8884924 - Flags: approval-mozilla-beta?
Comment on attachment 8884926 [details] [diff] [review]
Part 3: Don't use restricting SIDs when running from a network drive

See comment 12
Attachment #8884926 - Flags: approval-mozilla-beta?
Comment on attachment 8884922 [details] [diff] [review]
Part 1: Back out changesets 04edb03fb817 and d17ac655cc51

windows sandbox fix, beta55+
Attachment #8884922 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8884924 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8884926 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.