Closed
Bug 515891
Opened 15 years ago
Closed 14 years ago
Make oggz_comments.c more resilient against possible future problems
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status2.0 | --- | unaffected |
status1.9.2 | --- | wanted |
People
(Reporter: reed, Unassigned)
Details
(Whiteboard: [sg:nse] future-proofing)
Dan Kaminsky reported an issue with Vorbis to Monty of xiph.org that could possibly affect Firefox. -------------------- Finally, oggz_comments.c, you are so very lucky :) static char * oggz_strdup (const char * s) { char * ret; if (s == NULL) return NULL; ret = oggz_malloc (oggz_comment_len(s) + 1); if (ret == NULL) return NULL; return strcpy (ret, s); } So what does oggz_comment_len do... static size_t oggz_comment_len (const char * s) { size_t len; if (s == NULL) return 0; len = strlen (s); return oggz_comment_clamp(len); } Wait, a strlen followed by a clamp? With the result of the clamp not passed to a strncpy, but a strcpy? What's the clamp at? #define MAX_COMMENT_LENGTH 0xFFFFFFFE #define oggz_comment_clamp(c) MIN((c),MAX_COMMENT_LENGTH) You lucky devil :) If that was, say, 0x0000FFFF, the attacker could throw in a slightly larger string and you'd strcpy into an insufficiently sized array. Might want to poke at that code too. -----------------------
Reporter | ||
Updated•15 years ago
|
Whiteboard: [sg:critical?]
Comment 1•15 years ago
|
||
Just to be clear, absolutely, definitely not vulnerable here. Just really bad form, and might cause a *massively* vuln someday in the future (if MAX_COMMENT_LENGTH is ever shrunken).
Reporter | ||
Comment 2•15 years ago
|
||
Ok, thanks. Leaving this security-sensitive until Monty or somebody has had a chance to double-check that something vulnerable isn't being overlooked somewhere.
Summary: Possible vulnerability in oggz_comments.c → Make oggz_comments.c more resilient against possible future problems
Whiteboard: [sg:critical?] → [sg:nse]
Updated•15 years ago
|
Whiteboard: [sg:nse] → [sg:nse] future-proofing
Comment 3•14 years ago
|
||
We've removed liboggz on trunk, so this vulnerability is gone on trunk at least. May still be a vulnerability on other branches.
Comment 4•14 years ago
|
||
Might as well close this. Code is gone on trunk, and there's no known vuln on the branches.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 5•14 years ago
|
||
Could this be a problem on 64-bit systems? is the ogg container limited to 32-bit sizes? If not the size_t-based code from comment 0 will have the same problem initially described. We don't officially ship 64-bit on stable branches, but some of the linux vendors do.
status1.9.2:
--- → wanted
status2.0:
--- → unaffected
Comment 6•14 years ago
|
||
Vorbis comments in the bitstream are limited to 32-bit unsigned integers: http://xiph.org/vorbis/doc/v-comment.html
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•