New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 709967 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Last visit > 30 days ago
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

multiple memory leak in dict.c

Reported by vvva...@gmail.com, Apr 10 2017

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce the problem:
https://chromium.googlesource.com/chromium/src.git/+/59.0.3067.2/third_party/libxml/src/dict.c

the vulnerability present in the dict.c file in xmlDictLookup function. 

xmlDictLookup function has 3 parameters and xmlDictPtr dict, const xmlChar *name, int len. And in the https://chromium.googlesource.com/chromium/src.git/+/59.0.3067.2/third_party/libxml/src/dict.c#839

don't checked this
len > strlen((const char *) name)
condition

```
root@ubuntu:/home/vah13/Desktop/libxml-cpp-wrapper# valgrind --leak-check=full --show-leak-kinds=all  ./uri_h in/textfile.txt 
==6206== Memcheck, a memory error detector
==6206== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==6206== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==6206== Command: ./uri_h in/textfile.txt
==6206== 
==6206== Invalid read of size 1
==6206==    at 0x402D3F3: strlen (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==6206==    by 0x8048D56: main (writer.cpp:72)
==6206==  Address 0x49651f1 is 0 bytes after a block of size 1 alloc'd
==6206==    at 0x402A6DC: operator new(unsigned int) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==6206==    by 0x8048D43: main (writer.cpp:30)
==6206== 
==6206== Invalid read of size 1
==6206==    at 0x458D0DC: xmlDictComputeFastKey (dict.c:451)
==6206==    by 0x458D0DC: xmlDictLookup (dict.c:851)
==6206==    by 0x8048D73: main (writer.cpp:72)
==6206==  Address 0x49861e1 is 132,273 bytes inside an unallocated block of size 4,190,904 in arena "client"
==6206== 
==6206== Invalid read of size 1
==6206==    at 0x458D11C: xmlDictComputeFastKey (dict.c:455)
==6206==    by 0x458D11C: xmlDictLookup (dict.c:851)
==6206==    by 0x8048D73: main (writer.cpp:72)
==6206==  Address 0x49651f9 is 8 bytes after a block of size 1 alloc'd
==6206==    at 0x402A6DC: operator new(unsigned int) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==6206==    by 0x8048D43: main (writer.cpp:30)

```

for the fix, I think, need change code like this 
src.git/+/59.0.3067.2/third_party/libxml/src/dict.c#839
```
xmlDictLookup(xmlDictPtr dict, const xmlChar *name, int len) {
    unsigned long key, okey, nbi = 0;
    xmlDictEntryPtr entry;
    xmlDictEntryPtr insert;
    const xmlChar *ret;
    unsigned int l;

    if ((dict == NULL) || (name == NULL))
        return(NULL);

--    if (len < 0)
++    if (len < 0 || len > strlen((const char *) name))
        l = strlen((const char *) name);
    else
        l = len;

    if (((dict->limit > 0) && (l >= dict->limit)) ||
        (l > INT_MAX / 2))
        return(NULL);

    /*
     * Check for duplicate and insertion location.
     */
    okey = xmlDictComputeKey(dict, name, l);

```

What is the expected behavior?

What went wrong?
I reported information about this bug to bugzilla.gnome.org 

https://bugzilla.gnome.org/show_bug.cgi?id=781120

Did this work before? No 

Does this work in other browsers? N/A

Chrome version: 57.0.2987.133  Channel: stable
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)
Flash Version: Shockwave Flash 25.0 r0

 

Comment 1 by vvva...@gmail.com, Apr 10 2017

test1.c
825 bytes View Download
test2.c
769 bytes View Download
poc.txt
1 bytes View Download

Comment 2 by vvva...@gmail.com, Apr 10 2017

upd:
os->Linux, Ubuntu 14.04
Cc: pbomm...@chromium.org rsch...@chromium.org dominicc@chromium.org
Labels: M-57
Owner: dominicc@chromium.org
Status: WontFix (was: Unconfirmed)
The caller of xmlDictLookup is clearly buggy by passing a length that is much longer than "name". While that optimization is there--pass the length if known--also using strlen doesn't make sense. It would be a nice simplification to remove the len parameter. It would be best to do this sort of work upstream in libxml2 and then Chromium will adopt the change by rolling libxml2 changes. See the libxml2 site for instructions about how to do that.

Sign in to add a comment