1847529 - AddressSanitizer: stack-buffer-underflow [@ __asan_memcpy] with READ of size 16781312 with potentially corrupted FontEntry

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Christian Holler (:decoder)]

In experimental IPC fuzzing, we found the following crash on mozilla-central revision f14ed3bab724+ (fuzzing-asan-nyx-opt build):

=================================================================
==1733==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7fff1ae2fd40 at pc 0x5555557c3a5d bp 0x7ffffffc0a00 sp 0x7ffffffc01c8
READ of size 16781312 at 0x7fff1ae2fd40 thread T0
    #0 0x5555557c3a5c in __asan_memcpy _asan_rtl_:3
    #1 0x7fffc9977f40 in nsCharTraits<char>::copy(char*, char const*, unsigned long) xpcom/string/nsCharTraits.h:314:9
    #2 0x7fffc9977d11 in nsTSubstring<char>::Assign(char const*, unsigned long, std::nothrow_t const&) xpcom/string/nsTSubstring.cpp:440:3
    #3 0x7fffc9965ab3 in nsTSubstring<char>::Assign(char const*, unsigned long) xpcom/string/nsTSubstring.cpp:408:7
    #4 0x7fffc9965a4a in nsTString<char>::nsTString(char const*, unsigned long) xpcom/string/nsTString.h:75:11
    #5 0x7fffceeaacfd in mozilla::fontlist::String::AsString(mozilla::fontlist::FontList*) const gfx/thebes/SharedFontList.h:114:12
    #6 0x7fffceeaaac6 in gfxFcPlatformFontList::CreateFontEntry(mozilla::fontlist::Face*, mozilla::fontlist::Family const*) gfx/thebes/gfxFcPlatformFontList.cpp:2037:41
    #7 0x7fffcf258145 in gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0::operator()() const gfx/thebes/gfxPlatformFontList.cpp:1795:40
    #8 0x7fffcf257ecf in RefPtr<gfxFontEntry>& nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::EntryHandle::OrInsertWith<gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0>(gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0&&) objdir-ff-aflpp/dist/include/nsBaseHashtable.h:726:23
    #9 0x7fffcf257d45 in RefPtr<gfxFontEntry>& nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::LookupOrInsertWith<gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0>(mozilla::fontlist::Face* const&, gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0&&)::{lambda(auto:1)#1}::operator()<nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::EntryHandle>(nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::EntryHandle) const objdir-ff-aflpp/dist/include/nsBaseHashtable.h:423:26
    #10 0x7fffcf257bb1 in decltype(auto) nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::WithEntryHandle<nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::LookupOrInsertWith<gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0>(mozilla::fontlist::Face* const&, gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0&&)::{lambda(auto:1)#1}>(mozilla::fontlist::Face*, gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0&&)::{lambda(auto:1)#1}::operator()<nsTHashtable<nsBaseHashtableET<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry> > >::EntryHandle>(nsTHashtable<nsBaseHashtableET<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry> > >::EntryHandle) const objdir-ff-aflpp/dist/include/nsBaseHashtable.h:836:18
    #11 0x7fffcf2579f6 in decltype(auto) nsTHashtable<nsBaseHashtableET<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry> > >::WithEntryHandle<nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::WithEntryHandle<nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::LookupOrInsertWith<gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0>(mozilla::fontlist::Face* const&, gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0&&)::{lambda(auto:1)#1}>(mozilla::fontlist::Face*, gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0&&)::{lambda(auto:1)#1}>(mozilla::fontlist::Face*, gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0&&)::{lambda(auto:1)#1}::operator()<PLDHashTable::EntryHandle>(PLDHashTable::EntryHandle) const objdir-ff-aflpp/dist/include/nsTHashtable.h:437:18
    #12 0x7fffcf2577c4 in _ZN12PLDHashTable15WithEntryHandleIZN12nsTHashtableI17nsBaseHashtableETI12nsPtrHashKeyIN7mozilla8fontlist4FaceEE6RefPtrI12gfxFontEntryEEE15WithEntryHandleIZN15nsBaseHashtableIS7_SA_PS9_18nsDefaultConverterISA_SF_EE15WithEntryHandleIZNSI_18LookupOrInsertWithIZN19gfxPlatformFontList26GetOrCreateFontEntryLockedEPS6_PKNS5_6FamilyEE3$_0EERSA_RKSM_OT_EUlSU_E_EENSt13invoke_resultISU_JONSI_11EntryHandleEEE4typeESM_SV_EUlSU_E_EENSX_ISU_JONSC_11EntryHandleEEE4typeESM_SV_EUlSU_E_EENSX_ISU_JONS_11EntryHandleEEE4typeEPKvSV_ objdir-ff-aflpp/dist/include/PLDHashTable.h:605:12
    #13 0x7fffcf257638 in _ZN12nsTHashtableI17nsBaseHashtableETI12nsPtrHashKeyIN7mozilla8fontlist4FaceEE6RefPtrI12gfxFontEntryEEE15WithEntryHandleIZN15nsBaseHashtableIS5_S8_PS7_18nsDefaultConverterIS8_SD_EE15WithEntryHandleIZNSG_18LookupOrInsertWithIZN19gfxPlatformFontList26GetOrCreateFontEntryLockedEPS4_PKNS3_6FamilyEE3$_0EERS8_RKSK_OT_EUlSS_E_EENSt13invoke_resultISS_JONSG_11EntryHandleEEE4typeESK_ST_EUlSS_E_EENSV_ISS_JONSA_11EntryHandleEEE4typeESK_ST_ objdir-ff-aflpp/dist/include/nsTHashtable.h:434:25
    #14 0x7fffcf2573f7 in std::invoke_result<nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::LookupOrInsertWith<gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0>(mozilla::fontlist::Face* const&, gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0&&)::{lambda(auto:1)#1}, nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::EntryHandle&&>::type nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::WithEntryHandle<nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::LookupOrInsertWith<gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0>(mozilla::fontlist::Face* const&, gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0&&)::{lambda(auto:1)#1}>(mozilla::fontlist::Face*, gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0&&) objdir-ff-aflpp/dist/include/nsBaseHashtable.h:834:18
    #15 0x7fffcf184f55 in RefPtr<gfxFontEntry>& nsBaseHashtable<nsPtrHashKey<mozilla::fontlist::Face>, RefPtr<gfxFontEntry>, gfxFontEntry*, nsDefaultConverter<RefPtr<gfxFontEntry>, gfxFontEntry*> >::LookupOrInsertWith<gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0>(mozilla::fontlist::Face* const&, gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*)::$_0&&) objdir-ff-aflpp/dist/include/nsBaseHashtable.h:422:12
    #16 0x7fffcf17458a in gfxPlatformFontList::GetOrCreateFontEntryLocked(mozilla::fontlist::Face*, mozilla::fontlist::Family const*) gfx/thebes/gfxPlatformFontList.cpp:1794:8
    #17 0x7fffcef604ef in gfxPlatformFontList::GetOrCreateFontEntry(mozilla::fontlist::Face*, mozilla::fontlist::Family const*) gfx/thebes/gfxPlatformFontList.h:538:12
    #18 0x7fffcef5fbf5 in mozilla::fontlist::Family::SearchAllFontsForChar(mozilla::fontlist::FontList*, GlobalFontMatch*) gfx/thebes/SharedFontList.cpp:516:54
    #19 0x7fffcf17da23 in gfxPlatformFontList::GlobalFontFallback(nsPresContext*, unsigned int, unsigned int, mozilla::intl::Script, eFontPresentation, gfxFontStyle const*, unsigned int&, FontFamily&) gfx/thebes/gfxPlatformFontList.cpp:1238:18
    #20 0x7fffcf17aa7f in gfxPlatformFontList::SystemFindFontForChar(nsPresContext*, unsigned int, unsigned int, mozilla::intl::Script, eFontPresentation, gfxFontStyle const*, FontVisibility*) gfx/thebes/gfxPlatformFontList.cpp:1042:12
    #21 0x7fffcf1dfef9 in gfxFontGroup::WhichSystemFontSupportsChar(unsigned int, unsigned int, mozilla::intl::Script, eFontPresentation) gfx/thebes/gfxTextRun.cpp:3781:51
    #22 0x7fffcf1d5345 in gfxFontGroup::FindFontForChar(unsigned int, unsigned int, unsigned int, mozilla::intl::Script, gfxFont*, FontMatchType*) gfx/thebes/gfxTextRun.cpp:3396:10
    #23 0x7fffcf277e62 in void gfxFontGroup::ComputeRanges<char16_t>(nsTArray<gfxFontGroup::TextRange>&, char16_t const*, unsigned int, mozilla::intl::Script, mozilla::gfx::ShapedTextFlags) gfx/thebes/gfxTextRun.cpp:3478:11
    #24 0x7fffcf27132d in void gfxFontGroup::InitScriptRun<char16_t>(mozilla::gfx::DrawTarget*, gfxTextRun*, char16_t const*, unsigned int, unsigned int, mozilla::intl::Script, gfxMissingFontRecorder*) gfx/thebes/gfxTextRun.cpp:2708:3
    #25 0x7fffcf1db288 in void gfxFontGroup::InitTextRun<char16_t>(mozilla::gfx::DrawTarget*, gfxTextRun*, char16_t const*, unsigned int, gfxMissingFontRecorder*) gfx/thebes/gfxTextRun.cpp:2632:9
    #26 0x7fffcf1d95a3 in already_AddRefed<gfxTextRun> gfxFontGroup::MakeTextRun<char16_t>(char16_t const*, unsigned int, gfxTextRunFactory::Parameters const*, mozilla::gfx::ShapedTextFlags, nsTextFrameUtils::Flags, gfxMissingFontRecorder*) gfx/thebes/gfxTextRun.cpp:2498:3
    #27 0x7fffdcc4f33c in BuildTextRunsScanner::BuildTextRunForFrames(void*) layout/generic/nsTextFrame.cpp:2526:28
    #28 0x7fffdcc4981c in BuildTextRunsScanner::FlushFrames(bool, bool) layout/generic/nsTextFrame.cpp:1646:17
    #29 0x7fffdcc5c8b4 in BuildTextRuns(mozilla::gfx::DrawTarget*, nsTextFrame*, nsIFrame*, nsLineList_iterator const*, nsTextFrame::TextRunType) layout/generic/nsTextFrame.cpp:1565:11
    #30 0x7fffdcbf227d in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, mozilla::gfx::DrawTarget*, nsIFrame*, nsLineList_iterator const*, unsigned int*) layout/generic/nsTextFrame.cpp:2988:7
    #31 0x7fffdccb49ff in nsTextFrame::AddInlinePrefISizeForFlow(gfxContext*, nsIFrame::InlinePrefISizeData*, nsTextFrame::TextRunType) layout/generic/nsTextFrame.cpp:8592:7
    #32 0x7fffdccb6404 in nsTextFrame::AddInlinePrefISize(gfxContext*, nsIFrame::InlinePrefISizeData*) layout/generic/nsTextFrame.cpp:8732:10
    #33 0x7fffdc77ca13 in nsBlockFrame::GetPrefISize(gfxContext*) layout/generic/nsBlockFrame.cpp:980:16
    #34 0x7fffdc5afd97 in nsLayoutUtils::IntrinsicForAxis(mozilla::PhysicalAxis, gfxContext*, nsIFrame*, mozilla::IntrinsicISizeType, mozilla::Maybe<mozilla::LogicalSize> const&, unsigned int, int) layout/base/nsLayoutUtils.cpp:4925:30

