Closed Bug 1490234 Opened Last year Closed 11 months ago

Shared memory should not allow executable images to be mapped on Windows.

Categories

(Core :: IPC, defect, P1)

All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 63+ fixed
firefox62 --- wontfix
firefox63 + fixed
firefox64 + fixed

People

(Reporter: bobowen, Assigned: bobowen)

Details

(Keywords: csectype-priv-escalation, sec-high, Whiteboard: [post-critsmash-triage][adv-main63+][adv-esr60.3+])

Attachments

(2 files, 1 obsolete file)

I noticed the use of STANDARD_RIGHTS_REQUIRED in our Windows shared memory code.
When I searched for this I immediately hit [1], which basically says don't use it.

Checking the chromium code, they got rid of this in bug [2].
According to the fourth comment on that bug it seems like it's probably not an issue, but it isn't needed anyway.

However while looking at their existing code I noticed bug [3], which looks more troublesome and could make sandbox escapes easier.
Looks like this ipc chromium bug fell between a search on visible security bugs we did a while ago and getting the security bug notifications from chromium.

[1] https://blogs.msdn.microsoft.com/oldnewthing/20080227-00/?p=23303/
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=307301
[3] https://bugs.chromium.org/p/chromium/issues/detail?id=564238
The code has changed a fair bit since we branched, but I think this gives the same effect as the patches on their code.

Diffs for their patches:
https://chromium.googlesource.com/chromium/src/+/0474abea8469d78ce3988364ee273984ac49a9f2%5E%21/
https://chromium.googlesource.com/chromium/src/+/5c74461a29d66197afa19936247a0854ac4b1f01%5E%21/
Attachment #9007996 - Flags: review?(jld)
Group: core-security → dom-core-security
Version: 1.9.2 Branch → unspecified
I'm going to mark this as sec-high, but feel free to adjust it.
Comment on attachment 9007996 [details] [diff] [review]
Don't allow executable image sections to be mapped for SharedMemory on Windows

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

r=me with some minor changes:

