1761481 - Crash in [@ ConvertUnknownBreaks<T>]

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

Crash report: https://crash-stats.mozilla.org/report/index/cba74df2-84f7-40cd-a27f-f92390211013

Reason: EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE

Top 10 frames of crashing thread:

0 XUL char16_t* ConvertUnknownBreaks<char16_t> xpcom/io/nsLinebreakConverter.cpp:207
1 XUL nsLinebreakConverter::ConvertUnicharLineBreaksInSitu xpcom/io/nsLinebreakConverter.cpp:398
2 XUL nsLinebreakHelpers::ConvertPlatformToDOMLinebreaks widget/nsPrimitiveHelpers.cpp:183
3 XUL nsClipboard::TransferableFromPasteboard widget/cocoa/nsClipboard.mm:200
4 XUL nsClipboard::GetNativeClipboardData widget/cocoa/nsClipboard.mm:359
5 XUL nsClipboard::GetData widget/cocoa/nsClipboard.mm:757
6 XUL NS_InvokeByIndex 
7 XUL XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1130
8 XUL XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:921
9  @0x34fe8b0a83e6 

This is an out-of-bounds access detected by PHC. The allocation stack for the buffer that was overflown is the following:

#0    nsClipboard::GetNativeClipboardData(nsITransferable*, int) (XUL)
#1    nsClipboard::GetData(nsITransferable*, int) (XUL)
#2    NS_InvokeByIndex (XUL)
#3    XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XUL)
#4    XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XUL)
#5    _mh_execute_header (firefox)
#6    _mh_execute_header (firefox)
#7    _mh_execute_header (firefox)
#8    _mh_execute_header (firefox)
#9    _mh_execute_header (firefox)
#10    _mh_execute_header (firefox)
#11    _mh_execute_header (firefox)
#12    js::jit::MaybeEnterJit(JSContext*, js::RunState&) (XUL)
#13    js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) (XUL)
#14    JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (XUL)
#15    nsXPCWrappedJS::CallMethod(unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) (XUL)

This crash seems to happen mostly on macOS but I'm not sure if it's entirely macOS-specific or not. There's a few instances on Windows but with a different stack, like crash 95890c85-bfde-431f-a6e6-77a010220301. I don't know this code well enough so it might be that the code at the top of the stack is not checking the bounds correctly or it might be possible that the calling code allocated an array that's too small.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: Bug 1761481: Improve the conversion of line breaks. r=mstange

Comment 2

[Stephen A Pohl [:spohl]]

Comment on attachment 9269464 [details]
Bug 1761481: Improve the conversion of line breaks. r=mstange

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch is an obvious fix to an off-by-one mistake when checking the end of an array. The mitigating factor here is that this code is used by our drag and clipboard code, which should make it more difficult to craft an exploit. It is unclear how difficult it would be to deliberately craft clipboard data that would trigger this crash, or if it is possible at all.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all
• 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?: This should apply cleanly everywhere since this code goes back to the beginning of times.
• How likely is this patch to cause regressions; how much testing does it need?: Highly unlikely to cause regressions. I will be writing a gtest in a separate bug where I will also be cleaning up some of this old code.

Comment 3

[Andrew McCreight [:mccr8]]

Thanks for fixing this, Stephen. I'll move this to XPCOM as it looks like the issue is in ConvertUnknownBreaks.

Somebody should really rewrite this code so it isn't using raw arrays...

Comment 4

[Andrew McCreight [:mccr8]]

This code is very old, plus I found a crash from ESR that looks similar, so I'm marking all branches as affected: bp-f1582170-823f-4f28-bb3f-815550220329

I'm not entirely sure what the rating is. It does require some user interaction, but getting somebody to copy and paste isn't the hardest thing. On the third hand, it looks like the issue is just reading one byte past the end of an array, so maybe that's not very exploitable anyways.

Comment 5

[Daniel Veditz [:dveditz]]

There's only a problem if aInSrc ends with a CR. If it does we'll read one off the end and one of the following happens:

• the program crashes due to an access violation as in comment 4, or because PHC slaps us down as in comment 0. Not exploitable
• the next byte after the array is NOT a nsCRT::LF -- other than the one-byte read it works just as it should. Not exploitable
• the next byte happens to look like nsCRT::LF -- other than the one-byte read (twice) it works just as before. We do the exact same thing for CRLF as we do for the lone CR at the end. Not exploitable

An odd case might be if the next byte is part of some other allocation and changes on another thread between lines 207 and 236. In all combinations we malloc the same size buffer and write the same AppendLinebreak(dst, aDestBreak); whether we think the buffer ends in CR or CRLF. Should be OK to unhide this bug.

It's cool to see PHC catching evil memory access in action.

Comment 6

[Stephen A Pohl [:spohl]]

(In reply to Daniel Veditz [:dveditz] from comment #5)

[...]

Daniel, did you mean to set the sec-approval flag on the patch here? Or will this occur at a different point in time?

Comment 7

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9269464 [details]
Bug 1761481: Improve the conversion of line breaks. r=mstange

This got rated sec-other so clearing flag

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Improve the conversion of line breaks. r=mstange
https://hg.mozilla.org/integration/autoland/rev/063c85bcefeb4b898f38557f35bab127a8e32ea0
https://hg.mozilla.org/mozilla-central/rev/063c85bcefeb

Comment 9

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

The patch landed in nightly and beta is affected.
:spohl, 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 10

[Stephen A Pohl [:spohl]]

Comment on attachment 9269464 [details]
Bug 1761481: Improve the conversion of line breaks. r=mstange

Beta/Release Uplift Approval Request

• User impact if declined: Possibility of crashes when dragging text.
• 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: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch simply corrects a bounds check for the end of a buffer.
• String changes made/needed: none

Comment 11

[Dianna Smith [:diannaS]]

Comment on attachment 9269464 [details]
Bug 1761481: Improve the conversion of line breaks. r=mstange

Approved for 100.0b5

Comment 12

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f870ff835382