10 ms·

Nginx security advisory (CVE-2013-2028)

192 points by QUFB 13y ago

brokentone 13y ago
Wow, Nginx versions rapidly. A few weeks back I was current with "1.2.8-1.el6.ngx" and now 1.4.1-1.el6.ngx is in the repo. Meaning, the vulnerable versions were likely not out long.
- Shish2k 13y ago
  1.3 was the beta stream, and 1.2.8 is the most recent 1.2 release, so in those few weeks the only stable release you've missed is 1.4.0 (which may well have not been packaged at all, if the distro was waiting a few days for bugs like this to get ironed out)
Smrchy 13y ago
The official nginx ubuntu package has already been updated to 1.4.1
- pavs 13y ago
  Why is the server package so behind? I am on Ubuntu 12.04.2 LTS, I had use "add-apt-repository ppa:nginx/development" to get nginx/1.3.12, how do I get in to the latest release? Wiki says: Stable release 1.4.0 / 24 April 2013 Preview release 1.3.16 / 16 April 2013 How do I get into the latest stable bandwagon?
  - spindritf 13y ago
    Use their official repository[1] for the latest stable releases deb http://nginx.org/packages/ubuntu/ precise nginx deb-src http://nginx.org/packages/ubuntu/ precise nginx Those are updated very quickly. I haven't timed it but I'm yet to see an advisory before an update is available. [1] http://wiki.nginx.org/Install#Official_Debian.2FUbuntu_packages http://wiki.nginx.org/Install#Official_Debian.2FUbuntu_packa...
    - Shorel 13y ago
      Still no raring package. It seems I'm stuck with 1.2.6 for the time being.
    - pavs 13y ago
      Just for reference for others who might be interested: I had to install this public key from ubuntu server: sudo apt-key adv --keyserver keyserver.ubuntu.com --recv-keys ABF5BD827BD9BF62 Had to remove nginx-full and nginx-common. Then install nginx: apt-get install nginx Even though I told the installer to keep the default config (which it did) but over-written /etc/nginx/conf.d/default.conf The only change I had to make was port number, since I am using varnish changed my port to 8080 for nginx. Restarted nginx. nginx version: nginx/1.4.1 Thanks for the help!
      - ericcholis 13y ago
        I had the same trouble, pretty seamless upgrade otherwise.
      - lmcnearney 13y ago
        Another note: Packages provided by Ubuntu use /etc/nginx/sites-available/ and /etc/nginx/sites-available/ for storing site configs but the package directly from Nginx stores configs in /etc/nginx/conf.d/.
        intrawl 13y ago
        Thanks for pointing this out. I spent a little while trying to figure out why my configuration wasn't being recognized.
    - ab 13y ago
      Not sure what the schedule looks like in general, but last night I waited ~10 minutes after the advisory watching the packages show up one by one. Still faster than any other source, of course.
  - ck2 13y ago
    BTW compiling a custom nginx take all of 1 minute, it's a very very fast build, not like apache or php. I just wish it had a make test
  - egeozcan 13y ago
    I just build from source. Here's a sane guide from linode: http://library.linode.com/web-servers/nginx/installation/ubuntu-12.04-precise-pangolin http://library.linode.com/web-servers/nginx/installation/ubu...
- benmccann 13y ago
  Will the Ubuntu 12.04 LTS package get the security upgrade soon or do we need to add the nginx repo?
  - codexon 13y ago
    The problem affects nginx 1.3.9 - 1.4.0. Ubuntu 12.04.02 LTS uses Version: 1.1.19-1.
mef 13y ago
Incidentally, the affected source file (src/http/ngx_http_parse.c) is worth a read if you haven't before: http://hg.nginx.org/nginx/file/c72c5fbd1d07/src/http/ngx_http_parse.c http://hg.nginx.org/nginx/file/c72c5fbd1d07/src/http/ngx_htt...
- calinet6 13y ago
  Can you point out any highlights, or why you think it's worth a read, for those of us without the ability to look it over in detail?
- simfoo 13y ago
  That is the most cleanly written state machine I've ever seen in C. Beautiful code indeed!
  - chubot 13y ago
    Mmm, I would say it's definitely formatted beautifully. But I agree with the poster above in saying that it's error prone. I'm a big Nginx fan, but I think it's good by just sheer force of will... if you had like 10 people working on it it would probably fall apart. Something sqlite at least has extensive automated tests, which makes me much more confident about its quality.
