Bug #1551
mod_accesslog does not escape quotes
| Status: | Fixed | Start date: | ||
|---|---|---|---|---|
| Priority: | Normal | Due 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
mod_accesslog: escape special characters (fixes #1551, thx icy)
mod_accesslog: escape special characters (fixes #1551, thx icy)
Fix accesslog escape segfault (#1551)
Fix accesslog escape segfault (#1551)
Fix access log escaping of " and \\ (fixes #1551)
History
#1 Updated by stbuehler over 4 years ago
- Target version changed from 1.4.20 to 1.4.21
#2 Updated by ralf over 4 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 over 4 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 4 years ago
- Target version changed from 1.4.21 to 1.4.22
#5 Updated by stbuehler about 4 years ago
- Target version changed from 1.4.22 to 1.4.23
#6 Updated by stbuehler almost 4 years ago
- Target version changed from 1.4.23 to 1.4.24
#7 Updated by stbuehler over 3 years ago
- Assignee changed from jan to icy
#8 Updated by stbuehler over 3 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 over 3 years ago
- Target version changed from 1.4.x to 1.4.24
#10 Updated by stbuehler over 3 years ago
- Status changed from New to Fixed
- % Done changed from 0 to 100
Applied in changeset r2660.
#11 Updated by pmarinescu about 1 year 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 about 1 year ago
- Status changed from Reopened to Fixed
Applied in changeset r2831.
#13 Updated by stbuehler about 1 year ago
- Target version changed from 1.4.x to 1.4.31
Also available in: Atom