#1113 closed defect (wontfix)
Valgrind reports an invalid write of size 4
| Reported by: | Owned by: | ||
|---|---|---|---|
| Priority: | important | Component: | demuxer |
| Version: | HEAD | Severity: | major |
| Keywords: | Cc: | reimar, zlai88@…, nstockma@…, catchconv-bugreports@… | |
| Blocked By: | Blocking: | ||
| Reproduced by developer: | no | Analyzed by developer: | no |
Description
* Overview: Found a test case .mp4 file for mplayer where valgrind 3.3.1 reports an invalid write of size 4
The test case is "76-geass.mp4" available at the URL
http://www.cs.berkeley.edu/~quach/76-geass.mp4
* mplayer version
dev-SVN-r27139-4.1.2
* To reproduce:
1) Play 76-geass.mp4 using mplayer under Valgrind 3.3.1
* My OS:
Debian Etch Linux, Intel(R) Core(TM)2 Duo CPU T7250 @ 2.00GHz
uname -a:
Linux debian 2.6.18-6-486 #1 Fri Jun 6 21:47:01 UTC 2008 i686 GNU/Linux
Valgrind output, from command
==10615== Memcheck, a memory error detector.
==10615== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al.
==10615== Using LibVEX rev 1854, a library for dynamic binary translation.
==10615== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP.
==10615== Using valgrind-3.3.1, a dynamic binary instrumentation framework.
==10615== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al.
==10615== For more details, rerun with: -v
==10615==
MPlayer dev-SVN-r27139-4.1.2 (C) 2000-2008 MPlayer Team
CPU: Intel(R) Core(TM)2 CPU 6600 @ 2.40GHz (Family: 6, Model: 15, Stepping: 6)
CPUflags: MMX: 1 MMX2: 1 3DNow: 0 3DNow2: 0 SSE: 1 SSE2: 1
Compiled for x86 CPU with extensions: MMX MMX2 SSE SSE2
Playing 76-geass.mp4.
libavformat file format detected.
[mov,mp4,m4a,3gp,3g2,mj2 @ 0x862a1f0]Could not find codec parameters (Video: 0x0000)
LAVF_header: av_find_stream_info() failed
Quicktime/MOV file format detected.
==10615== Warning: set address range perms: large range 134217736 (defined)
Warning! pts=-523370558 length=37037
MOV: durmap and chunkmap sample count differ (1279210280 vs 0)
==10615== Invalid write of size 4
==10615== Stack hash: 412701254
==10615== at 0x8136CFA: mov_build_index (demux_mov.c:211)
==10615== by 0x813A6F2: lschunks (demux_mov.c:1307)
==10615== by 0x813B6B5: mov_read_header (demux_mov.c:1926)
==10615== by 0x811BD1E: demux_open_stream (demuxer.c:864)
==10615== by 0x811BFF1: demux_open (demuxer.c:991)
==10615== by 0x80751EE: main (mplayer.c:3238)
==10615== Address 0x4 is not stack'd, malloc'd or (recently) free'd
MPlayer interrupted by signal 11 in module: demux_open
- MPlayer crashed by bad usage of CPU/FPU/RAM. Recompile MPlayer with --enable-debug and make a 'gdb' backtrace and disassembly. Details in DOCS/HTML/en/bugreports_what.html#bugreports_crash.
- MPlayer crashed. This shouldn't happen. It can be a bug in the MPlayer code _or_ in your drivers _or_ in your gcc version. If you think it's MPlayer's fault, please read DOCS/HTML/en/bugreports.html and follow the instructions there. We can't and won't help unless you provide this information when reporting a possible bug.
==10615==
==10615== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 17 from 1)
==10615== malloc/free: in use at exit: 134,315,926 bytes in 2,182 blocks.
==10615== malloc/free: 2,323 allocs, 140 frees, 135,591,222 bytes allocated.
==10615== For counts of detected errors, rerun with: -v
==10615== searching for pointers to 2,182 not-freed blocks.
==10615== checked 137,027,876 bytes.
==10615==
==10615== LEAK SUMMARY:
==10615== definitely lost: 0 bytes in 0 blocks.
==10615== possibly lost: 0 bytes in 0 blocks.
==10615== still reachable: 134,315,926 bytes in 2,182 blocks.
==10615== suppressed: 0 bytes in 0 blocks.
==10615== Reachable blocks (those to which a pointer was found) are not shown.
==10615== To see them, rerun with: --leak-check=full --show-reachable=yes
This bug was found as part of the metafuzz project; see http://metafuzz.com/
Change History (12)
comment:1 by , 16 years ago
| Cc: | added |
|---|
comment:2 by , 16 years ago
| Cc: | added |
|---|---|
| Resolution: | → wontfix |
| Status: | new → closed |
comment:3 by , 16 years ago
| Resolution: | wontfix |
|---|---|
| Status: | closed → reopened |
(In reply to comment #1)
Our internal mov demuxer is awaiting removal. Non-exploitable bugs (DoS does
not count) will not be fixed, and this problem IMO falls in this category.
Hmm. Can I ask how you determined that this is non-exploitable? I wonder. Normally an InvalidWrite error has a high risk of being a symptom of an exploitable security vulnerability.
I just looked quickly at the relevant code, and I couldn't immediately see why this would be unexploitable. I have to confess that it looks to me like there's a significant risk this could be exploitable -- it looks to me like a classic integer overflow vulnerability. Here's the relevant code:
trak->samples_size=s;
trak->samples=calloc(s, sizeof(mov_sample_t));
for(i=0;i<s;i++)
trak->samples[i].size=trak->samplesize; <--- InvalidWrite occurs here
I haven't confirmed this, but it looks to me like the value of s is under the control of the attacker (i.e., under the control of whoever creates the .mp4 file). Also, we're on a 32-bit machine, so sizeof(mov_sample_t) = 16. Now suppose s=0x10000001, say. Then calloc() will allocate (unsigned int)16*s bytes, i.e., 0x00000010 bytes, i.e., only 16 bytes. But then the for loop will try to write to trak->sample[i].size for i=0,1,...,0x10000000. That's writing to memory outside the range of the allocated buffer trak->samples. Because the attacker can control the size of the buffer allocated by calloc(), the attacker has some control over the locations that get overwritten in this way. Also, because the attacker can control s, the attacker has some control over the values that get written into those locations.
In general, if we find a bug that allows the attacker to write an arbitrary value of his choosing at an arbitrary location (of his choosing) in the program's address space, it's game over, from a security point of view -- that immediately leads to an exploitable security vulnerability. In this case it appears that there are some partial restrictions on these values, so it's not immediately obvious whether this is exploitable, but I am concerned. Am I missing something? You're the expert on this code, not me, so I could easily be overlooking some factor that renders this non-exploitable.
By the way, I should mention that in this particular case with 76-geass.mp4, we had s=1279210280=0x4C3F3328. Therefore, on this input file, calloc() will try to allocate 0xC3F33280 bytes of memory. Of course there isn't likely to be that much memory available, so calloc() fails and returns NULL, so we have trak->samples = NULL. That's why, for 76-geass.mp4, when the code tries to write to trak->samples[i].size (where trak->samples = NULL and i = 0), it is trying to write to address 0x4, triggering the Valgrind InvalidWrite error and the segfault. Of course writing to memory address 0x4 is not itself likely to be exploitable. But this is just a symptom. If the attacker chooses some other value of s, then calloc() may not return NULL; it may just allocate an insufficient amount of memory and return a pointer to that (too-small) region of memory.
Do you agree? Or have I screwed up my analysis? I hope I'm not wasting your time!
comment:4 by , 16 years ago
Then calloc() will allocate (unsigned int)16*s
bytes, i.e., 0x00000010 bytes, i.e., only 16 bytes.
No. Such a calloc is in our and (to my knowledge) FFmpegs view not compliant and has a security issues. Only systems where calloc returns NULL are supported, and this will not change, whoever runs a system with a libc that has years-old integer overflow issues can not seriously expect any kind of security.
If calloc returns NULL in such a case, this particular code (on all systems I am aware of) should be guaranteed to crash with segfault before anything would be overwritten (I admit that a compiler probably could change the loop to be executed in reverse without violating the C standard, which would be a serious problem, but none of the supported compilers do that, and there is little sense in doing it).
I can very much understand you concerns and welcome the discussion, but given limited developer time handling it this way seemed sanest to us.
comment:5 by , 16 years ago
| Resolution: | → wontfix |
|---|---|
| Status: | reopened → closed |
OK, got it. That makes sense and seems reasonable to me.
And thanks for the education. I learned something new (I hadn't realized that modern calloc() implementations check for this; my fault entirely). Sorry to take up your time on this.
comment:10 by , 16 years ago
Sample can be obtained here:
http://www.metafuzz.com/testcases/965225-76-3915876026-InvalidWrite.tgz
Our internal mov demuxer is awaiting removal. Non-exploitable bugs (DoS does not count) will not be fixed, and this problem IMO falls in this category.