Nginx SPDY heap buffer overflow (affects 1.3.15 – 1.5.11) | Hacker News Reader

Nginx SPDY heap buffer overflow (affects 1.3.15 – 1.5.11)

nginx.org

86 pointsjvt29 comments

Before you panic: "The problem affects nginx 1.3.15 - 1.5.11, compiled with the ngx_http_spdy_module module (which is not compiled by default) and without --with-debug configure option, if the "spdy" option of the "listen" directive is used in a configuration file."

Well, SPDY is enabled by default on 1.5.11 from what I understand, so they better push out 1.5.12 out (at least not seeing it yet)

Patch is pretty interesting. Why was buffer overflow protection behind a debug flag?

http://nginx.org/download/patch.2014.spdy2.txt

Usual reason for mistakes like this is: someone was debugging something then (partially) forgot to clean up before committing.

Once that is done it's incredibly easy for these kind of bugs to go un-noticed for a long time.

Assuming you don't have a test for it, sure. And don't fuzz everything on a regular basis.

And why leave in a #if when patching too?

Keeps the changes to a minimum, reducing the risk of effecting anything else. (quicker + safer)

I assume the reason was to keep diff as minimal as possible. Note that it should apply to all mentioned versions without problems.

Without context (I'm not familiar with nginx' code) the blatant subtraction of pointers was scary, too.

As everyone is of course no doubt aware, subtraction of pointers is only valid in C if you can guarantee that the pointers point to the same "object" (or at most 1 character past the end of an object).

I would prefer an interface using a size_t count of available space, rather than passing an end-pointer around.

Again, this is based solely on reading the patch, so it might be perfectly fine. Considering nginx' reputation, I guess it's academic.

It's a common practice to use two pointers when parsing some stream data buffer. If you will use a pointer to the start of the buffer and the size of an available data to parse, then you would need to change two variables instead of one on every parsing stage. So you have more opportunity to make a mistake.

It's kind of implied by the parameters that they point to the same object; they're named "pos" and "end" after all. But I agree, a count variable is nicer.

The subtraction also has a (very theoretical) problem that its result must fit in ptrdiff_t (signed), and it would produce undefined behavior when you have a huge object whose size doesn't fit in ptrdiff_t (but does in size_t, by definition).

  -        if (ngx_list_init(&r->headers_in.headers, r->pool, sc->entries + 3,
  +        if (ngx_list_init(&r->headers_in.headers, r->pool, 20,

Personally, both the old code and the new code use magic numbers, and both send up a red flag to me. What does 20 mean? (I don't personally need an answer here: my point is it's not obvious and therefore it's easier for bugs to get through.)

I wonder if this discovery is a result of OpenBSD switching its focus from Apache to Nginx ?

It wasn't.

> Thanks to Lucas Molas, researcher at Programa STIC, Fundación Dr. Manuel Sadosky, Buenos Aires, Argentina.

I assume this only affects configurations with ngx_http_spdy_module enabled. Can anyone confirm whether or not that's the case?

Yup, it's in the announcement: http://mailman.nginx.org/pipermail/nginx-announce/2014/00013...

The problem affects nginx 1.3.15 - 1.5.11, compiled with the ngx_http_spdy_module module (which is not compiled by default) and without --with-debug configure option, if the "spdy" option of the "listen" directive is used in a configuration file.

It said it affected 1.3.15 to 1.5.11 but the fix is available in nginx 1.5.12, 1.4.7.

So for those who are using legacy version are they going to rely on distro vendor to push the patch? Just curious, even though I guess the number of users who have activated this experimental SPDY is low and people who actually have it enabled probably know how to fix it themselves.

1.5.12 is the latest devel branch and 1.4.7 is the latest release branch (released in response to this CVE and excepted from the vuln range). The patch works for any of the intervening versions.

As far as I'm aware, all the major distros backport nginx stable release

Why was it replaced with "#if 1" instead of just deleting the entire "if/end" delimiters? Won't that always evaluate to true?

Maybe they wanted a patch that could be easily applied to older versions. It might have an elif later too, although I'm guessing that's not too common after debug blocks.

To keep patch small. Now #if 1 is removed: http://hg.nginx.org/nginx/rev/b93db4e4ff77

It appears my Ubuntu server only updates nginx mainline to 1.5.11. I've disabled spdy support in my listen directive for now as an easier work around.

It's not affected on Debian/Ubuntu, check --with-debug flag.

Thank you.

btw, I use packages from nginx.org, they're updated in time with every release: http://nginx.org/en/linux_packages.html

Do the upstream packages still use a different configuration path from the Debian/Ubuntu packages?

Yes. There's no sites-available or sites-enabled, and some configuration is split off into different files in the conf.d directory.

Got to love C.

Almost as much as I love Java

Reply on news.ycombinator.com