Closed Bug 1861016 Opened 3 years ago Closed 2 years ago

Heap Buffer Overflow ASAN Crash in AES_InitContext when calling PK11_CreateContextBySymKey with CKM_AES_CBC_PAD

Categories

(Core :: Audio/Video: Playback, defect)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
125 Branch
Tracking Flags:
Tracking Status
firefox-esr115 124+ fixed
firefox123 --- wontfix
firefox124 + fixed
firefox125 + fixed

People

(Reporter: noah.lokocz, Assigned: alwu)

References

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [adv-main124+r][adv-esr115.9+r])

Attachments

(1 file, 2 external files, 1 obsolete file)

crash-e1bf2e61f8b54acc6b8e2dc6e9ddd6b3659dffa1
97 bytes, application/octet-stream
Details

+++ This bug was initially created as a clone of Bug #1861015 +++

One of the fuzzing runs of this fuzzer leads to an Heap Buffer Overflow ASAN crash within AES_InitContext when the CK_MECHANISM CKM_AES_CBC_PAD is used. The crash unit for the fuzzing run causing the crash is attached.

ASAN/Fuzzer output:
==35536==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000061c1a at pc 0x000101ac4a50 bp 0x00016fc2e020 sp 0x00016fc2d7d0
READ of size 16 at 0x602000061c1a thread T0
#0 0x101ac4a4c in __asan_memcpy+0x37c (libclang_rt.asan_osx_dynamic.dylib:arm64+0x50a4c)
#1 0x100609684 in AES_InitContext rijndael.c:1003
#2 0x10060a2d8 in AES_CreateContext rijndael.c:1092
#3 0x10046dbd0 in sftk_CryptInit pkcs11c.c:1249
#4 0x100472a64 in NSC_EncryptInit pkcs11c.c:1457
#5 0x10036b80c in pk11_context_init pk11cxt.c:206
#6 0x100366c70 in pk11_CreateNewContextInSlot pk11cxt.c:428
#7 0x100366488 in PK11_CreateContextBySymKey pk11cxt.c:510
#8 0x1001d30cc in LLVMFuzzerTestOneInput nssfuzz-NSC_EncryptUpdate_DecryptUpdate.cc:113
#9 0x1007bacc8 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:614
#10 0x1007ba5b0 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) FuzzerLoop.cpp:516
#11 0x1007bbcfc in fuzzer::Fuzzer::MutateAndTestOne() FuzzerLoop.cpp:760
#12 0x1007bcb3c in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, std::__1::allocator<fuzzer::SizedFile>>&) FuzzerLoop.cpp:905
#13 0x1007ac6f0 in fuzzer::FuzzerDriver(int*, char***, int ()(unsigned char const, unsigned long)) FuzzerDriver.cpp:911
#14 0x1007d6810 in main FuzzerMain.cpp:20
#15 0x18b655054 (<unknown module>)
#16 0xe261fffffffffffc (<unknown module>)

0x602000061c1a is located 0 bytes after 10-byte region [0x602000061c10,0x602000061c1a)
allocated by thread T0 here:
#0 0x101ad6354 in _Znwm+0x6c (libclang_rt.asan_osx_dynamic.dylib:arm64+0x62354)
#1 0x1001d3ddc in std::__1::vector<unsigned char, std::__1::allocator<unsigned char>>::__vallocate[abi:v170002](unsigned long) vector:756
#2 0x1001d3924 in void std::__1::vector<unsigned char, std::__1::allocator<unsigned char>>::__init_with_size[abi:v170002]<unsigned char const*, unsigned char const*>(unsigned char const*, unsigned char const*, unsigned long) vector:775
#3 0x1001d29d4 in LLVMFuzzerTestOneInput nssfuzz-NSC_EncryptUpdate_DecryptUpdate.cc:44
#4 0x1007bacc8 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:614
#5 0x1007ba5b0 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) FuzzerLoop.cpp:516
#6 0x1007bbcfc in fuzzer::Fuzzer::MutateAndTestOne() FuzzerLoop.cpp:760
#7 0x1007bcb3c in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, std::__1::allocator<fuzzer::SizedFile>>&) FuzzerLoop.cpp:905
#8 0x1007ac6f0 in fuzzer::FuzzerDriver(int*, char***, int ()(unsigned char const, unsigned long)) FuzzerDriver.cpp:911
#9 0x1007d6810 in main FuzzerMain.cpp:20
#10 0x18b655054 (<unknown module>)
#11 0xe261fffffffffffc (<unknown module>)

