GNOME Bugzilla – Bug 720615
Possible overflow in HTMLParser.c
Last modified: 2014-10-06 10:53:37 UTC
Created attachment 264424 [details] [review] Patch accounting for possible overflow [These were the line numbers at the time I noticed this bug. HEAD has changed slightly but the code is the same] The memory allocation on line 6291 uses math to calculate the amount of memory to allocate for a buffer. The math could result in an integer overflow, which would cause the 'content' buffer to be insufficient in size to hold the subsequent strcpy() and strcat() calls on 6293 and 6294. The root cause would be an 'encoding' type that was large enough to trigger an overflow. This could result in memory corruption and allow an attacker to execute arbitrary code. 6290 if (encoding) { 6291 content = xmlMallocAtomic (xmlStrlen(content_line) + strlen(encoding) + 1); 6292 if (content) { 6293 strcpy ((char *)content, (char *)content_line); 6294 strcat ((char *)content, (char *)encoding); I've attached a patch to detect the overflow and bail out in that case. I ran runsuite and runtest and saw no differences with this patch. I've tested compilation on 64bit OELinux and OS X 10.9.
I think if you worry about this situation, it can be fixed with following minimal change: - if (encoding) { + if (encoding && ((strlen(encoding)-xmlStrlen(content_line)-1) <= MAX_INT)) { Also, functions strncpy and strncat can be used, these will ensure copying exact no of bytes. One more question : For content_line xmlStrlen is used But for encoding strlen is used I think for encoding also, xmlStrlen can be used.
Hum, way too complex, the encoding string passed is an encoding name, the other string is fixed size. If the encoding string is over 1000 someone is doing nasty, just ignore the input, so I pushed a simpler patch just checking the encoding string length https://git.gnome.org/browse/libxml2/commit/?id=292a9f293decfcd1de8870d93866bf450f3f555f thanks for raising the issue ! Daniel