Bug #1551

mod_accesslog does not escape quotes

Added by icy over 6 years ago. Updated over 2 years ago.

Status:FixedStart date:
Priority:NormalDue date:
Assignee:icy% Done:

100%

Category:mod_accesslog
Target version:1.4.31
Missing in 1.5.x:No

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

Associated revisions

Revision 2660
Added by stbuehler almost 5 years ago

mod_accesslog: escape special characters (fixes #1551, thx icy)

Revision 2661
Added by stbuehler almost 5 years ago

mod_accesslog: escape special characters (fixes #1551, thx icy)

Revision 2662
Added by stbuehler almost 5 years ago

Fix accesslog escape segfault (#1551)

Revision 2664
Added by stbuehler almost 5 years ago

Fix accesslog escape segfault (#1551)

Revision 2831
Added by stbuehler over 2 years ago

Fix access log escaping of " and \\ (fixes #1551)

History

#1 Updated by stbuehler almost 6 years ago

  • Target version changed from 1.4.20 to 1.4.21

#2 Updated by ralf almost 6 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;
}

#3 Updated by icy almost 6 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.

#4 Updated by icy over 5 years ago

  • Target version changed from 1.4.21 to 1.4.22

#5 Updated by stbuehler over 5 years ago

  • Target version changed from 1.4.22 to 1.4.23

#6 Updated by stbuehler over 5 years ago

  • Target version changed from 1.4.23 to 1.4.24

#7 Updated by stbuehler almost 5 years ago

  • Assignee changed from jan to icy

#8 Updated by stbuehler almost 5 years ago

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

#9 Updated by stbuehler almost 5 years ago

  • Target version changed from 1.4.x to 1.4.24

#10 Updated by stbuehler almost 5 years ago

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

Applied in changeset r2660.

#11 Updated by pmarinescu over 2 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

#12 Updated by stbuehler over 2 years ago

  • Status changed from Reopened to Fixed

Applied in changeset r2831.

#13 Updated by stbuehler over 2 years ago

  • Target version changed from 1.4.x to 1.4.31

Also available in: Atom