SUMMARY: AddressSanitizer: heap-buffer-overflow (libclang_rt.asan_osx_dynamic.dylib:arm64+0x50a4c) in __asan_memcpy+0x37c
Shadow bytes around the buggy address:
0x602000061980: fa fa fd fd fa fa fd fa fa fa fd fd fa fa fd fd
0x602000061a00: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fa
0x602000061a80: fa fa fd fa fa fa fd fa fa fa fd fd fa fa fd fa
0x602000061b00: fa fa fd fd fa fa fd fa fa fa fd fd fa fa fd fa
0x602000061b80: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
=>0x602000061c00: fa fa 00[02]fa fa 00 02 fa fa fa fa fa fa fa fa
0x602000061c80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x602000061d00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x602000061d80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x602000061e00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x602000061e80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==35536==ABORTING
MS: 2 InsertRepeatedBytes-InsertRepeatedBytes-; base unit: adc83b19e793491b1c6ea0fd8b46cd9f32e592fc
0xa,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0x18,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,0xb,
\012\013\013\013\013\013\013\013\013\013\013\013\013\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\030\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013\013
artifact_prefix='./'; Test unit written to ./crash-e1bf2e61f8b54acc6b8e2dc6e9ddd6b3659dffa1
Base64: CgsLCwsLCwsLCwsLCxgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGAsLCwsLCwsLCwsLCwsLCwsLCwsLCwsLCwsLCwsLCwsLCwsLCwsLCw==
zsh: abort ./nssfuzz-NSC_EncryptUpdate_DecryptUpdate

Blocks: 1861033

aes_InitContext does not check the length of iv before the memcpy here: https://searchfox.org/mozilla-central/rev/aa189da432fac500242d9374f044947357f7144e/security/nss/lib/freebl/rijndael.c#918. This is arguably not a sec bug as it's the caller's responsibility to pass well formed mechanism parameters and CKM_AES_CBC asks for a 16 byte IV.

