1872920 - (CVE-2024-2605) Sandbox escape via process type confusion in mozwer

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Miloš]

Attachment: vn0027.patch

Sandbox escape via process type confusion in mozwer

TL;DR: A compromised content process on Windows 10 & Windows 11 can pretend to be a browser process, which will lead the mozwer module to spawn a crashreporter.exe process that inherits environment variables from the content process. By controlling these environment variables, the content process can execute arbitrary code from within the crash reporter and escape the sandbox.

Details

When one of the firefox processes throws an exception that must be handled out-of-process, a WER (Windows Error Reporting) process is spawned, which loads mozwer.dll and executes OutOfProcessExceptionEventCallback. At the end of this function, there's an attempt to determine whether the exception-throwing process is a browser process or a content process:

let wer_data = read_from_process::<InProcessWindowsErrorReportingData>(
    process,
    context as *mut InProcessWindowsErrorReportingData,
)?;

...

if wer_data.process_type == MAIN_PROCESS_TYPE {
    handle_main_process_crash(crash_report, process, &application_info)
} else {
    handle_child_process_crash(crash_report, process)
}

Where the context variable contains the address of the gInProcessWerData structure residing in the memory of the crashing process:

void RegisterRuntimeExceptionModule() {
...
gInProcessWerData.mProcessType = mozilla::GetGeckoProcessType();
gInProcessWerData.mOOMAllocationSizePtr = &gOOMAllocationSize;
if (FAILED(::WerRegisterRuntimeExceptionModule(sModulePath,
                                                 &gInProcessWerData))) {
    // The registration failed null out sModulePath to record this.
    *sModulePath = L'\0';
    return;
}
...
}

Since wer_data is read from the memory of the crashing process, a content process can pose as a browser process by simply modifying gInProcessWerData.mProcessType within its own memory. Once that happens, handle_main_process_crash will be executed instead of handle_child_process_crash. This execution path launches the crash reporter:

...
let mut environment = read_environment_block(process)?;
launch_crash_reporter_client(
    &application_information.install_path,
    &mut environment,
    &crash_report,
);
...

fn launch_crash_reporter_client(
    install_path: &Path,
    environment: &mut Vec<u16>,
    crash_report: &CrashReport,
) {
...
unsafe {
    if CreateProcessW(
        null_mut(),
        cmd_line.as_mut_ptr(),
        null_mut(),
        null_mut(),
        FALSE,
        NORMAL_PRIORITY_CLASS | CREATE_NO_WINDOW | CREATE_UNICODE_ENVIRONMENT,
        environment.as_mut_ptr() as *mut _,
        null_mut(),
        &mut si,
        &mut pi,
    ) != 0
    {
        CloseHandle(pi.hProcess);
        CloseHandle(pi.hThread);
    }
}
}

When launching the crash reporter, the environment variables are inherited from the crashing process. As such, a content process can set its own environment variables and have them also be set in the launched crash reporter process. By controlling the environment variables inside the crash reporter process, there are multiple paths that a content process could use to achieve arbitrary code exec from the crash reporter (thus, outside the sandbox):

• By abusing the MOZ_CRASHREPORTER_RESTART_ARG_* environment variables. These environment variables are used to determine the command line that should be executed in order to restart firefox. For example, by setting MOZ_CRASHREPORTER_RESTART_ARG_0=path//to//some//executable, MOZ_CRASHREPORTER_RESTART_ARG_1=some_arg_1 etc., the crash reporter can be made to execute an arbitrary file present on the system with arbitrary command line arguments. However, user interaction is required, as this code path is only triggered once "Restart" is pressed by the user. Note that the attacker can confuse/convince the user to press the "Restart" button by abusing the MOZ_CRASHREPORTER_STRINGS_OVERRIDE environment variable, which allows the attacker to control the text (and thus the layout) of the crash reporter window.
• By abusing the MOZ_REPLACE_MALLOC_LIB environment variable. Setting MOZ_REPLACE_MALLOC_LIB=path//to//some//library will make the crash reporter process load the specified library on boot. This could be used in conjunction with other vulnerabilities (e.g. allowing the attacker to plant a malicious library file somewhere on the system), but it could also be used stand-alone (e.g. specifying a UNC share path on an SMB-enabled machine, or using a vulnerable library to trigger a vulnerability and continue from there). No interaction is required here.
• By abusing the SystemRoot environment variable. This is similar to the case above, in that SystemRoot can be hijacked to side-load DLLs whose full path is resolved using this variable. Certain dependencies of the crash reporter load their DLLs by using this environment variable (e.g. %SystemRoot%\system32\rpcrt4.dll), making it possible to abuse this behaviour.