::: ipc/chromium/src/base/shared_memory_win.cc
@@ +13,5 @@
>  
> +namespace {
> +typedef enum _SECTION_INFORMATION_CLASS {
> +  SectionBasicInformation,
> +} SECTION_INFORMATION_CLASS;

A comment about the reason these definitions can't be included from a system header would be helpful.

@@ +32,5 @@
> +// Checks if the section object is safe to map. At the moment this just means
> +// it's not an image section.
> +bool IsSectionSafeToMap(HANDLE handle) {
> +  static NtQuerySectionType nt_query_section_func;
> +  if (!nt_query_section_func) {

This isn't thread-safe.  I suggest using a static local with an initializer instead; they're evaluated the first time they're reached, with thread safety provided by the C++ runtime.
Attachment #9007996 - Flags: review?(jld) → review+
cc :kmag because this affects read-only shared memory.  Also, does MemMapSnapshot::Freeze need to specify SECTION_QUERY now?
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #4)
> cc :kmag because this affects read-only shared memory.  Also, does
> MemMapSnapshot::Freeze need to specify SECTION_QUERY now?

I suppose if we want to add a similar sanity check for those mappings child-side it does, yeah. It already specifies reasonable access rights, though.
Carrying r+ from jld in comment 3.

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #3)
...
> ::: ipc/chromium/src/base/shared_memory_win.cc
> @@ +13,5 @@
> >  
> > +namespace {
> > +typedef enum _SECTION_INFORMATION_CLASS {
> > +  SectionBasicInformation,
> > +} SECTION_INFORMATION_CLASS;
> 
> A comment about the reason these definitions can't be included from a system
> header would be helpful.

Comment added.
 
> @@ +32,5 @@
> > +// Checks if the section object is safe to map. At the moment this just means
> > +// it's not an image section.
> > +bool IsSectionSafeToMap(HANDLE handle) {
> > +  static NtQuerySectionType nt_query_section_func;
> > +  if (!nt_query_section_func) {
> 
> This isn't thread-safe.  I suggest using a static local with an initializer
> instead; they're evaluated the first time they're reached, with thread
> safety provided by the C++ runtime.

I remembered that we owned this code and started to make style changes, but I forgot about changing this, thanks.
For simplicity I just used the GetProcAddress as the initializer and made the check unconditional as it's debug only.
Attachment #9008730 - Flags: review+
Attachment #9007996 - Attachment is obsolete: true
Comment on attachment 9008730 [details] [diff] [review]
Don't allow executable image sections to be mapped for SharedMemory on Windows

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
There is no exploit here, but something that might be used as part of a sandbox escape.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It's pretty obvious from the patch what we're trying to prevent and it wouldn't take much searching for someone to find the reasons why.

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch applies to Beta and Release.
I'll upload a rebased patch for ESR60.

How likely is this patch to cause regressions; how much testing does it need?
This is very similar to the chromium sandbox code that we already have, so I think regressions are unlikely. I doubt the extra system call when mapping shared memory would have a noticeable effect.
Attachment #9008730 - Flags: sec-approval?
Attachment #9008769 - Attachment description: ESR60 patch - Don't allow executable image sections to be mapped for SharedMemory on Windowsesr60-bug1490234.patch → ESR60 patch - Don't allow executable image sections to be mapped for SharedMemory on Windows
sec-approval+ for trunk. We'll want the patch nominated for beta.
Attachment #9008730 - Flags: sec-approval? → sec-approval+
Comment on attachment 9008730 [details] [diff] [review]
Don't allow executable image sections to be mapped for SharedMemory on Windows

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

::: ipc/chromium/src/base/shared_memory_win.cc
@@ +36,5 @@
> +// it's not an image section.
> +bool IsSectionSafeToMap(HANDLE handle) {
> +  static NtQuerySectionType nt_query_section_func =
> +    reinterpret_cast<NtQuerySectionType>(
> +      ::GetProcAddress(::GetModuleHandle(L"ntdll.dll"), "NtQuerySection"));

Note we have a helper for loading dynamic symbols like these: https://searchfox.org/mozilla-central/source/mozglue/misc/DynamicallyLinkedFunctionPtr.h#24
(In reply to Kris Maglione [:kmag] from comment #10)
...
> > +  static NtQuerySectionType nt_query_section_func =
> > +    reinterpret_cast<NtQuerySectionType>(
> > +      ::GetProcAddress(::GetModuleHandle(L"ntdll.dll"), "NtQuerySection"));
> 
> Note we have a helper for loading dynamic symbols like these:
> https://searchfox.org/mozilla-central/source/mozglue/misc/
> DynamicallyLinkedFunctionPtr.h#24

Thanks, I did not know about that.
Given this was all approved I didn't think it was worth changing it in this case, but I'll remember for next time.
Comment on attachment 9008730 [details] [diff] [review]
Don't allow executable image sections to be mapped for SharedMemory on Windows

Approval Request Comment
[Feature/Bug causing the regression]:
Since initial chromium IPC import.

[User impact if declined]:
Chance that the issue can be used to make a sandbox escape easier.

[Is this code covered by automated tests?]:
The code is probably hit in most tests.

[Has the fix been verified in Nightly?]:
No

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Very similar to code in our copy of the chromium sandbox code and in chromium itself.

[String changes made/needed]:
None
Attachment #9008730 - Flags: approval-mozilla-beta?
Comment on attachment 9008769 [details] [diff] [review]
ESR60 patch - Don't allow executable image sections to be mapped for SharedMemory on Windows

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
sec-high

User impact if declined:
Chance that the issue can be used to make a sandbox escape easier.

Fix Landed on Version:
64 (on inbound)

Risk to taking this patch (and alternatives if risky):
Low: this is very similar to code in our copy of the chromium sandbox code and in chromium itself.

String or UUID changes made by this patch:
None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #9008769 - Flags: approval-mozilla-esr60?
Attachment #9008730 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/e628509972b9
https://hg.mozilla.org/releases/mozilla-beta/rev/0b2b3eb663d9
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Comment on attachment 9008769 [details] [diff] [review]
ESR60 patch - Don't allow executable image sections to be mapped for SharedMemory on Windows

Fixes a sec-high, approved for ESR 60.3.
Attachment #9008769 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main63+][adv-esr60.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.