Sigil-Ebook / flightcrew

Automatically exported from code.google.com/p/flightcrew
GNU General Public License v3.0
34 stars 11 forks source link

XMLUni::fgXercesLoadSchema global-buffer-overflow #57

Closed De4dCr0w closed 4 years ago

De4dCr0w commented 4 years ago

There is a oob access in XMLString::compareIStringASCII , if psz1=psz2=XMLUni::fgXercesLoadSchema, it will obb access XMLUni::fgXercesLoadSchema by psz1++

int XMLString::compareIStringASCII( const XMLCh* const str1 , const XMLCh* const str2) { …… // Move upwards to next chars psz1++; psz2++; …… It was detected by ASAN: image

kevinhendricks commented 4 years ago

This routine is meant to compare strings of shorts (16 bit values) each with a full null string terminator at the end. That null string terminator must also be a short. A single null byte is not a correct terminator for these strings.

The code will escape the loop if any difference in char values or if not, the full null short is reached (the string terminator).

Is your fuzzer correctly generating "strings" for this routine.

Even the error output says that oob is located "0 bytes to the right" of the string you used. How is that out of bounds?

kevinhendricks commented 4 years ago

In fact that routine should terminate pointing at the 16 bit null value immediately at the end of that string.

Could this be an issue with your compiler or the alignment of short data? What system and compiler are you testing with?

De4dCr0w commented 4 years ago

In fact that routine should terminate pointing at the 16 bit null value immediately at the end of that string. but it will "psz1++; psz2++;" in the end and start the loop again , then break the loop because ch1 is null. so it will oob read once in the above process. Testing environment: Ubuntu 20.04, gcc version 9.3.0 with ASAN flightcrew can work well if I compile it without ASAN, so it's just a small problem.

kevinhendricks commented 4 years ago

Walk the loop: Here is the code:

XMLCh ch1;
    XMLCh ch2;

    for (;;) {
        if (*psz1 >= chLatin_A && *psz1 <= chLatin_Z)
            ch1 = *psz1 - chLatin_A + chLatin_a;
        else
            ch1 = *psz1;
        if (*psz2 >= chLatin_A && *psz2 <= chLatin_Z)
            ch2 = *psz2 - chLatin_A + chLatin_a;
        else
            ch2 = *psz2;

        // If an inequality, then return difference
        if (ch1 != ch2)
            return int(ch1) - int(ch2);

        // If either ended, then both ended, so equal
        if (!ch1)
            break;

        // Move upwards to next chars
        psz1++;
        psz2++;
    }
    return 0;
}

After comparing the character, if not equal, the return happens.

if the last char is a null, it will immediately break out of the loop (see the break statement).

So no increment and no accessing the next char happens after the null unless the compiler has unrolled the loop incorrectly, or there is compiler/access data alignment issue.

Please explain how it accesses an oob character.

Accessing the null terminal character itself is not an oob condition.

At least, I can not see the issue. So please walk me through it.

kevinhendricks commented 4 years ago

Closing this since as far as I can tell this error/warning is not correct. I am happy to be proven otherwise but really want proof.