- qqqqqq 13y ago
  I understand that they're doing string comparisons (of a kind), but what are those values defined in 'uint32_t'? Is it a translation of values into memory addresses?
  - tptacek 13y ago
    At the top of the file, in "usual"? They're bitfields of allowable characters.
    - qqqqqq 13y ago
      So /that's/ what they were for. Thanks!
avar 13y ago
I haven't given this more than a cursory look, but I love how the body message for the commit that seemingly introduced this flaw is "No functional changes.": https://github.com/git-mirror/nginx/commit/8a3f6ce https://github.com/git-mirror/nginx/commit/8a3f6ce Found with: git log --full-diff --reverse release-1.3.8.. -p src/http/ngx_http_parse.c
- tptacek 13y ago
  The change pulls chunked encoding parsing from the proxy to the core, but doesn't introduce new functionality. Also, are you sure this change introduces the flaw? It looks like it could have been there before the refactor, as well.
  - wulczer 13y ago
    Seems so, the code in question appears to have been added here: https://github.com/git-mirror/nginx/commit/7c7ad803a13d5f73b282e5a661ede3dcf9fc4aa1 https://github.com/git-mirror/nginx/commit/7c7ad803a13d5f73b...
- alcuadrado 13y ago
  Maybe I'm getting it wrong, but this comment seems so arrogant.. and with ngninx, that has one of the cleanest code bases of open source software actually used by many people :s
  - zzzcpan 13y ago
    It doesn't look arrogant to me. And it doesn't matter how clean the code looks like if it uses trillions of mutable states, lots of inconsistent interfaces, occasional pointer arithmetics, manual memory management, gotos all over the place and no module tests whatsoever. I'd say nginx is a pretty error prone piece of software, just like most of the software written in C.
    - nkozyra 13y ago
      Wait, "most of the software written in C" is "pretty error prone?" That's a pretty big statement right there.
xxpor 13y ago
It seems that Hacker News has turned off SPDY. Related?
- Tobu 13y ago
  SPDY is on (as of this comment anyway).
ericcholis 13y ago
Thanks to QUFB for submitting this, I would have likely missed this otherwise.
ancarda 13y ago
+ if (ctx->size < 0 || ctx->length < 0) { + goto invalid; + } + I don't know C/C++ but I was under the impression nobody used goto? Is there anything good/bad about using it?
- hderms 13y ago
  GOTO is often involved in error handling. If there are a bunch of different ways that code inside a function can fail, and the cleanup after failure involves tearing down the same objects, etc... then it's usually considered acceptable to use GOTO to target it.
- vault_ 13y ago
  It's not uncommon in C as an alternative to exceptions. You don't want to be using it for every-day flow control, but when you're checking results from syscalls repeatedly, using a gigantic nested if-else chain would get more messy than using goto. The other alternative is early return/exit calls, but then you have to remember to clean up everything each time before you return/exit.
- DasIch 13y ago
  In this case goto is used as a form of "exception" handling, which is a perfectly fine use case for goto if you have an imperative language like C without exceptions or exception handling mechanisms. In fact it would probably be bad style not to use goto in this case. If you have a better way to express the control flow you should use it instead though.
- shrughes 13y ago
  Why wouldn't you use goto? It's not like it isn't constrained to the function you're in.
- deleted 13y ago
  [deleted]
- Shorel 13y ago
  Simple minded programmers are banned from using goto. The reason is a lot of Basic programs from 1970 that lead to the term 'spaghetti code'. If you are an expert programmer, you can use it in some parts where efficiency is important. Finite State Machine implementations are a common use.
unix-junkie 13y ago
I don't get it, it seems to me that this fix is incorrect. It's pointless to check for such a signed integer overflow after it happened.
- lysium 13y ago
  Why is length stored with a signed integer in the first place?
  - tptacek 13y ago
    It's (mis)using off_t, which allows for negative offsets (for obvious reasons). You also don't need signed integers to have integer overflows.
- bcoates 13y ago
  I think you're right, unless nginx uses --fwrapv or it's equivalent on every compiler.
lowglow 13y ago
If you're running Nginx, We're having a SFHN meetup with Nginx on Thursday here in SF: http://sfhn-nginx.eventbrite.com/ http://sfhn-nginx.eventbrite.com/
FooBarWidget 13y ago
Phusion Passenger (Ruby/Python application server for Nginx) has been updated to use Nginx 1.4.1 by default: http://blog.phusion.nl/2013/05/07/phusion-passenger-4-0-2-released/ http://blog.phusion.nl/2013/05/07/phusion-passenger-4-0-2-re...

192 points by QUFB 13y ago

Wow, Nginx versions rapidly. A few weeks back I was current with "1.2.8-1.el6.ngx" and now 1.4.1-1.el6.ngx is in the repo. Meaning, the vulnerable versions were likely not out long.

Shish2k 13y ago
1.3 was the beta stream, and 1.2.8 is the most recent 1.2 release, so in those few weeks the only stable release you've missed is 1.4.0 (which may well have not been packaged at all, if the distro was waiting a few days for bugs like this to get ironed out)

Smrchy 13y ago

The official nginx ubuntu package has already been updated to 1.4.1

pavs 13y ago
Why is the server package so behind? I am on Ubuntu 12.04.2 LTS, I had use "add-apt-repository ppa:nginx/development" to get nginx/1.3.12, how do I get in to the latest release? Wiki says: Stable release 1.4.0 / 24 April 2013 Preview release 1.3.16 / 16 April 2013 How do I get into the latest stable bandwagon?
- spindritf 13y ago
  Use their official repository[1] for the latest stable releases deb http://nginx.org/packages/ubuntu/ precise nginx deb-src http://nginx.org/packages/ubuntu/ precise nginx Those are updated very quickly. I haven't timed it but I'm yet to see an advisory before an update is available. [1] http://wiki.nginx.org/Install#Official_Debian.2FUbuntu_packages http://wiki.nginx.org/Install#Official_Debian.2FUbuntu_packa...
  - Shorel 13y ago
    Still no raring package. It seems I'm stuck with 1.2.6 for the time being.
  - pavs 13y ago
    Just for reference for others who might be interested: I had to install this public key from ubuntu server: sudo apt-key adv --keyserver keyserver.ubuntu.com --recv-keys ABF5BD827BD9BF62 Had to remove nginx-full and nginx-common. Then install nginx: apt-get install nginx Even though I told the installer to keep the default config (which it did) but over-written /etc/nginx/conf.d/default.conf The only change I had to make was port number, since I am using varnish changed my port to 8080 for nginx. Restarted nginx. nginx version: nginx/1.4.1 Thanks for the help!
    - ericcholis 13y ago
      I had the same trouble, pretty seamless upgrade otherwise.
    - lmcnearney 13y ago
      Another note: Packages provided by Ubuntu use /etc/nginx/sites-available/ and /etc/nginx/sites-available/ for storing site configs but the package directly from Nginx stores configs in /etc/nginx/conf.d/.
      - intrawl 13y ago
        Thanks for pointing this out. I spent a little while trying to figure out why my configuration wasn't being recognized.
  - ab 13y ago
    Not sure what the schedule looks like in general, but last night I waited ~10 minutes after the advisory watching the packages show up one by one. Still faster than any other source, of course.
- ck2 13y ago
  BTW compiling a custom nginx take all of 1 minute, it's a very very fast build, not like apache or php. I just wish it had a make test
- egeozcan 13y ago
  I just build from source. Here's a sane guide from linode: http://library.linode.com/web-servers/nginx/installation/ubuntu-12.04-precise-pangolin http://library.linode.com/web-servers/nginx/installation/ubu...
benmccann 13y ago
Will the Ubuntu 12.04 LTS package get the security upgrade soon or do we need to add the nginx repo?
- codexon 13y ago
  The problem affects nginx 1.3.9 - 1.4.0. Ubuntu 12.04.02 LTS uses Version: 1.1.19-1.

mef 13y ago

Incidentally, the affected source file (src/http/ngx_http_parse.c) is worth a read if you haven't before: http://hg.nginx.org/nginx/file/c72c5fbd1d07/src/http/ngx_http_parse.c http://hg.nginx.org/nginx/file/c72c5fbd1d07/src/http/ngx_htt...

calinet6 13y ago
Can you point out any highlights, or why you think it's worth a read, for those of us without the ability to look it over in detail?
simfoo 13y ago
That is the most cleanly written state machine I've ever seen in C. Beautiful code indeed!
- chubot 13y ago
  Mmm, I would say it's definitely formatted beautifully. But I agree with the poster above in saying that it's error prone. I'm a big Nginx fan, but I think it's good by just sheer force of will... if you had like 10 people working on it it would probably fall apart. Something sqlite at least has extensive automated tests, which makes me much more confident about its quality.
