Closed Bug 1403744 Opened 7 years ago Closed 7 years ago

[Windows] Whitelist the per-user extensions dir XRE_USER_SYS_EXTENSION_DIR

Categories

(Core :: Security: Process Sandboxing, enhancement)

Product:
Core
Component:
Security: Process Sandboxing
Version:
56 Branch
Platform:
Unspecified
Windows
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: haik, Assigned: haik)

Details

(Whiteboard: sb+)

Attachments

(2 files)

Bug 1403744 - Part 1 - Whitelist the per-user extensions dir XRE_USER_SYS_EXTENSION_DIR on Windows.
59 bytes, text/x-review-board-request
bobowen
: review+
Details
Bug 1403744 - Part 2 - Test that the per-user extensions dir is readable from content on Windows.
59 bytes, text/x-review-board-request
bobowen
: review+
Details 
We should whitelist the per-user extensions dir for legacy extensions being used on build 56.
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: nobody → haftandilian
Whiteboard: sb+
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 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 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 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 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 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 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+
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.
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: