1663452 - AddressSanitizer: heap-use-after-free [@ __asan_memcpy] with READ of size 496 through [@ JSStructuredCloneData::ReadBytes] involving SessionHistoryEntry over IPC

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 82.0a1-20200906094118-https://hg.mozilla.org/mozilla-central/rev/38df8f36910ce8bcc5609841908a52d9c3e6da37.

For detailed crash information, see attachment.

This looks related to SessionHistory and IPC, not sure which component is best suited here.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 3

[Christian Holler (:decoder)]

There's another use-after-free crash signature here that's very likely the same, involving SessionHistoryEntry.

Comment 4

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 5

[Andrew McCreight [:mccr8]]

This looks interesting. I'm doing an initial triage.

Comment 6

[Andrew McCreight [:mccr8]]

Geeknik, do you have fission.sessionHistoryInParent set to true?

Comment 7

[Andrew McCreight [:mccr8]]

If the line numbers for PContentParent::OnMessageReceived match up to current m-c, then the message is SetActiveSessionHistoryEntryForFrame.

It looks like that message was added in bug 1649131, which landed not too long ago. Maybe fission.sessionHistoryInParent is set?

The structured clone container is created in IPDLParamTraits<dom::SessionHistoryInfo>::Read():

if (stateData.isSome()) {
    aResult->mStateData = new nsStructuredCloneContainer();
    if (aActor->GetSide() == ChildSide) {
      UnpackClonedMessageDataForChild(stateData.ref(), *aResult->mStateData);
    } else {
      UnpackClonedMessageDataForParent(stateData.ref(), *aResult->mStateData);
    }
  }

The UnpackClonedMessageDataFor* methods call BorrowFromClonedMessageDataFor*, not CopyFromClonedMessageDataFor*. Because it is a borrow, it only gets a non-owning reference to the underlying message data.

CanonicalBrowsingContext::SetActiveSessionHistoryEntryForFrame() does:
RefPtr<SessionHistoryEntry> newEntry = new SessionHistoryEntry(aInfo);

The ctor for that does this:

SessionHistoryEntry::SessionHistoryEntry(SessionHistoryInfo* aInfo)
    : mInfo(MakeUnique<SessionHistoryInfo>(*aInfo)), mID(++gEntryID) {}

Which I think ends up invoking the default constructor: SessionHistoryInfo(const SessionHistoryInfo& aInfo) = default;

This adds a new reference to the nsStructuredCloneContainer: RefPtr<nsStructuredCloneContainer> mStateData;

However, because the clone container was set with a borrow and not a copy, it will escape the scope of the message data and we get a UAF.

Comment 8

[Andrew McCreight [:mccr8]]

It seems dangerous that a refcounted class like nsStructuredCloneContainer is allowed to get a borrowed reference to IPC data.

Comment 9

[Brian Carpenter [:geeknik]]