However there is at least one potentially vulnerable caller (https://searchfox.org/mozilla-central/rev/aa189da432fac500242d9374f044947357f7144e/media/gmp-clearkey/0.1/ClearKeyUtils.cpp#133-134). That code uses assert to check that the IV buffer is 16 bytes, but I think that only fires in debug builds. (That code also fails to check that ctx is non-null, so we couldn't fix it from inside NSS if we wanted to.)

John, Noah,
Can you please set priority and severity for this ? Thank you !

Severity: -- → N/A
Flags: needinfo?(nlokocz)
Flags: needinfo?(jschanck)

PK11_CreateContextBySymKey is a dangerous function, but this isn't really a bug. I'm going to move this over to Core: Audio/Video: Playback as they're the only consumer that I see in searchfox.

I can't easily tell whether it's possible to pass a short IV to ClearKeyUtils::DecryptCbcs. It would put my mind at ease if this assert was an if statement and error return.

Assignee: nobody → nobody
No longer blocks: 1861033
Severity: N/A → --
Component: Libraries → Audio/Video: Playback
No longer depends on: 1860538, 1861015
Flags: needinfo?(nlokocz)
Flags: needinfo?(jschanck)
Product: NSS → Core
Version: other → Trunk

The severity field is not set for this bug.
:jimm, could you have a look please?

For more information, please visit BugBot documentation.

Flags: needinfo?(jmathies)
Severity: -- → S2
Flags: needinfo?(jmathies)
Blocks: media-triage
Flags: needinfo?(alwu)
Assignee: nobody → alwu
No longer blocks: media-triage
Flags: needinfo?(alwu)

Paul, do you know whether we can guarantee that IV is always 16 bytes? We seems always ensure 16 bytes IV for webm [1], but don't have such guarantee for mp4 [2]. If so, it seems it's possible to hit the problem if we have 8 bytes IV in mp4?

[1] https://searchfox.org/mozilla-central/rev/41edcdf7fe44678c5913a603a286b1fc3979d540/dom/media/webm/WebMDemuxer.cpp#786-791
[2] https://searchfox.org/mozilla-central/rev/41edcdf7fe44678c5913a603a286b1fc3979d540/dom/media/mp4/SampleIterator.cpp#196

Flags: needinfo?(padenot)

Comment on attachment 9379269 [details]
Bug 1861016 - some fixes.

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: In theory, the exploit would probably be easy to be hit by providing IV less than 16 bytes.
  • 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: 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 patch is not able to backport to the branch before Fx123 because the file I changed was in another place in our tree in the older brances, but it's easy to create a patch for backport.
  • How likely is this patch to cause regressions; how much testing does it need?: Hard to tell, for webM we've guaranteed that IV must be 16 bytes, but we don't have such restriction for mp4. If user is watching a site "using ClearKey" + "using 8 bytes IV in mp4", then it's possible that the video can't be played. But I don't know if it's possible and ClearKey is not used for the real EME playback in the wild because it's just something used for primarily testing.
  • Is the patch ready to land after security approval is given?: Yes
  • Is Android affected?: No
Attachment #9379269 - Flags: sec-approval?

Comment on attachment 9379269 [details]
Bug 1861016 - some fixes.

Approved to land and request uplift.

Attachment #9379269 - Flags: sec-approval? → sec-approval+

Comment on attachment 9379269 [details]
Bug 1861016 - some fixes.

Beta/Release Uplift Approval Request

  • User impact if declined: Unexpected behavior caused by heap-buffer-overflow
  • Is this code covered by automated tests?: No
  • 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): This patch doesn't introduce new behavior and structural change, it's just about adding additional conditions to guard invalid parameters which could cause problem in the external library.
  • String changes made/needed:
  • Is Android affected?: No
Attachment #9379269 - Flags: approval-mozilla-beta?
Attached file Bug 1861016 - some fixes. (obsolete) —

Original Revision: https://phabricator.services.mozilla.com/D201260

Attachment #9381904 - Flags: approval-mozilla-beta?
Attachment #9381904 - Attachment is obsolete: true
Attachment #9381904 - Flags: approval-mozilla-beta?
Pushed by alwu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/33e0f77d82ce some fixes. r=media-playback-reviewers,jolin

https://hg.mozilla.org/mozilla-central/rev/33e0f77d82ce

Group: crypto-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox125: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 125 Branch
Flags: needinfo?(padenot)

Comment on attachment 9379269 [details]
Bug 1861016 - some fixes.

Approved for 124.0b4

Attachment #9379269 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-

I don't remember what discussion we were having in that Oct 25 triage meeting that led to the sec-other for this group of NSS fuzzer bugs, but it was before we found the Media caller and potential for exploiting through .mp4

Keywords: sec-other → sec-high

[Tracking Requested - why for this release]:
This bug had an incorrectly low sec- rating that was not updated when we discovered this API was used by Media code processing web content. As a potential sec-high vulnerability it should have been tracked forthe 115.9 ESR

status-firefox-esr115: wontfix → affected
tracking-firefox-esr115: --- → ?
Whiteboard: [adv-main124+r]

Comment on attachment 9379269 [details]
Bug 1861016 - some fixes.

Approved for 115.9esr.

Attachment #9379269 - Flags: approval-mozilla-esr115+
Whiteboard: [adv-main124+r] → [adv-main124+r][adv-esr115.9+r]
Group: core-security-release
See Also: → CVE-2026-2788
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: