Nginx security advisory (CVE-2013-2028) | Hacker News Reader

Nginx security advisory (CVE-2013-2028)

mailman.nginx.org

192 pointsQUFB54 comments

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

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.

Seems so, the code in question appears to have been added here:

https://github.com/git-mirror/nginx/commit/7c7ad803a13d5f73b...

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

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.

Wait, "most of the software written in C" is "pretty error prone?"

That's a pretty big statement right there.

IMO, "most of the software written in any language" would be a good fit for that sentence.

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)

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.

To me "no functional changes" just means that the commit does not mean to introduce/change any functionality, for example if you refactor code or clean up.

   * Has functional change + introduces bugs

   * No functional changes + introduces bugs

   * Has functional changes + doesn't introduce bugs

   * No functional changes + doesn't introduce  bugs

Also I haven't seen too many commit that say "this introduces 5 new accidental bugs into the system". Just saying, not sure what ou are criticizing here

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

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?

That is the most cleanly written state machine I've ever seen in C. Beautiful code indeed!

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.

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?

At the top of the file, in "usual"? They're bitfields of allowable characters.

So /that's/ what they were for. Thanks!

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

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

That's because c1 is always 'O' (in order to compare things like : 'POST', 'COPY', 'MOVE', etc.)

In the first case, comparing 2 integer (4x 8bit) is faster than omitting one byte.

That's is, using ngx_str3Ocmp with c1 != 'O' would be a usage error.

Edit: sltkr, sorry didn't see your reply.

I spend most of my time in dynamic languages so I have to ask... is this that much faster than a good regex library that it warrants a hand rolled state machine? How normal is something like this in a typical C/C++ codebase?

People (should) only implement things in C/C++ after thoroughly profiling the dynamic language POC and being sure they need the performance that a C implementation affords. So yes, the C implementation is going to sacrifice readability for performance. Of course, there are better and worse ways of accomplishing the same thing.

A "good" regex library will dynamically generate code; and if the regex is simple enough, generate code that implements a DFA rather than NFA (or PDA, for Perl regexes). So for a "good" library, no, it won't be much faster.

But very few regex libraries are that "good", because it's a combination of extreme speed with very low flexibility (DFA has exponential state explosion in worst cases compared to equivalent NFA, and isn't able to deal with e.g. backreferences). The vast majority of good regex libraries will be an order of magnitude slower. Average regex libraries included in most language distributions will be slower again.

The chief exception is compiler lexer generators like lex and flex. They produce code very similar to the state machine linked. And that's probably the most common place to see this kind of thing.

Encoding the state of the machine implicitly as the program counter, rather than an explicit state variable, often results in more readable code and is the more usual way to do it when writing it by hand. It also saves a register, important on some architectures. But the technique has slightly more limited expressiveness owing to needing to stick with structured programming constructs.

The official nginx ubuntu package has already been updated to 1.4.1

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?

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

Still no raring package.

It seems I'm stuck with 1.2.6 for the time being.

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!

I had the same trouble, pretty seamless upgrade otherwise.

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/.

Thanks for pointing this out. I spent a little while trying to figure out why my configuration wasn't being recognized.

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.

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

I just build from source. Here's a sane guide from linode: http://library.linode.com/web-servers/nginx/installation/ubu...

Will the Ubuntu 12.04 LTS package get the security upgrade soon or do we need to add the nginx repo?

The problem affects nginx 1.3.9 - 1.4.0.

Ubuntu 12.04.02 LTS uses Version: 1.1.19-1.

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.

Why is length stored with a signed integer in the first place?

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.

I think you're right, unless nginx uses --fwrapv or it's equivalent on every compiler.

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

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.

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.

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.

Why wouldn't you use goto? It's not like it isn't constrained to the function you're in.

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.

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

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

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

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.

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)

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

SPDY is on (as of this comment anyway).

Reply on news.ycombinator.com