Skip to content

Instantly share code, notes, and snippets.

@nickrolfe
Created October 31, 2018 12:10
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save nickrolfe/9d0c574bb8a7852f612047835d867e3d to your computer and use it in GitHub Desktop.
Save nickrolfe/9d0c574bb8a7852f612047835d867e3d to your computer and use it in GitHub Desktop.

I spotted a vulnerability in Icecast, the open-source streaming media server maintained by the Xiph.org Foundation. Attackers could craft HTTP headers that would overwrite the server's stack contents, leading to remote code execution. Since Icecast is commonly used to host internet radio stations, a motivated attacker could potentially take a station off air.

Icecast servers running version 2.4.3 (or earlier) and using URL authentication are affected, and should be upgraded to version 2.4.4 as soon as possible.

The vulnerability has been assigned the identifier CVE-2018-18820.

snprintf: a safer version of sprintf?

At this point, it's well known that sprintf is unsafe, since it provides no protection against buffer overflow. It's not unusual to see documentation that points users to snprintf as a safer version, since it will truncate the output if the buffer is too small. But what people often don't realize is that, when it truncates, snprintf does not return the number of bytes it wrote. In fact, it returns the number of bytes it would have written if the output buffer were big enough. Worse still, it can't do anything to protect you against buffer overflow if you provide a size argument that's larger than the actual size of the buffer.

Here is the vulnerable code from Icecast, where it loops over HTTP headers from a user request and copies them into a buffer, building the body of a POST request to send to an authentication server:

post_offset += snprintf(post + post_offset,
                        sizeof(post) - post_offset,
                        "&%s%s=%s",
                        url->prefix_headers ? url->prefix_headers : "",
                        cur_header, header_valesc);

Let's look at a slightly simplified version of this code:

post_offset += snprintf(post + post_offset,
                        sizeof(post) - post_offset,
                        "%s",
                        cur_header);

Let's consider a situation where sizeof(post) is 10, and 8 bytes have already been written (i.e., post_offset is 8). What happens when the next header to be copied is "baz"?

The output gets truncated, but post_offset gets incremented beyond the end of the buffer:

Now let's imagine there's another header to be copied, with contents "AAAAA...". We are in an interesting position: the size argument to snprintf is sizeof(post) - post_offset, which will underflow to become an enormously large number. The result is that subsequent calls to snprintf can effectively write as much data as they want. That data will be written to post + post_offset, somewhere beyond the end of the post buffer, and will overwrite other contents of the stack.

This means we can send one long HTTP header that will get truncated, but whose length will allow us to position post_offset anywhere in the stack we choose. Then we can send a second HTTP header whose contents will be written to that location.

One difficulty for an attacker is that some sanitization of the headers is performed before they are copied with snprintf, so they are somewhat limited in what data they can write to the stack. My proof-of-concept exploit only caused a segfault in the server process - effectively a denial-of-service attack - but I suspect a sufficiently motivated and clever attacker would be able to upgrade this attack to achieve full-blown remote code execution.

The fix

The folks at Xiph patched the bug quickly, and the fix is pretty simple. They simply check the return value from snprintf, and, if it would cause post_offset to point beyond the end of the buffer, log an error and exit the loop. Otherwise, add it to post_offset and continue, as before.

LGTM: the automated vulnerability finder

We automatically analyze thousands of open-source projects on LGTM, and one of our standard queries produces alerts for suspicious uses of snprintf that seem to match this pattern. That is, where the size argument is derived from the return value of a previous call to snprintf.

My coworker Kevin Backhouse has written in the past about how this query caught a vulnerability in rsyslog, and this vulnerability in Icecast was similar. The alerts were hidden in plain sight, simply waiting for a human to notice them.

If you manage or contribute to an open-source project, I encourage you to enable LGTM's PR integration so you can can notice bugs like this before they get merged into your codebase.

Disclosure timeline

  • 2018-10-16: vulnerability and proof-of-concept exploit disclosed to Xiph.
  • 2018-10-16: bug acknowledged by Xiph.
  • 2018-10-31: patched version (2.4.4) released.
Display the source blob
Display the rendered blob
Raw
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Display the source blob
Display the rendered blob
Raw
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Display the source blob
Display the rendered blob
Raw
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment