

Nginx security advisory (CVE-2013-2028) - QUFB
http://mailman.nginx.org/pipermail/nginx-announce/2013/000112.html

======
avar
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>

Found with:

    
    
        git log --full-diff --reverse release-1.3.8.. -p src/http/ngx_http_parse.c

~~~
alcuadrado
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
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.

~~~
paraboul
Sorry but that's a really stupid comment.

You are just saying that C is not clean. (IMHO) nginx code-base is doing a
really good job dealing with micro-optimization.

 _> code looks like if it uses trillions of mutable state_

Actually, that's a very good approach. e.g. : nginx parser get optimized out
as a big jump table (<http://en.wikipedia.org/wiki/Branch_table>)

~~~
zzzcpan
I didn't mean nginx parser and state machines in general as mutable states,
but rather state changes in variables across multiple functions and files.
They are impossible to keep track of and nginx is full of them.

------
mef
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_htt...](http://hg.nginx.org/nginx/file/c72c5fbd1d07/src/http/ngx_http_parse.c)

~~~
SoftwareMaven
Are these supposed to do different things on different architectures, or do
they magically do the same thing without comparing C1 in the "else" case?

    
    
      #if (NGX_HAVE_LITTLE_ENDIAN && NGX_HAVE_NONALIGNED)
      #define ngx_str3Ocmp(m, c0, c1, c2, c3).   \
           *(uint32_t *) m == ((c3 << 24) | (c2 << 16) | (c1 << 8) | c0)
      #else /* !(NGX_HAVE_LITTLE_ENDIAN && NGX_HAVE_NONALIGNED) */
      #define ngx_str3Ocmp(m, c0, c1, c2, c3)    \
           m[0] == c0 && m[2] == c2 && m[3] == c3

~~~
sltkr
If you look at where they are used, then it looks like these macros do not
need to compare m[1] to c1 because whenever they are used it is already known
that m[1] == 'O' (hence "str3O" in the name -- that's 3 and the letter O, not
number zero).

I guess c1 is included in the first case because it's faster to compare the
whole 32-bit word than mask out the known byte (and since c0, c1, c2 and c3
are compile time constants, the whole right-hand-side of the comparison will
be optimized by constant folding anyway).

(It's ridiculous micro-optimization in my opinion, but at least it is correct,
and it is limited to a single file.)

------
Smrchy
The official nginx ubuntu package has already been updated to 1.4.1

~~~
pavs
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
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_packa...](http://wiki.nginx.org/Install#Official_Debian.2FUbuntu_packages)

~~~
pavs
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!

~~~
lmcnearney
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
Thanks for pointing this out. I spent a little while trying to figure out why
my configuration wasn't being recognized.

------
unix-junkie
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
Why is length stored with a signed integer in the first place?

~~~
tptacek
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.

------
ancarda

        +    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?

~~~
DasIch
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.

------
FooBarWidget
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-re...](http://blog.phusion.nl/2013/05/07/phusion-
passenger-4-0-2-released/)

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

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

------
brokentone
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
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)

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

~~~
Tobu
SPDY is on (as of this comment anyway).

