Ticket #471 (closed Bug report: fixed)

Opened 10 years ago

Last modified 4 years ago

PATCH: libhttpd crash on invalid HTTP headers

Reported by: anonymous Owned by: wichert
Priority: high Milestone: Gateway 1.1.5
Component: Gateway Version:
Keywords: Cc:

Description

In libhttpd/api.c, line 532 you write:

cp = index(buf,':') + 2; if(cp) {

This is a really nasty bug: if index() returns NULL, cp becomes 2 and if(cp) only fails if index returns -2. This will never happen. Therefore you should write something like this:

cp = index(buf,':'); if(cp) {

cp += 2;

Greetings, CK

Attachments

ticket471.2.patch Download (1.5 KB) - added by gbastien 8 years ago.
Patch for second part of the bug

Change History

Changed 10 years ago by anonymous

What about this bug? It's really critical. Had non-reproducable crashes and it caused me about 2 hours to find it.

Changed 10 years ago by anonymous

This bug caused our wifidog gateway (>1000 users) to crash every 5 to 20 minutes (non-reproducable, too). After patching, the gateway works like a charm.

Changed 10 years ago by networkfusion

  • component changed from Auth server, Feature request to Gateway
  • milestone changed from Not yet assigned to a Milestone to Gateway 1.1.5

Changed 10 years ago by wichert

  • owner set to wichert
  • priority changed from low to high

Your change does not make sense: Since the strncasecmp() call returned a match it is impossible for index to return anything but a sensible index. If it returns NULL the only possible reason is that your machine has bad memory which happened to flip a bit.

In fact since we know that strncasecmp returned a match we already know the exact index of the colon, so the whole test is senseless.

What would make sense is to test for *cp instead, ie test if there is an actual value specified in the HTTP host header. Could it be that you have incoming requests that contain an empty Host header?

Changed 10 years ago by wichert

  • summary changed from Bug in libhttpd/api.c, line 532 to libhttpd crash on invalid HTTP headers

Changed 10 years ago by anonymous

I dunno. It's not my machine, I only searched for the bug. I didn't look for the sense of the code, I saw what the error was: index() (sic! not strncasecmp()) returned NULL and therefore cp was 2 and passed if(cp). That led to memory access at address 0 causing a segfault.

Changed 10 years ago by anonymous

Memory access at address 2! not 0!

Changed 9 years ago by jean-philippe.menil@…

I've submitted core debug on this ticket (527). The workaround fix the problem, who appear only with a lot of users.

Changed 9 years ago by benoitg

  • cc spam-11-2007@… removed
  • summary changed from libhttpd crash on invalid HTTP headers to PATCH: libhttpd crash on invalid HTTP headers

Changed 9 years ago by benoitg

  • status changed from new to closed
  • resolution set to fixed

Fixed in [1423]

Changed 8 years ago by jean-philippe.menil@…

Hi,
i reopen the ticket due to the following bug:
Core was generated by `/usr/bin/wifidog -s -c /etc/wifidog/XX.conf -w /tmp/wdctl-XX.sock'.
Program terminated with signal 11, Segmentation fault.
[New process 8650]
[New process 8651]
[New process 8652]
[New process 8656]
[New process 8654]
[New process 10017]
[New process 8657]
[New process 10019]
[New process 8659]
[New process 8655]
[New process 8658]
[New process 10018]
[New process 8653]
[New process 9906]
#0 0x00007f1f30def82c in httpdReadRequest (server=0x143a170, r=0x7f1f200072d0) at api.c:493.[[BR]] 493 if (strncmp(cp,"Basic ", 6) != 0)
(gdb) where
#0 0x00007f1f30def82c in httpdReadRequest (server=0x143a170, r=0x7f1f200072d0) at api.c:493[[BR]] #1 0x000000000040b41a in thread_httpd (args=<value optimized out>) at httpd_thread.c:61[[BR]] #2 0x00007f1f309bd73a in pthread_create@@GLIBC_2.2.5 () from /lib/libpthread.so.0
#3 0x0000000000000000 in ?? ()

I can't reproduce it, as i can't reproduce the bug above.
It happens rarely, but it crash the gateway.

Changed 8 years ago by gbastien

Patch for second part of the bug

Changed 8 years ago by gbastien

  • status changed from closed to reopened
  • resolution fixed deleted

The fix applied for this bug was done in a "if" block starting with

if (strncasecmp(buf,"Host: ",6) == 0) (line 533)

The line mentioned here is from a different block with the same pattern starting with

if (strncasecmp(buf,"Authorization: ",15) == 0) (line 493)

A quick guess would be to do the same fix to that other block of code.

Here's a patch (untested for now)

Changed 8 years ago by jean-philippe.menil@…

Ok, alse expected, it seems to do the trick.

Changed 4 years ago by benoitg

  • status changed from reopened to closed
  • resolution set to fixed
Note: See TracTickets for help on using tickets.