Project

General

Profile

Actions

Bug #2559

closed

accesslog_append_escaped does not hex-escape properly

Added by TheJH about 10 years ago. Updated over 8 years ago.

Status:
Fixed
Priority:
Normal
Category:
mod_accesslog
Target version:
ASK QUESTIONS IN Forums:

Description

When accesslog_append_escaped hex-escapes a character, it incorrectly uses the type "char" instead of "unsigned char", meaning that on typical systems, the offset relative to 'A' or '0' can become negative. Not a very big issue though because the resulting characters are still in the allowed range – not dangerous, just weird. :D

Here is a simple PoC:

$ echo -ne 'GET /blah HTTP/1.1\r\nHost: blah\r\nUser-Agent: \xad\xbe\r\n\r\n' | nc ::1 80
HTTP/1.1 404 Not Found
Content-Type: text/html
Content-Length: 345
Date: Wed, 05 Mar 2014 04:52:08 GMT
Server: lighttpd/1.4.34

Then look into the access log:

::1 blah - [05/Mar/2014:05:52:08 +0100] "GET /blah HTTP/1.1" 404 345 "-" "\x+-\x,." 

The fix is pretty straightforward, just cast the input chars to unsigned before splitting them into two parts with / and %:

-----------------------------------------------------------------------
diff -rpNU 8 lighttpd-1.4.34/src/mod_accesslog.c lighttpd-1.4.34-logescape/src/mod_accesslog.c
--- lighttpd-1.4.34/src/mod_accesslog.c 2013-11-13 18:30:52.000000000 +0100
+++ lighttpd-1.4.34-logescape/src/mod_accesslog.c       2014-03-05 06:01:34.683818499 +0100
@@ -199,19 +199,19 @@ static void accesslog_append_escaped(buf
                                BUFFER_APPEND_STRING_CONST(dest, "\\t");
                                break;
                        case '\v':
                                BUFFER_APPEND_STRING_CONST(dest, "\\v");
                                break;
                        default: {
                                        /* non printable char => \xHH */
                                        char hh[5] = {'\\','x',0,0,0};
-                                       char h = c / 16;
+                                       char h = ((unsigned char)c) / 16;
                                        hh[2] = (h > 9) ? (h - 10 + 'A') : (h + '0');
-                                       h = c % 16;
+                                       h = ((unsigned char)c) % 16;
                                        hh[3] = (h > 9) ? (h - 10 + 'A') : (h + '0');
                                        buffer_append_string_len(dest, &hh[0], 4);
                                }
                                break;
                        }
                }
        }
-----------------------------------------------------------------------
Actions #1

Updated by TheJH about 10 years ago

Argh, stupid formatting. This was the access log entry:

::1 blah - [05/Mar/2014:05:52:08 +0100] "GET /blah HTTP/1.1" 404 345 "-" "\x+-\x,." 

And here is the patch again:

diff -rpNU 8 lighttpd-1.4.34/src/mod_accesslog.c lighttpd-1.4.34-logescape/src/mod_accesslog.c
--- lighttpd-1.4.34/src/mod_accesslog.c 2013-11-13 18:30:52.000000000 +0100
+++ lighttpd-1.4.34-logescape/src/mod_accesslog.c       2014-03-05 06:01:34.683818499 +0100
@@ -199,19 +199,19 @@ static void accesslog_append_escaped(buf
                                BUFFER_APPEND_STRING_CONST(dest, "\\t");
                                break;
                        case '\v':
                                BUFFER_APPEND_STRING_CONST(dest, "\\v");
                                break;
                        default: {
                                        /* non printable char => \xHH */
                                        char hh[5] = {'\\','x',0,0,0};
-                                       char h = c / 16;
+                                       char h = ((unsigned char)c) / 16;
                                        hh[2] = (h > 9) ? (h - 10 + 'A') : (h + '0');
-                                       h = c % 16;
+                                       h = ((unsigned char)c) % 16;
                                        hh[3] = (h > 9) ? (h - 10 + 'A') : (h + '0');
                                        buffer_append_string_len(dest, &hh[0], 4);
                                }
                                break;
                        }
                }
        }

Actions #2

Updated by stbuehler about 10 years ago

  • Target version set to 1.4.36
Actions #3

Updated by stbuehler over 8 years ago

  • Description updated (diff)
Actions #4

Updated by stbuehler over 8 years ago

  • Status changed from Patch Pending to Fixed
  • % Done changed from 0 to 100

Applied in changeset r2992.

Actions

Also available in: Atom