From 104bab75f2068211852825e93bbbc3b0ea98c903 Mon Sep 17 00:00:00 2001 From: "Daniel Knoppel (Phusion)" Date: Mon, 18 May 2015 14:01:22 +0200 Subject: [PATCH 1/5] Fix invalid memory access. http_parse_host() depends on u->field_data[UF_HOST], but this if() allowed the method to be called if only u->field_data[UF_SCHEMA] was set, resulting in use of unintialized pointers. --- http_parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http_parser.c b/http_parser.c index 0fa1c362..b27fd496 100644 --- a/http_parser.c +++ b/http_parser.c @@ -2376,7 +2376,7 @@ http_parser_parse_url(const char *buf, size_t buflen, int is_connect, /* host must be present if there is a schema */ /* parsing http:///toto will fail */ - if ((u->field_set & ((1 << UF_SCHEMA) | (1 << UF_HOST))) != 0) { + if ((u->field_set & (1 << UF_SCHEMA)) != 0 && (u->field_set & (1 << UF_HOST)) != 0) { if (http_parse_host(buf, u, found_at) != 0) { return 1; } From 62cd85912e73dfb02e51286659c5a56562c540cc Mon Sep 17 00:00:00 2001 From: "Daniel Knoppel (Phusion)" Date: Mon, 18 May 2015 15:43:40 +0200 Subject: [PATCH 2/5] Fixup: the patch stopped host parsing in the CONNECT flow. This update restores that functionality while maintaining the memory access fix (only call http_parse_host if UF_HOST is set). --- http_parser.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/http_parser.c b/http_parser.c index b27fd496..741cffc2 100644 --- a/http_parser.c +++ b/http_parser.c @@ -2376,10 +2376,8 @@ http_parser_parse_url(const char *buf, size_t buflen, int is_connect, /* host must be present if there is a schema */ /* parsing http:///toto will fail */ - if ((u->field_set & (1 << UF_SCHEMA)) != 0 && (u->field_set & (1 << UF_HOST)) != 0) { - if (http_parse_host(buf, u, found_at) != 0) { - return 1; - } + if ((u->field_set & (1 << UF_SCHEMA)) && (u->field_set & (1 << UF_HOST)) == 0) { + return 1; } /* CONNECT requests can only contain "hostname:port" */ @@ -2387,6 +2385,12 @@ http_parser_parse_url(const char *buf, size_t buflen, int is_connect, return 1; } + if (u->field_set & (1 << UF_HOST)) { + if (http_parse_host(buf, u, found_at) != 0) { + return 1; + } + } + if (u->field_set & (1 << UF_PORT)) { /* Don't bother with endp; we've already validated the string */ unsigned long v = strtoul(buf + u->field_data[UF_PORT].off, NULL, 10); From 686220789eb125386f586ff430ca26be5503908d Mon Sep 17 00:00:00 2001 From: "Daniel Knoppel (Phusion)" Date: Mon, 18 May 2015 17:01:11 +0200 Subject: [PATCH 3/5] Fix test failure & 80 char limit. http_parse_host also affects UF_PORT and cannot be moved below the check --- http_parser.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/http_parser.c b/http_parser.c index 741cffc2..9bcd1fed 100644 --- a/http_parser.c +++ b/http_parser.c @@ -2376,12 +2376,8 @@ http_parser_parse_url(const char *buf, size_t buflen, int is_connect, /* host must be present if there is a schema */ /* parsing http:///toto will fail */ - if ((u->field_set & (1 << UF_SCHEMA)) && (u->field_set & (1 << UF_HOST)) == 0) { - return 1; - } - - /* CONNECT requests can only contain "hostname:port" */ - if (is_connect && u->field_set != ((1 << UF_HOST)|(1 << UF_PORT))) { + if ((u->field_set & (1 << UF_SCHEMA)) && + (u->field_set & (1 << UF_HOST)) == 0) { return 1; } @@ -2391,6 +2387,11 @@ http_parser_parse_url(const char *buf, size_t buflen, int is_connect, } } + /* CONNECT requests can only contain "hostname:port" */ + if (is_connect && u->field_set != ((1 << UF_HOST)|(1 << UF_PORT))) { + return 1; + } + if (u->field_set & (1 << UF_PORT)) { /* Don't bother with endp; we've already validated the string */ unsigned long v = strtoul(buf + u->field_data[UF_PORT].off, NULL, 10); From 015094c963689f2158a0022bab2e7d04b7a7dba0 Mon Sep 17 00:00:00 2001 From: "Daniel Knoppel (Phusion)" Date: Tue, 19 May 2015 00:28:42 +0200 Subject: [PATCH 4/5] Revert code formatter change.. --- http_parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http_parser.c b/http_parser.c index 9bcd1fed..f6bf9d36 100644 --- a/http_parser.c +++ b/http_parser.c @@ -2389,7 +2389,7 @@ http_parser_parse_url(const char *buf, size_t buflen, int is_connect, /* CONNECT requests can only contain "hostname:port" */ if (is_connect && u->field_set != ((1 << UF_HOST)|(1 << UF_PORT))) { - return 1; + return 1; } if (u->field_set & (1 << UF_PORT)) { From 1b4fa946f0fb6dc200dc82bf06af15d354c7ef91 Mon Sep 17 00:00:00 2001 From: "Daniel Knoppel (Phusion)" Date: Tue, 19 May 2015 17:03:50 +0200 Subject: [PATCH 5/5] Test trigger for invalid memory access fix. --- http_parser.c | 1 + 1 file changed, 1 insertion(+) diff --git a/http_parser.c b/http_parser.c index f6bf9d36..91f76f24 100644 --- a/http_parser.c +++ b/http_parser.c @@ -2232,6 +2232,7 @@ http_parse_host_char(enum http_host_state s, const char ch) { static int http_parse_host(const char * buf, struct http_parser_url *u, int found_at) { + assert(u->field_set & (1 << UF_HOST)); enum http_host_state s; const char *p;