A few remarks about this crash:

1. The stack is truncated. There is an underlying issue in the fuzzing toolstack where the stacks are truncated to 4K. I am trying to locate where this is happening and see if we end up finding it again once this is fixed.

2. Even though it looks like a content stack trace, this should be in the parent. We have an ASan crash handler that only intercepts and picks up crashes in the parent process.

3. This issue is not reproducible in my replay mode at all. I can see in the replay mode that the "SetCharacterMap" message is sent to PContent, so maybe that is related. I've been discussing this with :jfkthame already and our current working theory is that something in that message is corrupting the state, but the actual use of memory is triggered later, maybe by the UI and unrelated to IPC messages.

I will post the content of the potentially related IPC message in the next comment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Christian Holler (:decoder)]

DEBUG: MakeTargetDecision: Top-Level Protocol: PContent Protocol: PContent msgType: PContent::Msg_SetCharacterMap (0 56 2147483647)
INFO: Replaying IPC packet with payload:

  0x01 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
  0x00 0x00 0x00 0x00 0x00 0xFF 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0xFF 0xEE 0x00 0x00
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
  0x00 0x00 0xFF 0x00 0xF7 0x01 0xAD 0xAB 0x31 0x87 0x16 0x00 0x00 0x00 0x00 0x31
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x2C 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
  0x00 0x00 0x00 0x51 0x7E 0x28 0x05 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0x6E
  0x00 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0x5F 0x00 0x00 0x00 0x00 0x00 0x00
  0x00 0x00 0x00 0x80 0x00 0x00 0x00 0x00 0x00 0x20 0x02 0x06 0xF6 0xA5 0xF4 0x84
  0x01 0xF0 0x04 0x16 0xE8 0x54 0xB9 0xB6 0xC6 0x29 0xA2 0x0B 0x1A 0x71 0x4F 0x85
  0x98 0x40 0x7C 0x54 0x31 0x9E 0xC9 0x50 0xFE 0xC9 0xD7 0x1A 0xCF 0x87 0x7C 0x16
  0xF1 0xA1 0x8C 0xB7 0xAF 0x9A 0x2A 0xC3 0x55 0xAE 0x59 0x75 0x6B 0x48 0x27 0x1C
  0x6D 0x1E 0xDA 0xD4 0x80 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0xDF 0xAB 0xAB 0x6E 0x00
  0xAB 0xAB 0xAB 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0xAB 0xAB 0xAB 0xAB 0xAB
  0xAB 0xAB 0xAB 0xAB 0xAB 0x9F 0x01 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0x54 0xAB
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x47 0x00 0x00 0x00 0x00 0x00 0xAB 0xAB 0xAB
  0xAB 0x9D 0xAB 0xAB 0x00 0x00 0x00 0x00 0x00 0xAB 0xAB 0xAB 0xAB 0xAB 0xB3 0x5A
  0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0xAF 0x53 0x00 0xAB 0xAB 0xAB 0xAB
  0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0x6E 0x00 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB 0xAB
  0xAB 0xAB 0x00 0x00 0x00 0x01 0x00 0x00 0x00 0x00 0x00 0x53 0x4E 0x47 0xB7 0xA3
  0xAC 0x00 0x00 0x47 0x00 0x00 0x00 0xFF 0xFF 0xFF 0xFF 0xFF 0xFF 0xE3 0x00 0x00
  0x00 0x00 0x00 0x00 0x00 0x10 0x6B 0x00 0x00 0x00 0x0D 0xBE 0x93 0x6D 0x59 0x99
  0x5A 0x96 0xA9 0xDD 0x82 0x17 0x0B 0x9A 0x2B 0x7F 0x12 0xA6 0x5F 0xDB 0x83 0xBB
  0x6E 0x45 0xFE 0x22 0xAF 0x69 0x5D 0x5F 0x31 0x35 0xF4 0x91 0x8D 0x27 0xEA 0xB0
  0xED 0x69 0x4E 0x2D 0x5B 0x7D 0x35 0xF4 0x91 0x8D 0x27 0xEA 0xEA 0x40 0x64 0xED