Fix

To fix the issue, you need to ensure that mozwer doesn't trust the content process to be truthful about its process type, i.e. this information should come from somewhere unreachable to the content process. I'm not familiar with all the intrinsics of the underlying implementation, but querying the process' command line parameters (as is already being done in the child process crash code path) to determine its type would be a more suitable check, as the process cannot directly affect this.

Proof of Concept & Repro Steps

We assume that a content process has been fully compromised and can execute arbitrary code. The proof of concept consist of:

• The "payload" function that's executed from the content process and performs the steps necessary to trigger the sandbox escape:
  • Set gInProcessWerData.mProcessType to GeckoProcessType_Default to pose as a browser process to mozwer.
  • Setup the appropriate environment variables for code execution. Depending on the choice of option, one of the aforementioned methods is used. The first method is chosen by default, with calc.exe being the command line to execute.
  • Trigger a crash that needs to be handled OOP by WER. For example, we use a classic stack-based buffer overflow, which will be detected by the stack protector and throw out a stack corruption exception.
• A hook inside of the console.time() method implementation that's used to trigger the payload function via a crafted HTML file. This part is irrelevant to the vulnerability and the exploit and is used solely for demonstration purposes.

To reproduce the issue:

• Apply the attached vn0027.patch as a git patch in the root directory of firefox source.
  • git apply /path/to/vn0027.patch
• Build firefox.
• Ensure that local mozwer.dll has been registered as a WER exception handler module:
  • HKLM\SOFTWARE\Microsoft\Windows\Windows Error Reporting\RuntimeExceptionHelperModules contains a path/to/mozwer.dll DWORD entry with value 0.
• Ensure that crash reporter files/folders have been properly setup in %APPDATA%\Roaming\Mozilla\Firefox\Crash Reports:
  • There is an InstallTime file corresponding to the firefox build number/timestamp that was built earlier.
  • The pending folder exists.
• Grab the attached vn0027.html. Start a local http server in the directory where vn0027.html resides:
  • python3 -m http.server
• Open previously-built firefox and navigate to localhost:8000/vn0027.html.
• Observe that firefox shows that the tab crashed.
• Observe that crashreporter.exe is executed and the crash reporter window is shown. Observe that the process inherited environment variables from the content process corresponding to the html file that was opened earlier. (You can observe this via e.g. process explorer)
• Observe that pressing "Restart" spawns calc.exe (in case the first method is used for code exec), or a specified library has been loaded into crashreporter.exe (if using the second or third method for code exec).

NOTE: I couldn't get mozwer to work on Windows 10 Pro 22H2 (19045.3803), though it could be a local issue. During DLL loading, the process attempts to load user32.dll as one of the dependencies of mozwer.dll and this throws out a 0xC0000135 error. Everything works as expected on a clean latest Windows 11 insider build, so I recommend using that to test, should you run into any issues on your regular setup.

IMPORTANT

It is very important to mention that the impact of the vulnerability extends beyond firefox. Since mozwer.dll is registered as a WER exception handler module in the registry, any (non-firefox) sandboxed process on the system can use it via WerRegisterRuntimeExceptionModule("path/to/mozwer.dll", ...) and escape its sandbox using this vulnerability. This virtually means that having firefox installed on the system makes it possible to escape from any sandboxed process running on the system.

Let me know if any further information is required. I won't be dedicating more time to this issue, unless requested.

Comment 1

[Miloš]

Attachment: vn0027.html

Comment 2

[Daniel Veditz [:dveditz]]

Haven't validated this, but sec-high if true. Bob, can you take a look?

Comment 3

[Daniel Veditz [:dveditz]]

incidentally, "about:crashcontentprocess" will trigger a contentprocess crash in case that's useful in the future, as a separate step from your patch that sets up the exploit conditions.

Comment 4

[Bob Owen (:bobowen)]

