CommandLineParserWin::HandleCommandLine() (toolkit\xre\CmdLineAndEnvUtils.h) reads beyond bounds when fed an unterminated string via a malformed WM_COPYDATA message. This message can be sent by a hostile program running in a different -- even a less-privileged -- account context than the victim instance of FF. [1]

The read beyond bounds could cause FF to navigate to a page that could injure the user, such as if the memory beyond the end of the string contains a site's access token. This is unlikely, but not inconceivable. [2]

Attached is a POC that demonstrates the bug. Use it thusly:

1. Create a VS console project and replace the VS-generated sample program with the POC. Build it.
2. Find a throwaway VM.
3. Ensure that no instances of FF are currently running on the system.
4. Run FF 88.0 using the default profile, from an elevated admin account cmd prompt.
5. Notice that FF runs in the admin account's context, but (fortunately!) disables the admin token.
6. Attach a debugger to FF's chrome process.
7. Set a BP on CommandLineParserWin::HandleCommandLine().
8. Run the POC in a different, nonprivileged account than the one in which you ran FF.
9. When the BP fires, examine the aCmdLineString argument and notice that it begins with 8 bytes of 'a' (which is what the POC sent). There might or might not be an accidental "terminating" 0 following the last 'a'. Step the code and notice that it reads at least one byte beyond the 8 that the POC sent. Depending on your OS and memory layout, FF might crash attempting to read a no-access page.
10. Rebuild the POC with BYTE_COUNT == 128*1024. Rerun the above sequence and notice that Windows puts the WM_COPYDATA message on the heap instead of the stack, potentially allowing longer reads beyond bounds.

The bug is in WinRemoteMessageReceiver::Parse() (toolkit/components/remote/winremotemessage.cpp), which assumes that aMessageData->lpData is null-terminated. It should instead use the count from aMessageData->cbData, which contains the exact number of bytes that Windows copied from the sender's context into FF's context.

[1] I will soon post a bug documenting other ways in which WM_COPYDATA can be accessed and abused or accidentally invoked, such as by a different instance of FF running in a different account's context. Probably other Windows messages can be abused similarly. I don't know how much FF can do to protect itself from these others.

[2] Windows copies small WM_COPYDATA messages onto the stack, the relevant portions of which contain local variables and stack-frame data related to handling the WM_COPYDATA message. Windows copies large WM_COPYDATA messages onto the heap, sometimes followed by a no-access page.

Toshi, is this something you could investigate? Thanks.

The reporter is right. WinRemoteMessageReceiver::Parse should handle the case where COPYDATASTRUCT::lpData is not null-terminated. I could reproduce a simple crash caused by an inaccessible page just after the received buffer.

It looks like this code was added in bug 1650637, which landed in Firefox 81.

It's not regressed by bug 1650637. My patch just moved the logic. The logic itself is very old. I can see it existed even before v67.

[1] I will soon post a bug documenting other ways in which WM_COPYDATA can be accessed and abused or accidentally invoked, such as by a different instance of FF running in a different account's context. Probably other Windows messages can be abused similarly. I don't know how much FF can do to protect itself from these others.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1706759 .

Make CommandLineParserWin::HandleCommandLine take nsTSubstring. r=mossop
https://hg.mozilla.org/integration/autoland/rev/ec09f17201b4ca9b609386d94cafe7f4733fea84
https://hg.mozilla.org/mozilla-central/rev/ec09f17201b4

The patch landed in nightly and beta is affected.
:toshi, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment on attachment 9217600 [details]
Bug 1706501 - Make CommandLineParserWin::HandleCommandLine take nsTSubstring. r=mossop

Beta/Release Uplift Approval Request

• User impact if declined: The worst case is if an attacker could craft stack or heap nicely, they might open a malicious URL by sending a window message from an external program. Or, a random window message can easily cause crash, that should not happen.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix is basically to add a boundary check in string buffer operation. It's simple and easily understood. Since this is a security bug, I didn't include a testcase for this patch, but I already wrote a test case and verified the fix with it. It will be landed separately later (bug bug 1708741).
• String changes made/needed:

While the overread is clearly wrong, it's not clear to us (from a bounty-awarding perspective) that the data being read is part of what the attacker sends. MS will copy the remote message correctly, it's the receiving Firefox that gets it wrong and reads into it's own heap.

Also, local-attacker is generally not part of our threat model, with an exception for the admin-privileged maintenance service because that's a concern for Enterprise folks.

Toshi, could you also request an uplift to ESR? Thanks

Comment on attachment 9221012 [details]
Bug 1706501 - Make HandleCommandLine take nsACString

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The possible exploit scenario is a local-attacker situation, but as described below, an external program can easily cause Firefox to crash by sending a window message, that should not be possible in ESR version, too.
• User impact if declined: The worst case is if an attacker could craft stack or heap nicely, they might open a malicious URL by sending a window message from an external program. Or, a random window message can easily cause crash, that should not happen.
• Fix Landed on Version: 90
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix is basically to add a boundary check in string buffer operation. It's simple and easily understood.
• String or UUID changes made by this patch: None

Comment on attachment 9217600 [details]
Bug 1706501 - Make CommandLineParserWin::HandleCommandLine take nsTSubstring. r=mossop

Approved for 89 beta 11, thanks.

https://hg.mozilla.org/releases/mozilla-beta/rev/46213ee42ebb

Comment on attachment 9221012 [details]
Bug 1706501 - Make HandleCommandLine take nsACString

Approved for 78.11esr, thanks.

https://hg.mozilla.org/releases/mozilla-esr78/rev/80e97d09f422