qqqqqq 13y ago
I understand that they're doing string comparisons (of a kind), but what are those values defined in 'uint32_t'? Is it a translation of values into memory addresses?
- tptacek 13y ago
  At the top of the file, in "usual"? They're bitfields of allowable characters.
  - qqqqqq 13y ago
    So /that's/ what they were for. Thanks!

avar 13y ago

I haven't given this more than a cursory look, but I love how the body message for the commit that seemingly introduced this flaw is "No functional changes.": https://github.com/git-mirror/nginx/commit/8a3f6ce https://github.com/git-mirror/nginx/commit/8a3f6ce Found with: git log --full-diff --reverse release-1.3.8.. -p src/http/ngx_http_parse.c

tptacek 13y ago
The change pulls chunked encoding parsing from the proxy to the core, but doesn't introduce new functionality. Also, are you sure this change introduces the flaw? It looks like it could have been there before the refactor, as well.
- wulczer 13y ago
  Seems so, the code in question appears to have been added here: https://github.com/git-mirror/nginx/commit/7c7ad803a13d5f73b282e5a661ede3dcf9fc4aa1 https://github.com/git-mirror/nginx/commit/7c7ad803a13d5f73b...
alcuadrado 13y ago
Maybe I'm getting it wrong, but this comment seems so arrogant.. and with ngninx, that has one of the cleanest code bases of open source software actually used by many people :s
- zzzcpan 13y ago
  It doesn't look arrogant to me. And it doesn't matter how clean the code looks like if it uses trillions of mutable states, lots of inconsistent interfaces, occasional pointer arithmetics, manual memory management, gotos all over the place and no module tests whatsoever. I'd say nginx is a pretty error prone piece of software, just like most of the software written in C.
  - nkozyra 13y ago
    Wait, "most of the software written in C" is "pretty error prone?" That's a pretty big statement right there.

xxpor 13y ago

It seems that Hacker News has turned off SPDY. Related?

Tobu 13y ago
SPDY is on (as of this comment anyway).

ericcholis 13y ago

Thanks to QUFB for submitting this, I would have likely missed this otherwise.

ancarda 13y ago

+ if (ctx->size < 0 || ctx->length < 0) { + goto invalid; + } + I don't know C/C++ but I was under the impression nobody used goto? Is there anything good/bad about using it?

hderms 13y ago
GOTO is often involved in error handling. If there are a bunch of different ways that code inside a function can fail, and the cleanup after failure involves tearing down the same objects, etc... then it's usually considered acceptable to use GOTO to target it.
vault_ 13y ago
It's not uncommon in C as an alternative to exceptions. You don't want to be using it for every-day flow control, but when you're checking results from syscalls repeatedly, using a gigantic nested if-else chain would get more messy than using goto. The other alternative is early return/exit calls, but then you have to remember to clean up everything each time before you return/exit.
DasIch 13y ago
In this case goto is used as a form of "exception" handling, which is a perfectly fine use case for goto if you have an imperative language like C without exceptions or exception handling mechanisms. In fact it would probably be bad style not to use goto in this case. If you have a better way to express the control flow you should use it instead though.
shrughes 13y ago
Why wouldn't you use goto? It's not like it isn't constrained to the function you're in.
deleted 13y ago
[deleted]
Shorel 13y ago
Simple minded programmers are banned from using goto. The reason is a lot of Basic programs from 1970 that lead to the term 'spaghetti code'. If you are an expert programmer, you can use it in some parts where efficiency is important. Finite State Machine implementations are a common use.

unix-junkie 13y ago

I don't get it, it seems to me that this fix is incorrect. It's pointless to check for such a signed integer overflow after it happened.

lysium 13y ago
Why is length stored with a signed integer in the first place?
- tptacek 13y ago
  It's (mis)using off_t, which allows for negative offsets (for obvious reasons). You also don't need signed integers to have integer overflows.
bcoates 13y ago
I think you're right, unless nginx uses --fwrapv or it's equivalent on every compiler.

lowglow 13y ago

If you're running Nginx, We're having a SFHN meetup with Nginx on Thursday here in SF: http://sfhn-nginx.eventbrite.com/ http://sfhn-nginx.eventbrite.com/

FooBarWidget 13y ago

Phusion Passenger (Ruby/Python application server for Nginx) has been updated to use Nginx 1.4.1 by default: http://blog.phusion.nl/2013/05/07/phusion-passenger-4-0-2-released/ http://blog.phusion.nl/2013/05/07/phusion-passenger-4-0-2-re...