Project

General

Profile

Actions

Bug #1551

closed

mod_accesslog does not escape quotes

Added by icy almost 17 years ago. Updated over 12 years ago.

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

Description

mod_accesslog does not escape characters like quotes so it is possible to corrupt or inject stuff into the access.log

POC: curl localhost/foo\"bar && tail -1 /var/log/lighttpd/access.log
or: curl localhost -H "Referer: foo\" \"bar" && tail -1 /var/log/lighttpd/access.log


Related issues 1 (0 open1 closed)

Has duplicate Bug #918: lighttpd does not escape double quotes in request logsDuplicateActions
Actions #1

Updated by stbuehler about 16 years ago

  • Target version changed from 1.4.20 to 1.4.21
Actions #2

Updated by ralf about 16 years ago

what about this?:

src/mod_accesslog.c


                        case FORMAT_HEADER:
                                /* BUG #1551
                                 *
                                 * apache: 192.168.100.5 - - [27/Oct/2008:19:51:14 +0100] "GET /catchall/blank.html HTTP/1.1" 200 - "foo\" \"bar" "lwp-request/2.07" 
                                 * lighty: 127.0.0.1 localhost:8080 - [27/Oct/2008:19:57:10 +0100] "GET /crap.html HTTP/1.1" 404 345 "foo" "bar" "lwp-request/2.07" 
                                 *
                                 * */
                                if (NULL != (ds = (data_string *)array_get_element(con->request.headers, p->conf.parsed_format->ptr[j]->string->ptr))) {
                                        /*
                                         * IF the first character of the format string is a " or ' then quote them  otherwise copy.
                                         */
                                        if (ds->value->used && ds->value->ptr && (ds->value->ptr[0] == '\'' || ds->value->ptr[0] == '\"')) {
                                                char escape[2] = {0,};
                                                escape[0] = ds->value->ptr[0];
                                                buffer_append_string_buffer_escape(b, ds->value, escape);
                                        } else {
                                                buffer_append_string_buffer(b, ds->value);
                                        }

                                } else {
                                        buffer_append_string_len(b, CONST_STR_LEN("-"));
                                }

                                break;

src/buffer.c

/* this function s*cks a bit because its slow.. */
int buffer_append_string_buffer_escape(buffer *b, const buffer *src, const char *escape)
{
        size_t          nsrc;
        const char      *psrc;

        if (!src) return -1;
        if (src->used == 0) return 0;

        if (!escape || 0 == escape[0]) {
                return buffer_append_string_buffer(b,src);
        }

        /*
         * XXX
         *
         * - byte by byte copy s*cks
         * - mostly the escape character is not in src
         *
         */
        for (psrc=src->ptr,nsrc=src->used; nsrc; --nsrc,++psrc) {
                const char      *pesc;

                for (pesc=escape; *pesc; ++pesc) {
                        if (*psrc==*pesc) {
                                buffer_append_string(b, "\\");
                                break;
                        }
                }

                buffer_append_string_len(b, psrc, 1);
        }

        return 0;
}

Actions #3

Updated by icy about 16 years ago

  • Patch available set to No

Like I mentioned in the bugreport, the bug not only applies to header values (referer) but also to the path.
Generally speaking: all strings supplied by the client which can have a " or other characters that could confuse accesslog parsers.

Actions #4

Updated by icy almost 16 years ago

  • Target version changed from 1.4.21 to 1.4.22
Actions #5

Updated by stbuehler over 15 years ago

  • Target version changed from 1.4.22 to 1.4.23
Actions #6

Updated by stbuehler over 15 years ago

  • Target version changed from 1.4.23 to 1.4.24
Actions #7

Updated by stbuehler about 15 years ago

  • Assignee changed from jan to icy
Actions #8

Updated by stbuehler about 15 years ago

  • Target version changed from 1.4.24 to 1.4.x
  • Missing in 1.5.x set to No
Actions #9

Updated by stbuehler about 15 years ago

  • Target version changed from 1.4.x to 1.4.24
Actions #10

Updated by stbuehler about 15 years ago

  • Status changed from New to Fixed
  • % Done changed from 0 to 100

Applied in changeset r2660.

Actions #11

Updated by pmarinescu over 12 years ago

  • Status changed from Fixed to Reopened
  • Target version changed from 1.4.24 to 1.4.x

Am I missing something or this does not fix the issue? The patch is supposed to work by escaping quotes with a backslash but the code intended to do that is dead:

165        if (str->ptr[i] >= ' ' && str->ptr[i] <= '~') {
166            /* printable chars */
167            buffer_append_string_len(dest, &str->ptr[i], 1);
168        } else switch (str->ptr[i]) {
169        case '"':
170            BUFFER_APPEND_STRING_CONST(dest, "\\\"");
171            break;

Obviously, str->ptr[i] cannot be " in the else branch ('"' >= ' ' && '"' <= '~' is always true).

For one request without referrer and one with it set to 'foo" "bar', the latest lighttpd version logs:

127.0.0.1 - - [18/Apr/2012:02:14:44 +0100] "GET /index.html HTTP/1.0" 200 4348 "-" "-"
127.0.0.1 - - [18/Apr/2012:02:14:44 +0100] "GET /index.html HTTP/1.0" 200 4348 "foo" "bar" "-"

While I believe that the intended behavior is to have the 2nd as
127.0.0.1 - - [18/Apr/2012:02:14:44 +0100] "GET /index.html HTTP/1.0" 200 4348 "foo\" \"bar" "-"

Paul

Actions #12

Updated by stbuehler over 12 years ago

  • Status changed from Reopened to Fixed

Applied in changeset r2831.

Actions #13

Updated by stbuehler over 12 years ago

  • Target version changed from 1.4.x to 1.4.31
Actions #14

Updated by stbuehler almost 9 years ago

  • Has duplicate Bug #918: lighttpd does not escape double quotes in request logs added
Actions

Also available in: Atom