1

while reading on how, and what is buffer overflow on the owasp page I saw documentation on a buffer overflow in the LibPNG

It said the following code is vulnerable:

if (!(png_ptr->mode & PNG_HAVE_PLTE)) {
    /* Should be an error, but we can cope with it */
    png_warning(png_ptr, "Missing PLTE before tRNS");
}
else if (length > (png_uint_32)png_ptr->num_palette) {
    png_warning(png_ptr, "Incorrect tRNS chunk length");
    png_crc_finish(png_ptr, length);
    return;
}
...
png_crc_read(png_ptr, readbuf, (png_size_t)length);

I am having trouble understanding why it's vulnerable.

  • 1
    The website provides a couple of paragraphs explaining why it might be vulnerable to a buffer overflow. Can you detail what part of their explanation is confusing? – RoraΖ Feb 20 '17 at 18:14
  • I read the websites paragraphs on it but i still couldn't understand it. – Fumerian Gaming Feb 20 '17 at 18:21
  • @FumerianGaming Are you aware why length checks are important? – Arminius Feb 20 '17 at 18:22
  • yes I am fully aware of why they are important. Bounds checking is used to make sure the input string doesn't overflow the buffer. – Fumerian Gaming Feb 20 '17 at 18:33
2

From the wiki article you linked:

The code appears to safely perform bounds checking because it checks the size of the variable length, which it later uses to control the amount of data copied by png_crc_read(). However, immediately before it tests length, the code performs a check on png_ptr->mode, and if this check fails a warning is issued and processing continues. Because length is tested in an else if block, length would not be tested if the first check fails, and is used blindly in the call to png_crc_read(), potentially allowing a stack buffer overflow.

The code appears to check if the length is shorter than the buffer. But the length checking is done in an else if block. When png_ptr->mode & PNG_HAVE_PLTE is false, a warning is logged and program execution continues without performing the else branch and thus without checking length > (png_uint_32)png_ptr->num_palette. That means in any situation where the "Missing PLTE before tRNS" warning is generated, the length is passed to png_crc_read without ever being checked.

A plausible explanation for how this bug could have been introduced is that "Missing PLTE before tRNS" used to be an error-condition which aborted the whole function. In that case there was no reason to bother what would happen afterwards. But some time later someone reduced it to a warning-condition without checking what that meant for any subsequent code.

Edit: Another possibility for a buffer overflow in the above code could be if the value of png_ptr->num_palette comes from an untrustworthy source. But the example doesn't mention where it comes from, so I doubt that's what this example wants to illustrate.

  • 1
    For reference, here is the patched function. – Arminius Feb 20 '17 at 18:38
  • I would also like to use this opportunity to reiterate my favorite advise for C security: don't write security-relevant code in C. Use a modern programming language which doesn't even allow you to do stupid things like reading and writing outside of array boundaries. If that's not an option, at least use C++, but use it without arrays. Use std::string and std::vector instead. – Philipp Feb 20 '17 at 18:52
1

I'll answer with comments within the example.

If for some reason the first if evaluates to be TRUE then all that happens is a warning is logged.

if (!(png_ptr->mode & PNG_HAVE_PLTE)) {
    png_warning(png_ptr, "Missing PLTE before tRNS");
}

However, if it evaluates TRUE then the following length check will never be run because the else-if block has been resolved.

else if (length > (png_uint_32)png_ptr->num_palette) {
    png_warning(png_ptr, "Incorrect tRNS chunk length");
    png_crc_finish(png_ptr, length);
    return;
}

Now when the png_crc_read is called, it's possible that no length check was ever made.

...
png_crc_read(png_ptr, readbuf, (png_size_t)length);

This is a vulnerability if the mode can be controlled by the attacker to force the code path to skip over the length check. The attacker can then provide a larger length than should be possible, causing a buffer overflow. Depending on where this overflow happens it may or may not be exploitable.

  • but isnt the length checked in the the 3 parameter of png_crc_read. – Fumerian Gaming Feb 20 '17 at 19:44
  • No that is just a cast to a different data type. The length check is in the else-if – RoraΖ Feb 20 '17 at 20:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.