Make oggz_comments.c more resilient against possible future problems




9 years ago
4 years ago


(Reporter: reed, Unassigned)



Firefox Tracking Flags

(status2.0 unaffected, status1.9.2 wanted)


(Whiteboard: [sg:nse] future-proofing)



9 years ago
Dan Kaminsky reported an issue with Vorbis to Monty of 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 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.


9 years ago
Whiteboard: [sg:critical?]

Comment 1

9 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).

Comment 2

9 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]
Whiteboard: [sg:nse] → [sg:nse] future-proofing
We've removed liboggz on trunk, so this vulnerability is gone on trunk at least. May still be a vulnerability on other branches.
Might as well close this.  Code is gone on trunk, and there's no known vuln on the branches.
Last Resolved: 8 years ago
Resolution: --- → FIXED
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
Vorbis comments in the bitstream are limited to 32-bit unsigned integers:
Group: core-security
You need to log in before you can comment on or make changes to this bug.