(In reply to Daniel Veditz [:dveditz] Back Jan 8 2024 from comment #2)

Haven't validated this, but sec-high if true. Bob, can you take a look?

I'm having trouble validating this as I can't get the WER module to work at all.
I'll keep trying (including the Windows 11 insider build), but this might need to wait for gsvelto.
The attack looks plausible from inspection though.

Comment 5

[Miloš]

(In reply to Bob Owen (:bobowen) from comment #4)

(In reply to Daniel Veditz [:dveditz] Back Jan 8 2024 from comment #2)

Haven't validated this, but sec-high if true. Bob, can you take a look?

I'm having trouble validating this as I can't get the WER module to work at all.
I'll keep trying (including the Windows 11 insider build), but this might need to wait for gsvelto.
The attack looks plausible from inspection though.

I recommend double-checking if you properly setup the registry and the AppData folder (steps #3 and #4 in the report), these were the main obstacles I had to overcome to get my local module to work. Of course, this is all setup automatically on production builds during installation. Keep in mind that the name of the InstallTime{BUILD_NUMBER} file needs to be updated after every local build in order to reflect the build id (timestamp?) corresponding to the value in the executable. This was particularly problematic for me as I had to trace file accesses from Process Monitor to figure out what the value of the build number actually is.

If the issue persists even though everything is setup properly, it does seem plausible that you could be running into the same issue that I've encountered on my local W10.

Comment 6

[Bob Owen (:bobowen)]

(In reply to ynwarcs from comment #5)
...

I recommend double-checking if you properly setup the registry and the AppData folder (steps #3 and #4 in the report), these were the main obstacles I had to overcome to get my local module to work. Of course, this is all setup automatically on production builds during installation. Keep in mind that the name of the InstallTime{BUILD_NUMBER} file needs to be updated after every local build in order to reflect the build id (timestamp?) corresponding to the value in the executable. This was particularly problematic for me as I had to trace file accesses from Process Monitor to figure out what the value of the build number actually is.

Thanks, it is working when I fast fail the parent process (on a VM), so the general set up must be OK, but it's not working on the content process.
I don't even see it attempt to read the InstallTime file for the content crash.
I'll try the insider build.

Comment 7

[Bob Owen (:bobowen)]

WER reporting for content is still failing for me on an insider build (22635.2921).

It appears (from Yannis) that at least one of the things that is blocking this is that win32k lockdown is causing a failure in the WerFault process as it loads dependencies of mozwer.dll.
Presumably WerFault is picking up the win32k lockdown from the content process in some way.
I can get the PoC to work, but only if I disable win32k lockdown on the content process.
ynwarcs - is it possible that win32k lockdown is disabled on your insider build setup for some reason?
You can see this in the "Sandbox" section of about:support.

All but one of the WER reported content crashes that I've spot checked from the last two weeks, have win32k lockdown disabled for various reasons. They are all from older versions, with a handful from esr115.
The one with it apparently enabled has what appear to be some sort of security modules loaded from "Beijing Qihu Technology Co., Ltd.", which may be interfering with things.

All that being said we should not be relying on data read from the crashed process to make these decisions, so we need to fix that.
Other child processes that do not have win32k lockdown could be used, although I believe the content process would be the main worry.

I'm not sure the escape immediately allows arbitrary code execution outside the sandbox, but it would allow any existing application to be called with arbitrary command line parameters from the fake restart, so this could be part of a chain of attacks.
As I understand it, the other env vars mentioned would probably need other vulnerable files to be accessible from the machine already.
There are other env vars that could be problematic as well.

We think that using the command line for the process type could also be abused.
Perhaps we could look at the integrity and access token of the process.
We could write the PID of the main process to a file, but we might not want to do that really early in start-up. However we could just not allow restart if the crash was too early.

Comment 8

[Miloš]

(In reply to Bob Owen (:bobowen) from comment #7)

ynwarcs - is it possible that win32k lockdown is disabled on your insider build setup for some reason?
You can see this in the "Sandbox" section of about:support.

You're correct that win32k lockdown is disabled on my insider build:

• Win32k Lockdown State for Content Process = Win32k Lockdown disabled -- Missing Remote WebGL

In hindsight, I should've checked this before reporting rather than assuming that this is evidence that the failure on the W10 build was a local issue. Apologies for raising a (semi) false alarm with the vulnerability, then.

All but one of the WER reported content crashes that I've spot checked from the last two weeks, have win32k lockdown disabled for various reasons. They are all from older versions, with a handful from esr115.
The one with it apparently enabled has what appear to be some sort of security modules loaded from "Beijing Qihu Technology Co., Ltd.", which may be interfering with things.

I assumed that mozwer was intended to work with all firefox processes, mainly because I don't see another way of handling OOP-processed exceptions. Is this correct? Does this mean that you're missing crash reports from locked-down processes that would otherwise be handled by WER?

Either way, thanks for getting to the bottom of the issue. From my point of view, the fact that win32k lockdown prevents the bug from being reached significantly downgrades the severity of the bug. Though it should definitely be fixed before/if mozwer is enabled for locked-down processes.

We think that using the command line for the process type could also be abused.

Then you should most likely also change the child-process-crash code path to stop relying on the command line of the content process to execute code in the parent process. If you backtrace the wer_notify_proc parameter in this CreateRemoteThread call, you'll notice that it's read from the command line of the content process and used as the address of the function to be executed in the parent process.

Comment 9

[Bob Owen (:bobowen)]

(In reply to ynwarcs from comment #8)

(In reply to Bob Owen (:bobowen) from comment #7)

ynwarcs - is it possible that win32k lockdown is disabled on your insider build setup for some reason?
You can see this in the "Sandbox" section of about:support.

You're correct that win32k lockdown is disabled on my insider build:

• Win32k Lockdown State for Content Process = Win32k Lockdown disabled -- Missing Remote WebGL

In hindsight, I should've checked this before reporting rather than assuming that this is evidence that the failure on the W10 build was a local issue. Apologies for raising a (semi) false alarm with the vulnerability, then.

No apology required, this is still definitely a problem that we need to fix, but win32k lockdown on the content process is a fortunate mitigating factor.

All but one of the WER reported content crashes that I've spot checked from the last two weeks, have win32k lockdown disabled for various reasons. They are all from older versions, with a handful from esr115.
The one with it apparently enabled has what appear to be some sort of security modules loaded from "Beijing Qihu Technology Co., Ltd.", which may be interfering with things.

I assumed that mozwer was intended to work with all firefox processes, mainly because I don't see another way of handling OOP-processed exceptions. Is this correct? Does this mean that you're missing crash reports from locked-down processes that would otherwise be handled by WER?

Either way, thanks for getting to the bottom of the issue. From my point of view, the fact that win32k lockdown prevents the bug from being reached significantly downgrades the severity of the bug. Though it should definitely be fixed before/if mozwer is enabled for locked-down processes.

Yes, it does mean we're missing them and it's been good to discover a cause for this.
As you say, win32k lockdown is certainly significant as without that other sandbox escapes are probably available.

We think that using the command line for the process type could also be abused.

Then you should most likely also change the child-process-crash code path to stop relying on the command line of the content process to execute code in the parent process. If you backtrace the wer_notify_proc parameter in this CreateRemoteThread call, you'll notice that it's read from the command line of the content process and used as the address of the function to be executed in the parent process.

Thanks, yes we'll look at that as well.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Win32k Lockdown State for Content Process = Win32k Lockdown disabled -- Missing Remote WebGL

I've seen this before when e.g. running x86_64 Firefox on ARM under emulation, or other VM setups where the 3D drivers are too primitive. I guess reporter was analyzing the exploit in a VM.

If the attacker has no way to force us to end up in this fallback (e.g. by crashing the GPU driver), it's not too bad - and I do think we analyzed this at some point. But it'd be nice to not require this fallback path in a VM :-/

Comment 11

[Gabriele Svelto [:gsvelto]]

I've been looking at how to address this and there's two things that I believe must be done:

• Make mozwer use the new mozannotation crates to pull out the annotations. The mozannotation crates do not rely on external data to find the annotations, they look for them using the PE headers, and if something is messed up they just skip them (that is, it never trusts the data extracted from the crashed process). This removes the need for passing an address to mozwer to look things for that we'd need to trust.
• Make mozwer decide whether a process is the parent process only by examining it, possibly by using NtQueryInformationProcess(). The PPID for a process should come directly from the kernel and by examining it we should be able to tell if we're a child process or not. This would make sure that child processes can't abuse mozwer to run the crash reporter client.

Comment 12

[Andrew McCreight [:mccr8]]

I'll mark this sec-other because it sounds like it is blocked by the win32k lockdown but maybe that's not right.

Comment 13

[Gabriele Svelto [:gsvelto]]

Yes, we confirmed that win32k lockdown prevents the exploit from working, but the issue is real and I'm working on it.

Comment 14

[Gabriele Svelto [:gsvelto]]

I'm filing bugs to fix the various issues highlighted here, including the core problem of mistaking a child process for a main one. Since several changes are required I'm splitting them out in different bugs which I'll link here.

Comment 15

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1872920 - Change the way we identify processes in WER r=yjuglaret

Depends on D198699

Comment 16

[Gian-Carlo Pascutto [:gcp]]

it is blocked by the win32k lockdown but maybe that's not right

For most users, this is true. But there are reasonable use cases (using Firefox in a VM) where that won't be true.

Comment 17

[Bob Hood [:bhood]]

E.g., bug 1843782

Comment 18

[Ashley Hale [:ahale]]

I don't think bug 1847382 has much relation to this bug, as that requires a very different strategy to exploit (merely crashing the browser is easy with that one, but code execution in the VM guest is unproven and may be difficult, and then proceeding to execute code on the VM host is unproven and may also be difficult, as Microsoft has taken significant measures towards securing Windows graphics drivers - I don't doubt there are bugs, but they may be difficult to exploit).

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1872920 - Make the crash reporter client locate the executable by itself r=yjuglaret

Comment 20

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1872920 - Change the way we identify the parent process r=yjuglaret

Depends on D199352

Comment 21

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1872920 - Prevent the crash reporer from restarting Firefox when dealing with WER crashes r=yjuglaret

Comment 22

[Andrew McCreight [:mccr8]]

I'm clearing the flag, because it is a valid security issue, just not in the standard supported configuration.

Comment 23

[Gian-Carlo Pascutto [:gcp]]

just not in the standard supported configuration

I don't think we say that Firefox is "not fully supported" on VM anywhere. If we disclaim that it is, we should open a documentation bug.

Note that some competitions like pwn2own also use VM (though we did not reproduce this bug on the setup they use).

Comment 24

[Gian-Carlo Pascutto [:gcp]]

Reporter, can you clarify in what environment you originally reproduced this?

Comment 25

[Miloš]

Sure, it was a Windows 11 Insider 26020 Build running in a Hyper-V VM with Enhanced Session enabled. I couldn't reproduce it outside of the VM environment.

Comment 26

[Andrew McCreight [:mccr8]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #23)

I don't think we say that Firefox is "not fully supported" on VM anywhere. If we disclaim that it is, we should open a documentation bug.

Note that some competitions like pwn2own also use VM (though we did not reproduce this bug on the setup they use).

Ok. I don't really know what the status of supportedness is. If we are trying to support that, then that further suggests it shouldn't be sec-other like I had marked it.

Comment 27

[Daniel Veditz [:dveditz]]

I'm calling it sec-high again, because it does represent a sandbox escape. The fact that the win32k lockdown protects most people is a mitigation, but it doesn't protect all people nor all Firefox sandboxed processes. It's also not great that our handler exposes other applications to this risk.

Comment 28

[Gian-Carlo Pascutto [:gcp]]

Bug 1868737 (fixed in Firefox 123) made sure some GFX features keep working even if we can't identify the environment we're running under. VMWare supplies a GFX driver version, so it was not affected, but Parallels and Hyper-V apparently do not. Before the patch in bug 1868737, this would cause us to disable almost all GFX features in those VM, including some that are required for win32k lockdown.

Comment 29

[Bob Hood [:bhood]]

Gabriele, is the patch in comment 21 a viable solution for issue, or is it just a temporary mitigation? Now that you've flagged it as an S1, I'm curious if it will make it into Fx123.

Comment 30

[Pascal Chevrel:pascalc]

Gabriele, you set it as P1/S1, do we have reasons to believe that we should block the 123 release on the resolution of this sec bug? (Release Candidate builds today) Thanks

Comment 31

[Gabriele Svelto [:gsvelto]]

Sorry for the noise, I intended this to be P1/S2, not P1/S1. The patch in comment 21 is enough to fix the issue here but I'm trying to tighten up the interface between the child processes and the main process further, to avoid any potential future exploits on that end.

Comment 32

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1872920 - Change how we notify the main process when we intercept a crash via WER r=yjuglaret

Comment 33

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1872920 - Introduce the process_reader crate r=yjuglaret

Comment 34

[Gabriele Svelto [:gsvelto]]

Attachment: WIP: Bug 1872920 - Make mozwer use the process reader crate

Comment 35

[Gabriele Svelto [:gsvelto]]

Attachment: WIP: Bug 1872920 - Change how we write WER data

Comment 36

[Gabriele Svelto [:gsvelto]]

Try runs are green, local testing works fine and all patches are reviewed. I'm landing this today. Sorry it took a bit but we went for a multi-layer fix that tries to address all the potential issues, this entails:

1. The crash reporter client cannot launch arbitrary applications anymore, it can only launch the Firefox executable it came with
2. When the crash reporter is launched from the WER runtime exception module it will refuse to restart Firefox (or any other application) offering the user only to send the report and quit. We consider this an acceptable functional regression until we implement fully out-of-process crash generation
3. The runtime exception module does not read or rely on memory from the crashed process save for identifying child processes, all necessary information is retrieved from sources that cannot have been affected by the crashed process (such as the NT kernel and the application installation)
4. When identifying a child process the runtime exception module checks for its sandboxing status. This means that in a default configuration a child process "masquerading" as the parent process would not be identified as such and still treated as a child process
5. Because of 1 and 2 even if a child process in a non-standard configuration successfully masquerades as the parent process it won't be able to do anything besides launching the crash reporter client: it cannot affect it nor force it to launch another application anymore
6. Last but not least, when dealing with child process crashes the mechanism used to communicate with the parent process does not require any knowledge of the parent process layout. Communication is unidirectional and the address we jump into the parent process is now effectively hard-coded

Note that in the case of a default configuration with Win32k lockdown enabled the child process path still won't work as per the comments here. I want all these fixes to be in place before we make the WER runtime exception module compatible with a Win32k locked down environment.

Comment 37

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9376000 [details]
Bug 1872920 - Make the crash reporter client locate the executable by itself r=yjuglaret

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch set fixes one by one all the possible ways a compromised process could exploit the Windows Error Reporting runtime exception module, as such it does provide hints as what would need to be done to build an exploit.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All branches are affected
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: The largely affects things that happen after Firefox has crashed, as such it cannot really make things worse even in the case of regressions.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: No

Comment 38

[Tom Ritter [:tjr]]

Comment on attachment 9376000 [details]
Bug 1872920 - Make the crash reporter client locate the executable by itself r=yjuglaret

Approved to land and uplift if needed

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/694ae335f2b1
Make the crash reporter client locate the executable by itself r=yjuglaret,bobowen
https://hg.mozilla.org/integration/autoland/rev/cb217b8c1289
Change the way we identify the parent process r=yjuglaret,bobowen
https://hg.mozilla.org/integration/autoland/rev/cedd5c4eb7d5
Prevent the crash reporer from restarting Firefox when dealing with WER crashes r=yjuglaret,bobowen
https://hg.mozilla.org/integration/autoland/rev/a0c1596012ab
Introduce the process_reader crate r=yjuglaret,bobowen
https://hg.mozilla.org/integration/autoland/rev/0b4047d91582
Change how we notify the main process when we intercept a crash via WER r=yjuglaret,bobowen

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/694ae335f2b1
https://hg.mozilla.org/mozilla-central/rev/cb217b8c1289
https://hg.mozilla.org/mozilla-central/rev/cedd5c4eb7d5
https://hg.mozilla.org/mozilla-central/rev/a0c1596012ab
https://hg.mozilla.org/mozilla-central/rev/0b4047d91582

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:gsvelto, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox124 to wontfix.

For more information, please visit BugBot documentation.

Comment 42

[Gabriele Svelto [:gsvelto]]

The patch is working as expected, we get parent process crash reports as usual, we get GPU process crash reports which we weren't getting anymore and we're not getting content crashes which is also expected, since we'd get them only from users with Win32k lockdown disabled. Time for the uplifts.

Comment 43

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9376000 [details]
Bug 1872920 - Make the crash reporter client locate the executable by itself r=yjuglaret

Beta/Release Uplift Approval Request

• User impact if declined: The flaw allows an attacker who has obtained control of a content process to escape the sandbox and execute applications of its own choosing on the user machine (this affects a significant subset of users but not all of them). Additionally an attacker could leverage our Windows Error Reporting runtime exception module to attack other applications that have sandboxed processes exposed to the web.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1874235
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changes happen mostly in code paths that are executed after Firefox has already crashed. As such, the chance of introducing visible regressions is rather small.
• String changes made/needed: None
• Is Android affected?: No

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: See above
• Fix Landed on Version: 125
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): See above

Comment 44

[Gabriele Svelto [:gsvelto]]

Not all the flags have been set correctly but the approval request applies to all the patches and also to bug 1874235 which is also required though it only fixes functional changes but does not affect the security issue here.

Comment 45

[Dianna Smith [:diannaS]]

Comment on attachment 9376000 [details]
Bug 1872920 - Make the crash reporter client locate the executable by itself r=yjuglaret

Approved for 124.0b4

Comment 46

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/74c15d8edc87
https://hg.mozilla.org/releases/mozilla-beta/rev/ff1674ed959e
https://hg.mozilla.org/releases/mozilla-beta/rev/50a7cd68ca32
https://hg.mozilla.org/releases/mozilla-beta/rev/079865992447
https://hg.mozilla.org/releases/mozilla-beta/rev/3548868cfa54

Comment 47

[Dianna Smith [:diannaS]]

:gsvelto looks like the dependency bug 1874235 doesnt graft cleanly to esr (i think it is missing bug 1875097). let me know what you would like me to do

Comment 48

[Gabriele Svelto [:gsvelto]]

As we discussed I need to rework this patches without bug 1875097 to land them on ESR. Unfortunately I was too quick about landing that because it made my life simpler, but I forgot that it would be impossible to uplift it.

Comment 49

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1872920 - Make the crash reporter client locate the executable by itself

Original Revision: https://phabricator.services.mozilla.com/D199352

Comment 50

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1872920 - Change the way we identify the parent process

Original Revision: https://phabricator.services.mozilla.com/D199353

Comment 51

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1872920 - Prevent the crash reporer from restarting Firefox when dealing with WER crashes

Original Revision: https://phabricator.services.mozilla.com/D199856

Comment 52

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1872920 - Introduce the process_reader crate

Original Revision: https://phabricator.services.mozilla.com/D201590

Comment 53

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1872920 - Change how we notify the main process when we intercept a crash via WER

This patch makes several fundamental changes to the logic we use to inform
the main process whenever the WER runtime exception module intercepts a child
process crash:

• We no longer read the process type or any other data from the child process;
  the process type is passed as the runtime exception module's context
• We no longer read the address of the memory area used to communicate with the
  main process from the child process arguments. Instead we allocate memory
  directly into the main process and store the required information there
• We don't read anything from the main process either, the pointer to the
  function used to notify the main process is now found by looking out its
  dedicated section in the parent process' xul.dll mapping
• We no longer read the OOM crash annotation from a child process, this
  functionality will be restored by making the module use the mozannotation
  crates to fetch all the annotations

Original Revision: https://phabricator.services.mozilla.com/D201589

Comment 54

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9388000 [details]
Bug 1872920 - Make the crash reporter client locate the executable by itself

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: See comment 43
• User impact if declined: See comment 43
• Fix Landed on Version: 125
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The alternative would be to remove the WER runtime exception module entirely from ESR; but it would blind us to the type of crashes it catches.

Comment 55

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/48f187ce1d98
https://hg.mozilla.org/releases/mozilla-esr115/rev/d63417193b5f
https://hg.mozilla.org/releases/mozilla-esr115/rev/c507da671b49
https://hg.mozilla.org/releases/mozilla-esr115/rev/a89e97918b1b
https://hg.mozilla.org/releases/mozilla-esr115/rev/1247a19d2f5b

Comment 56

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1872920 - Fix building mozwer-rust for rust version 1.69.0

Comment 57

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9388229 [details]
Bug 1872920 - Fix building mozwer-rust for rust version 1.69.0

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes the build regression introduced with the other uplifts
• User impact if declined: We can't build Firefox :-(
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):

Comment 58

[Phabricator Automation]

Uplift Approval Request

• String changes made/needed: None
• Explanation of risk level: Low
• Needs manual QE test: no
• Risk associated with taking this patch: Low
• Fix verified in Nightly: no
• User impact if declined: See ticket
• Is Android affected?: no
• Steps to reproduce for manual QE testing: N/A
• Code covered by automated testing: no

Comment 59

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/332453ecfde5

Comment 60

[Tyson Smith [:tsmith]]

Attachment: advisory.txt

Comment 61

[Miloš]

Could the advisory be changed to credit "goodbyeselene"? I realize I've forgotten to include this information in the original post, apologies for that. If not, it's not a big deal. Thanks!

Comment 62

[Tom Ritter [:tjr]]

(In reply to Miloš from comment #61)

Could the advisory be changed to credit "goodbyeselene"? I realize I've forgotten to include this information in the original post, apologies for that. If not, it's not a big deal. Thanks!

Yes, I will update the advisory