xorl %eax, %eax

CVE-2009-0478: Squid HTTP Version Remote DoS

leave a comment »

Another bug for one of the most popular proxy servers out there. This was reported by Joshua Morin, Mikko Varpiola and Jukka Taimisto from the CROSS project to the Squid project and disclosed on 2 February 2009. The affected versions are listed in the published advisory. The following code snippets where ripped of 2.7 (Stable 5) branch of the Squid Caching server. The buggy function is located at src/HttpMsg.c. This source code file contains the main parsing routines for HTTP messages and it was originally written by Alex Rousskov. The vulnerable code can be found on this function:

159  int
160  httpMsgParseRequestLine(HttpMsgBuf * hmsg)
161  {

This function is responsible for parsing the request line of an HTTP message, the author is probably suspicious for some off-by-one errors too…

155   * TODO:
156   *   * have it indicate "error" and "not enough" as two separate conditions!
157   *   * audit this code as off-by-one errors are probably everywhere!
158   */

So, if you read it carefully you might end up with one or more nice 0days for Squid. Not bad… Not bad at all! Anyway, let’s examine the bug. An HTTP request line has the following format:

    HTTP/1.0 200 OK

Where the first token represents the protocol/version and the proceeding number, the HTTP code returned plus its equivalent string. The version can be separated to major, which in this case is 1 and minor which is 0. Now, let’s move to httpMsgParseRequestLine() and see what happens there:

162      int i = 0;
163      int retcode;
164      int maj = -1, min = -1;
      ...
257              /* next should be 1 or more digits */
258              maj = 0;
259              for (; i < hmsg->req_end && (xisdigit(hmsg->buf[i])); i++) {
260                  maj = maj * 10;
261                  maj = maj + (hmsg->buf[i]) - '0';
262              }
      ...
296      assert(maj != -1);

Here is the first vulnerability. The above code retrieves the major number. If it finds it, it multiplies it by 10 and then updates it until it either reaches the end of the request, or it finds something that it is not a digit. This means, that if we enter a negative number it will be inserted to maj which is a signed integer as defined at line 164. And when it reaches the assert(3) call at line 296 it will cause the following error:

HttpMsg.c:296: httpMsgParseRequestLine: Assertion `maj != -1' failed.
Aborted

This was patched simply by doing:

         maj = 0;
-        for (; i < hmsg->req_end && (xisdigit(hmsg->buf[i])); i++) {
+        for (; i < hmsg->req_end && (xisdigit(hmsg->buf[i])) && maj < 65536; i++) {
         maj = maj * 10;
         maj = maj + (hmsg->buf[i]) - '0';
         }
-        if (i >= hmsg->req_end) {
+        if (i &gt;= hmsg->req_end || maj >= 65536) {
         retcode = -1;

This ensures that maj cannot be larger than 65535 (0xffff). The next assertion was on the parsing of the minor version number which similarly was doing this:

276              /* next should be one or more digits */
277              i++;
278              min = 0;
279              for (; i < hmsg->req_end && (xisdigit(hmsg->buf[i])); i++) {
280                  min = min * 10;
281                  min = min + (hmsg->buf[i]) - '0';
282              }
      ...
297      assert(min != -1);

It’s exactly the same vulnerability so there is no need to discuss it. The fix of this was:

<pre>         min = 0;
-        for (; i < hmsg->req_end && (xisdigit(hmsg->buf[i])); i++) {
+        for (; i < hmsg->req_end && (xisdigit(hmsg->buf[i])) && min < 65536; i++) {
         min = min * 10;
         min = min + (hmsg->buf[i]) - '0';
         }
-
+        if (maj >= 65536) {
+        retcode = -1;
+        goto finish;
+        }
         /* Find whitespace, end of version */

Well, you can trigger this vulnerability simply by sending an HTTP request line that has a negative major and/or minor version number. It is no more than 10 lines of code in most programming languages. These bugs are really pissing me off. Most of my friends already know that for at least 2+ years I’m a big supporter of ilja’s opinion on assert(3) calls for production code which you can read at his blag. In addition to this, Squid is just full of assertions… Imagine, a crappy parsing like this can lead to a remote DoS.. How retarded is this?

Written by xorl

February 11, 2009 at 19:37

Posted in bugs

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s