(In reply to Andrew McCreight [:mccr8] from comment #6)

Geeknik, do you have fission.sessionHistoryInParent set to true?

It looks like that pref was true when this crash occurred.

Comment 10

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

Set release status flags based on info from the regressing bug 1649131

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to Andrew McCreight [:mccr8] from comment #7)

The UnpackClonedMessageDataFor* methods call BorrowFromClonedMessageDataFor*, not CopyFromClonedMessageDataFor*. Because it is a borrow, it only gets a non-owning reference to the underlying message data.

We could just bypass UnpackClonedMessageDataFor* and call CopyFromClonedMessageDataFor* directly (or can we maybe use StealFromClonedMessageDataFor*?).

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1663452, steal structured clone data for pushState, r=peterv

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

something like
if (aActor->GetSide() == ChildSide) {

•  UnpackClonedMessageDataForChild(stateData.ref(), *aResult->mStateData);
  

•  aResult->mStateData->CopyFromClonedMessageDataForChild(stateData.ref());
  

  } else {

•  UnpackClonedMessageDataForParent(stateData.ref(), *aResult->mStateData);
  

•  aResult->mStateData->CopyFromClonedMessageDataForParent(stateData.ref());
  

  }

(did test that yet)

Comment 14

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

https://hg.mozilla.org/integration/autoland/rev/8bd2cae0fa5a579fc2ba6fe9d877960312295223
https://hg.mozilla.org/mozilla-central/rev/8bd2cae0fa5a

Comment 15

[Brian Carpenter [:geeknik]]

Could this possibly be related or is this something new? Firefox (20200913213803) isn't generating an ASan stack, however when playing with iframes, I've triggered this a few times, haven't triggered the UAF though:

Assertion failure: mHistory, at /builds/worker/checkouts/gecko/docshell/shistory/ChildSHistory.cpp:198
#01: ???[/home/geeknik/firefox/libxul.so +0x11af1e65]
#02: ???[/home/geeknik/firefox/libxul.so +0x97d9726]
#03: ???[/home/geeknik/firefox/libxul.so +0xa033ddf]
#04: ???[/home/geeknik/firefox/libxul.so +0xbcdafe2]
#05: ???[/home/geeknik/firefox/libxul.so +0x12a0265d]
#06: ???[/home/geeknik/firefox/libxul.so +0x129ef976]
#07: ???[/home/geeknik/firefox/libxul.so +0x129d235d]
#08: ???[/home/geeknik/firefox/libxul.so +0x12a02f61]
#09: ???[/home/geeknik/firefox/libxul.so +0x12a0528a]
#10: ???[/home/geeknik/firefox/libxul.so +0x12ba479c]
#11: ???[/home/geeknik/firefox/libxul.so +0xa1e3062]
#12: ???[/home/geeknik/firefox/libxul.so +0xdf09938]
#13: ???[/home/geeknik/firefox/libxul.so +0xdf0a152]
#14: ???[/home/geeknik/firefox/libxul.so +0xdf10a47]
#15: ???[/home/geeknik/firefox/libxul.so +0xdeefa5d]
#16: ???[/home/geeknik/firefox/libxul.so +0x7976881]
#17: ???[/home/geeknik/firefox/libxul.so +0x7285d4b]
#18: ???[/home/geeknik/firefox/libxul.so +0x7086289]
#19: ???[/home/geeknik/firefox/libxul.so +0x7082943]
#20: ???[/home/geeknik/firefox/libxul.so +0x70845eb]
#21: ???[/home/geeknik/firefox/libxul.so +0x7084c2e]
#22: ???[/home/geeknik/firefox/libxul.so +0x5dab3c8]
#23: ???[/home/geeknik/firefox/libxul.so +0x5da6875]
#24: ???[/home/geeknik/firefox/libxul.so +0x5da42d6]
#25: ???[/home/geeknik/firefox/libxul.so +0x5da4868]
#26: ???[/home/geeknik/firefox/libxul.so +0x5db26e5]
#27: ???[/home/geeknik/firefox/libxul.so +0x5dd4df8]
#28: ???[/home/geeknik/firefox/libxul.so +0x5ddec52]
#29: ???[/home/geeknik/firefox/libxul.so +0x708dbdd]
#30: ???[/home/geeknik/firefox/libxul.so +0x6fa6a63]
#31: ???[/home/geeknik/firefox/libxul.so +0xe7fa8ab]
#32: ???[/home/geeknik/firefox/libxul.so +0x1277bd30]
#33: ???[/home/geeknik/firefox/libxul.so +0x6fa6a63]
#34: ???[/home/geeknik/firefox/libxul.so +0x1277b5c2]
#35: ???[/home/geeknik/firefox/firefox-bin +0x15f979]
#36: __libc_start_main[/lib64/libc.so.6 +0x271a3]
#37: ???[/home/geeknik/firefox/firefox-bin +0xb2859]
#38: ??? (???:???)
AddressSanitizer:DEADLYSIGNAL

Comment 16

[Peter Van der Beken [:peterv]]

(In reply to Brian Carpenter [:geeknik] from comment #15)

Could this possibly be related or is this something new? Firefox (20200913213803) isn't generating an ASan stack, however when playing with iframes, I've triggered this a few times, haven't triggered the UAF though:

Assertion failure: mHistory, at /builds/worker/checkouts/gecko/docshell/shistory/ChildSHistory.cpp:198

Is this with the fission.sessionHistoryInParent set to true? If so, at this point there are a number of known crashes that should be mostly fixed by the time we turn it on by default. And note that at first it will only be enabled when fission is enabled.

Comment 17

[Brian Carpenter [:geeknik]]

Yes, it is enabled, but I was jusut concerned we replaced one crash with another. I'll ignore it.

Comment 18

[Peter Van der Beken [:peterv]]

(In reply to Brian Carpenter [:geeknik] from comment #17)

Yes, it is enabled, but I was jusut concerned we replaced one crash with another. I'll ignore it.

By next week we should turn it on by default when fission is enabled. If you test with fission we're definitely interested in crashes, and we'll get reports for them for session history in parent starting then too.

Thanks for the report!

Comment 19

[Daniel Veditz [:dveditz]]

Technically this pref setting was not valid for the bounty program when this was filed (indeed, still isn't), but since we're close to enabling it (comment 18) as a normal part of fission and may not have found it ourselves by then we did want to at least award a partial bounty for it.

Comment 20

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

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.