Closed Bug 1080803 Opened 5 years ago Closed 5 years ago

ASAN heap buffer overflow in ClearKey CDM

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 --- disabled
firefox36 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: jld, Assigned: eflores)

References

Details

(Keywords: csectype-bounds, sec-high)

Attachments

(2 files, 1 obsolete file)

Attached file fail.log
While investigating bug 1080077 I found this (full log is attached):

==32614==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x613000029ffa at pc 0x7f17befe3983 bp 0x7f17bde700d0 sp 0x7f17bde700c8
READ of size 1 at 0x613000029ffa thread T2 (GMPThread)
[...]
==32614==WARNING: readlink("/proc/self/exe") failed with errno 38, some stack frames may not be symbolized
    #0 0x7f17befe3982 (/home/jld/obj/gecko-dev/obj-x86_64-unknown-linux-gnu/dist/bin/gmp-clearkey/0.1/libclearkey.so+0xf982)
[...]
0x613000029ffa is located 0 bytes to the right of 378-byte region [0x613000029e80,0x613000029ffa)
allocated by thread T2 (GMPThread) here:
    #0 0x497899 (/proc/self/exe+0x497899)
    #1 0x7f17db774647 (/home/jld/obj/gecko-dev/obj-x86_64-unknown-linux-gnu/dist/bin/libmozalloc.so+0x2647)
    #2 0x7f17befe293a (/home/jld/obj/gecko-dev/obj-x86_64-unknown-linux-gnu/dist/bin/gmp-clearkey/0.1/libclearkey.so+0xe93a)

The out-of-bounds access is media/gmp-clearkey/0.1/ClearKeyUtils.cpp line 66, in ClearKeyUtils::DecryptAES:

      aData[i + j] ^= enc[2 * OAES_BLOCK_SIZE + j];

The allocation is media/gmp-clearkey/0.1/ClearKeyDecryptionManager.cpp line 314, in ClearKeyDecryptor::Decrypt:

  vector<uint8_t> tmp(aBuffer->Size());

This is also happening in m-c ASAN builds, except that it looks like this:

18:03:09     INFO -  1781 INFO TEST-START | /tests/content/media/test/test_encryptedMediaExtensions.html
18:03:09     INFO -  I/SampleTable( 1887): There are reordered frames present.
18:03:10     INFO -  =================================================================
18:03:10     INFO -  Sandbox: seccomp sandbox violation: pid 2671, syscall 16, args 2 21505 139925991184328 139925991183840 139925991184384 1.  Killing process.
18:03:11     INFO -  1782 INFO TEST-OK | /tests/content/media/test/test_encryptedMediaExtensions.html | took 1900ms

We don't get anything from ASAN because its attempt to do tcgetattr() violated the sandbox policy, we don't get a PROCESS-CRASH because ASAN disables the regular crash reporter, mozharness doesn't know about the "Sandbox" line because the hook for that is Android-specific, and for reasons beyond my current understanding this doesn't cause the actual test to report a failure.


I don't know if this bug needs to be security-sensitive, given that it's not exposed to web content yet.  (Also, I accidentally let this cat out of the bag in a public IRC channel before I realized what it implied.)  But the flag can always be cleared later if that's the case.
See Also: → 1080165
Edwin, can you look at this? It should only require a small minor fix... It's blocking more sandboxing work.
Flags: needinfo?(edwin)
Attached patch 1080803.patch (obsolete) — Splinter Review
Flags: needinfo?(edwin)
Attached patch 1080803.patchSplinter Review
oops
Assignee: nobody → edwin
Attachment #8502832 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8502832 - Flags: review?(cpearce)
Attachment #8502835 - Flags: review?(cpearce)
Attachment #8502835 - Flags: review?(cpearce) → review+
sec-other because "not exposed to web content yet".
Keywords: sec-other
Comment on attachment 8502835 [details] [diff] [review]
1080803.patch

content/media/test/test_encryptedMediaExtensions.html passes with this patch in my local ASAN build.
Attachment #8502835 - Flags: feedback+
Blocks: 1081242
No longer blocks: 1080077
https://hg.mozilla.org/mozilla-central/rev/9183e31ea8ca
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(In reply to Karl Tomlinson (:karlt) from comment #4)
> sec-other because "not exposed to web content yet".

That may be fair for things that will never be exposed to the web, but when we're working on features that we intend to enable in the future then it's better to use a rating that reflects what the risk will be at that time. We can always mark the specific version statuses as "disabled".
Group: 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.