After this message, it sends multiple PWindowGlobal::Msg_UpdateDocumentTitle messages. Maybe one of these could trigger the TextFrame change in the crash trace? Unfortunately we don't have any nice tooling (yet) to transfer these raw binary blobs into useful IPC structures per signature. That would indeed be quite handy to debug situations like this.

Comment 4

[Jonathan Kew [:jfkthame]]

At least one potential source of corruption in the font-list data, which could lead to something like this, is that Pointer::ToPtr fails to check the validity of the offset field in the Pointer value it's decoding. So if a broken/malicious content process passes a bad Pointer to the parent in a message like SetCharacterMap, it could cause the parent to generate an incorrect Face pointer and through this, corrupt other parts of the font-list data. (Or possibly generate a pointer to some other area altogether, and corrupt something else on the heap or stack.)

So in ToPtr, we should check the offset against the allocated size of the block, and fail (return null) if it's invalid instead of giving a bad pointer back to the caller.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1847529 - Provide typed versions of Pointer::ToPtr. r=#gfx-reviewers

Comment 6

[Christian Holler (:decoder)]

I can confirm that the Offset() < block->Allocated() condition in the patch is violated by the replay of the message in the testcase.

I also managed to cause a crash without the patch by adding this after the GetShmemCharMap call in gfxPlatformFontList::SetCharacterMap:

+    nsAutoCString desc(face->mDescriptor.AsString(SharedFontList()));
+    fprintf(stderr, "DEBUG: Desc: %s\n", desc.get());

It does not crash consistently and when it crashes, it crashes with a wild-ptr segfault, not with a stack-buffer-underflow, but it's likely that that's just by chance and depends on ASan's red zone placements. If we are sufficiently out-of-bounds, ASan won't detect it as an over/underflow anymore and maybe we crash, maybe we don't.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 9347687 [details]
Bug 1847529 - Provide typed versions of Pointer::ToPtr. r=#gfx-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's pretty clear from the patch that the issue is about passing an invalid parameter in an IPC message from content to parent.

I don't think this can be directly exploited by itself, but an already-compromised content process that is able to send attacker-controlled IPC messages could use this to escalate to controlling the parent (corrupting data in the parent, achieving arbitrary code execution, etc.)

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• 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?: No
• If not, how different, hard to create, and risky will they be?: Should transplant cleanly, I expect.
• How likely is this patch to cause regressions; how much testing does it need?: Minimal risk, just adding a missing bounds-check.
• Is Android affected?: Yes

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 9347687 [details]
Bug 1847529 - Provide typed versions of Pointer::ToPtr. r=#gfx-reviewers

Is this fix sufficient? Please see my question at https://phabricator.services.mozilla.com/D185564#6138327

