Closed Bug 1080803 Opened 5 years ago Closed 5 years ago
ASAN heap buffer overflow in Clear
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.
Edwin, can you look at this? It should only require a small minor fix... It's blocking more sandboxing work.
5 years ago
Attachment #8502835 - Flags: review?(cpearce) → review+
sec-other because "not exposed to web content yet".
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+
(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".
You need to log in before you can comment on or make changes to this bug.