Closed Bug 1451308 Opened 3 years ago Closed 2 years ago

Crash in nsClipboard::GetNativeDataOffClipboard


(Core :: Widget: Win32, defect)

58 Branch
Not set



Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- fixed
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox64 --- wontfix
firefox65 --- fixed
firefox66 --- fixed


(Reporter: philipp, Unassigned)



(4 keywords, Whiteboard: [adv-main65-])

Crash Data


(1 obsolete file)

This bug was filed from the Socorro interface and is
report bp-9656c64a-571d-44f6-920c-13a700180404.

Top 10 frames of crashing thread:

0 xul.dll nsClipboard::GetNativeDataOffClipboard widget/windows/nsClipboard.cpp:560
1 xul.dll nsClipboard::GetDataFromDataObject widget/windows/nsClipboard.cpp:627
2 xul.dll nsClipboard::GetNativeClipboardData widget/windows/nsClipboard.cpp:1002
3 xul.dll nsBaseClipboard::GetData widget/nsBaseClipboard.cpp:77
4 xul.dll mozilla::dom::ContentParent::RecvGetClipboard dom/ipc/ContentParent.cpp:2648
5 xul.dll mozilla::dom::PContentParent::OnMessageReceived ipc/ipdl/PContentParent.cpp:8024
6 xul.dll mozilla::ipc::MessageChannel::DispatchSyncMessage ipc/glue/MessageChannel.cpp:2101
7 xul.dll mozilla::ipc::MessageChannel::DispatchMessageW ipc/glue/MessageChannel.cpp:2059
8 xul.dll mozilla::ipc::MessageChannel::RunMessage ipc/glue/MessageChannel.cpp:1909
9 xul.dll mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:1942


these crash reports on windows seem to have become a bit more common after firefox 58 - overall they are still fairly low in volume though.
Looks like maybe we're getting non-null-terminated data and the strlen is running off the end? If we found a random null in our own memory before hitting an access violation we might then leak memory contents.
Group: core-security → layout-core-security
I'm not sure I understand why we're taking a strlen of |aData|, GetGlobalData writes a length for the value to a return-argument.
Duplicate of this bug: 1461739
bug 1461739 describes some steps that led to the crash.

35% of the crashes come from zh-cn builds, so perhaps there's content that's more prone to crashing there.
GetGlobalData always returns a null-terminated buffer though:
I'm guessing from the code comments that the intention of the NS_strlen
is to catch any embedded NULs in the actual data preceding that?
(I don't know why that's desirable when we don't know what kind of
data we're dealing with though.  It seems bogus if it's some kind
of binary data for example.)
In theory, GlobalSize could return an odd number and then we would
write the two zero bytes misaligned to the start when interpreted
as char16_t so that NS_strlen doesn't find it.

A possible fix for that would be to allocate/write three extra
zeroed bytes when allocSize is odd, instead of just two.

Alternatively, as Alex suggests, replace the NS_strlen call with
"*aLen = allocLen;".

Unrelated issue, but we might want to fix it while we're here:
if malloc() fails then we miss to call GlobalUnlock(aHGBL):
With the ASAN crash in hand, we now that the issue is GlobalSize returning an odd number: "0x11ede48c83e3 is located 0 bytes to the right of 419-byte region [0x11ede48c8240,0x11ede48c83e3)".

Simply padding by 3 zero bytes, instead of 2 seems like the easiest fix.
(ASAN crash is from bug 1513201)
Duplicate of this bug: 1513201
Duping this to the ASan bug after talking Alex. According to him, the ASan trace provided missing information to fix this issue properly and he would like to make it possible for a small bounty to be considered for that reason. The patch will move over to bug 1513201 as well.
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1513201
Attachment #9030459 - Attachment is obsolete: true
Whiteboard: [adv-main65-]
Group: layout-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.