clearing the sec-approval request for now. You don't need to re-answer all the questions when you add it back

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 9347687 [details]
Bug 1847529 - Provide typed versions of Pointer::ToPtr. r=#gfx-reviewers

Security Approval Request

[see comment 7]

Comment 10

[Tom Ritter [:tjr] (OOTO until April)]

Comment on attachment 9347687 [details]
Bug 1847529 - Provide typed versions of Pointer::ToPtr. r=#gfx-reviewers

Approved to land and uplift

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 9347687 [details]
Bug 1847529 - Provide typed versions of Pointer::ToPtr. r=#gfx-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Potential path for a compromised content process to escape content sandboxing and corrupt/compromise the parent process
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): No change in behavior in normal use; patch adds bounds checking to protect against corrupt/malicious IPC messages sent to the parent process.
• String changes made/needed:
• Is Android affected?: Yes

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad43d22e453c
Provide typed versions of Pointer::ToPtr. r=gfx-reviewers,lsalzman

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24195ef5e1a7
Backed out changeset ad43d22e453c for causing reftest failures. CLOSED TREE

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/284152af309e
Provide typed versions of Pointer::ToPtr. r=gfx-reviewers,lsalzman

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/284152af309e

Comment 16

[Donal Meehan [:dmeehan]]

Comment on attachment 9347687 [details]
Bug 1847529 - Provide typed versions of Pointer::ToPtr. r=#gfx-reviewers

Approved for 117.0b7

Comment 17

[Pulsebot]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This needs a bit of rebasing for ESR102. Can you please attach that patch and request approval when you get a chance? Thanks!

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1847529 - [esr102] Provide typed versions of Pointer::ToPtr. r=#gfx-reviewers

Comment 20

[Jonathan Kew [:jfkthame]]

Patch for esr102 attached, but not yet tested -- mach is failing for me. I'll try on a different system....

Comment 21

[Jonathan Kew [:jfkthame]]

Ryan, can you check if the esr102 patch builds OK? My attempts have all failed but with errors that don't look related to this patch in any way (different issues on each of macOS, Windows, Linux), so I'm guessing it should be OK given the right build environment....

Comment 22

[Jonathan Kew [:jfkthame]]

Comment on attachment 9348883 [details]
Bug 1847529 - [esr102] Provide typed versions of Pointer::ToPtr. r=#gfx-reviewers

Rebased to esr102, but be aware that I have not been able to build & test locally.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9347687 [details]
Bug 1847529 - Provide typed versions of Pointer::ToPtr. r=#gfx-reviewers

Approved for 115.2esr.

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/e836d52a4783

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jonathan Kew [:jfkthame] from comment #21)

Ryan, can you check if the esr102 patch builds OK? My attempts have all failed but with errors that don't look related to this patch in any way (different issues on each of macOS, Windows, Linux), so I'm guessing it should be OK given the right build environment....

LGTM on this Try push: https://treeherder.mozilla.org/jobs?repo=try&revision=ac0da67a11ebc43d3f244a5d7e1bb3aef5d8efe6

Any other test suites we should try throwing at it?

Comment 26

[Jonathan Kew [:jfkthame]]

No, should be fine: if it builds on all platforms, and can run reftests without deadlocks/crashes/assertions/leaks, I'm comfortable that I didn't scramble the rebase. :)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9348883 [details]
Bug 1847529 - [esr102] Provide typed versions of Pointer::ToPtr. r=#gfx-reviewers

Approved for 102.15esr.

Comment 28

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr102/rev/2302205cf28f