Closed Bug 533647 Opened 10 years ago Closed 10 years ago
Memory corruption vulnerability in tools/codesighs/codesighs
Received a security vulnerability notice to security@ from Jeremy Brown regarding the codesighs (https://wiki.mozilla.org/Codesighs) code: 257 while(0 == retval && NULL != fgets(lineBuffer, sizeof(lineBuffer), inOptions->mInput)) (gdb) 259 trimWhite(lineBuffer); (gdb) trimWhite (inString=0xbfffd310 "1\tCODE\t", 'A' <repeats 15 times>, "\t", 'A' <repeats 15 times>, "\t", 'A' <repeats 15 times>, "\t", 'A' <repeats 145 times>...) at codesighs.c:213 213 int len = strlen(inString); (gdb) 215 while(len) (gdb) 217 len--; (gdb) 219 if(isspace(*(inString + len))) (gdb) 221 *(inString + len) = '\0'; (gdb) 215 while(len) (gdb) 217 len--; (gdb) 219 if(isspace(*(inString + len))) (gdb) 228 } (gdb) codesighs (inOptions=0xbffff350) at codesighs.c:261 261 scanRes = sscanf(lineBuffer, (gdb) i r eax 0x0 0 ecx 0xb7fe468c -1208072564 edx 0x82 130 ebx 0x9d8ff4 10326004 esp 0xbfffd040 0xbfffd040 ebp 0xbffff328 0xbffff328 esi 0x0 0 edi 0x0 0 eip 0x8048945 0x8048945 <codesighs+142> eflags 0x246 [ PF ZF IF ] cs 0x73 115 ss 0x7b 123 ds 0x7b 123 es 0x7b 123 fs 0x0 0 gs 0x33 51 (gdb) s 270 if(6 == scanRes) (gdb) i r eax 0x6 6 ecx 0x414141 4276545 edx 0x0 0 ebx 0x9d8ff4 10326004 esp 0xbfffd040 0xbfffd040 ebp 0xbffff328 0xbffff328 esi 0x0 0 edi 0x0 0 eip 0x804899d 0x804899d <codesighs+230> eflags 0x282 [ SF IF ] cs 0x73 115 ss 0x7b 123 ds 0x7b 123 es 0x7b 123 fs 0x0 0 gs 0x33 51 (gdb) <test file> 1 CODE AAAAAAAAAAAAAAA AAAAAAAAAAAAAAA AAAAAAAAAAAAAAA AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA </test file> maptsvdifftool may share the same bug.
I'm not sure what all the gdb output is trying to say, but I'm guessing you're saying the problem is that we're passing buffers that aren't big enough for your test input to sscanf. However, this is a debugging tool used in specific situations, to read as input output generated by other programs. So even if it crashes in response to some other input, I don't see how that's a vulnerability. This tool is not designed to handle arbitrary input, nor does it need to be.
Component: General → Build Config
QA Contact: general → build-config
Yes, as I step through the program and show the registers before and after codesighs calls sscanf(), memory can be corrupted by simply feeding codesighs a file to parse. I came across Codesighs when checking out Google's Gadgets for Linux. It was included as third party software. And I'd bet GG is not the only package that includes Codesighs. You could also point out Firefox vulnerabilities only affect people browsing the web using Mozilla Firefox.
Comparing this bug to a bug in Firefox is ridiculous - millions of people use Firefox to consume "arbitrary content" (the Web) all the time. codesighs is far less popular, has a much more specific and narrow use case, and a victim would need to go through a lot of trouble to give it arbitrary content that will trigger this problem.
Would you like to tell me exactly which phrase, fragment or sentence of mine compares Codesighs to Firefox or anything else? Google's Chromium operating system and Google Gadgets for Linux both include Codesighs as third party software in their packages. Is giving someone a file to be checked by Codesighs a lot of trouble? I think not. The fact remains that this is a vulnerability in Mozilla software and should be fixed. "ridiculous" is the lack of character Mozilla is showing by not taking it seriously.
I'm not arguing that it shouldn't be fixed. I was trying to point out that the last paragraph of comment 2 is inflammatory and not constructive. I now regret having commented at all, though.
After one day this was unmarked as a security issue. *sigh*
What command line did you use to invoke codesighs to observe the problem? And does the white space in the file matter? There's a line break in what's attached above, but I suspect it got mangled. Could you attach the original?
I think this ought to fix it. I couldn't observe any valgrind warnings or crashes running './codesighs -i your-input-file', though, even without the patch.
Attachment #417367 - Flags: review?(benjamin)
In order to observe problems, I needed to convert the spaces to tabs and make the strings longer. This does fix observed problems on this test file.
Jeremy, it's only a vulnerability if you run the tool on untrusted content, and given the nature of the tool that's very unlikely.
We could apply that MITIGATION for any software. "Thunderbird is only vulnerable if I open an untrusted email" "Adobe Reader is only vulnerable if I open an untrusted PDF" So, "Codesighs is only vulnerable if you run it on untrusted content"? I'd love to actually see that documented by someone with authority...
(In reply to comment #11) > "Thunderbird is only vulnerable if I open an untrusted email" > > "Adobe Reader is only vulnerable if I open an untrusted PDF" Those are the normal ways of using Thunderbird and Adobe Reader. They're not the normal way of using codesighs.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Jeremy Brown, the reporter, blogged about this bug at http://jbrownsec.blogspot.com/2009/12/mozilla-code-sighs.html.
Summary: Vulnerability in tools/codesighs/codesighs.c → Memory corruption vulnerability in tools/codesighs/codesighs.c
(In reply to comment #6) > After one day this was unmarked as a security issue. *sigh* No, it was un-hidden. The Mozilla project hates having hidden, secret stuff only a few people know about. It's against our nature, and seriously kinks our normal development style of letting as many people as possible pitch in where they're interested. It also increases the amount of work required because the security team must then coordinate and manage making sure the right developers have access to the bugs. But protecting users from attack is another important value, and in the case of remotely triggerable vulnerabilities user safety wins out over our dislike of having to hide information. But when the risk is low (particularly local attacks) openness wins out. Doesn't mean we don't think it's a security issue.
Whiteboard: [sg:moderate local]
You need to log in before you can comment on or make changes to this bug.