[Windows] Whitelist the per-user extensions dir XRE_USER_SYS_EXTENSION_DIR

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: haik, Assigned: haik)

Tracking

56 Branch
mozilla58
Unspecified
Windows
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: sb+)

Attachments

(2 attachments)

(Assignee)

Description

a year ago
We should whitelist the per-user extensions dir for legacy extensions being used on build 56.
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
I haven't had a chance to test the posted patch on release/56 yet. I haven't touched sandboxBroker.cpp before, but could take this bug given how this and bug 1393805 are related and both will be needed in 56.
(Assignee)

Updated

a year ago
Assignee: nobody → haftandilian
Whiteboard: sb+

Comment 3

a year ago
mozreview-review
Comment on attachment 8913060 [details]
Bug 1403744 - Part 1 - Whitelist the per-user extensions dir XRE_USER_SYS_EXTENSION_DIR on Windows.

https://reviewboard.mozilla.org/r/184462/#review190060

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:753
(Diff revision 1)
> +
> +#ifdef ENABLE_SYSTEM_EXTENSION_DIRS
> +    // Read access to the per-user extensions directory
> +    AddCachedDirRule(mPolicy, sandbox::TargetPolicy::FILES_ALLOW_READONLY,
> +                     sUserExtensionsDir, NS_LITERAL_STRING("\\*"));
> +#endif

Just started to look at this when I realised you'd already taken it, thanks!

This looks good except that I assume this should be in SetSecurityLevelForContentProcess.

Also in SetSecurityLevelForContentProcess would you mind fixing the existing comments for chrome and extensions, I missed a "to " out before them.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8913060 [details]
Bug 1403744 - Part 1 - Whitelist the per-user extensions dir XRE_USER_SYS_EXTENSION_DIR on Windows.

https://reviewboard.mozilla.org/r/184462/#review190060

> Just started to look at this when I realised you'd already taken it, thanks!
> 
> This looks good except that I assume this should be in SetSecurityLevelForContentProcess.
> 
> Also in SetSecurityLevelForContentProcess would you mind fixing the existing comments for chrome and extensions, I missed a "to " out before them.

Thanks. Moved the AddCachedDirRule() call to SetSecurityLevelForContentProcess() and added a test. The test is only enabled for Windows and Mac because build 56 for Linux doesn't include $HOME read-access restrictions so none of this applies.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8913060 [details]
Bug 1403744 - Part 1 - Whitelist the per-user extensions dir XRE_USER_SYS_EXTENSION_DIR on Windows.

https://reviewboard.mozilla.org/r/184462/#review190060

> Thanks. Moved the AddCachedDirRule() call to SetSecurityLevelForContentProcess() and added a test. The test is only enabled for Windows and Mac because build 56 for Linux doesn't include $HOME read-access restrictions so none of this applies.

Update: I made the test just apply to Windows. The tests need some more involved changes to make them work properly with the per-user extension dir on Mac and I don't want to invest time on that now, given that this stuff is only relevant to 56.

Comment 11

a year ago
mozreview-review
Comment on attachment 8913060 [details]
Bug 1403744 - Part 1 - Whitelist the per-user extensions dir XRE_USER_SYS_EXTENSION_DIR on Windows.

https://reviewboard.mozilla.org/r/184462/#review190488
Attachment #8913060 - Flags: review?(bobowencode) → review+

Comment 12

a year ago
mozreview-review
Comment on attachment 8913853 [details]
Bug 1403744 - Part 2 - Test that the per-user extensions dir is readable from content on Windows.

https://reviewboard.mozilla.org/r/185250/#review190492

The comment for ENABLE_SYSTEM_EXTENSION_DIRS suggests it might not be defined for test builds, does that not affect this test?
Also, do we need to allow for that in some circumstances or perhaps the comment is out of date.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

a year ago
mozreview-review-reply
Comment on attachment 8913853 [details]
Bug 1403744 - Part 2 - Test that the per-user extensions dir is readable from content on Windows.

https://reviewboard.mozilla.org/r/185250/#review190492

Good point. The test was clean on try and I don't see any reference to the --disable-system-extension-dirs switch in any config files. I suspect we don't need to worry about the test concerns mentioned in old-configure.in because we run our tests on filesystems that (I assume) are rolled back for each new job. I'm trying to confirm that. It would be an inconvenience if someone is running locally and they're disabling the dir and running this test.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=917f477468c580e78b1cd5c83a519a049c6d266f

Comment 16

a year ago
mozreview-review
Comment on attachment 8913853 [details]
Bug 1403744 - Part 2 - Test that the per-user extensions dir is readable from content on Windows.

https://reviewboard.mozilla.org/r/185250/#review190654

r+ so we can get this landed and ready for potential uplfit (assuming we're reasonably confident this isn't going to trip people up with that configure flag).
Attachment #8913853 - Flags: review?(bobowencode) → review+
(Assignee)

Comment 17

a year ago
mozreview-review-reply
Comment on attachment 8913853 [details]
Bug 1403744 - Part 2 - Test that the per-user extensions dir is readable from content on Windows.

https://reviewboard.mozilla.org/r/185250/#review190654

I think it's reasonable to land the test. If it does end up causing problems, the test changes can be backed out without additional risk. Backing out the test would be OK given how use of the directory to autoload legacy extensions is about to be unsupported.

Comment 18

a year ago
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dfbab5ec9995
Part 1 - Whitelist the per-user extensions dir XRE_USER_SYS_EXTENSION_DIR on Windows. r=bobowen
https://hg.mozilla.org/integration/autoland/rev/2f6fa388b067
Part 2 - Test that the per-user extensions dir is readable from content on Windows. r=bobowen
https://hg.mozilla.org/mozilla-central/rev/dfbab5ec9995
https://hg.mozilla.org/mozilla-central/rev/2f6fa388b067
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.