Hi!
I was testing relayd and found an issue which would make relayd crash.
Reproduce:
Test config /etc/relayd.conf:
http protocol "test" {
pass response
block url "host"
return error
}
relay "testing" {
listen on "127.0.0.1" port 8080
protocol "test"
forward to www.openbsd.org port 80
}
# relayd -vv -d -f /etc/relayd.conf
$ curl -H 'Host: ' 127.0.0.1:8080/
or
$ printf 'GET HTTP/1.0\r\nHost: \r\nConnection: close\r\n\r\n' | nc 127.0.0.1
8080
Result: relayd crashes:
startup
protocol 1: name test
flags: used, return, relay flags:
type: http
pass response
block request url "host" value "*"
relay testing, session 1 (1 active), 0, 127.0.0.1 -> :80, invalid
host name (400 Bad Request)
ca exiting, pid 15473
hce exiting, pid 21139
ca exiting, pid 31583
ca exiting, pid 4581
pfe exiting, pid 29863
relay exiting, pid 25103
relay exiting, pid 10471
parent terminating, pid 22282
After a bit of searching I found in relay_lookup_url():
...
if (canonicalize_host(host, ph, sizeof(ph)) == NULL) {
relay_abort_http(con, 400, "invalid host name", 0);
return (RES_FAIL);
}
...
relay_abort_http will call relay_close and close the connection
and free() it. When this happens relay_test() will still use it after
relay_close in relay_apply_actions() and after relay_test is called in
relay_http.c: relay_read_http().
The patch below is a bit messy, it sets cre->con to NULL after
relay_abort_http (which free'd the connection) and checks this state
later.
I follow relayd and httpd with great interest and like how httpd is
rapidly improving lately, nice work!
Kind regards,
Hiltjo
Patch:
Index: relay_http.c
===================================================================
RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v
retrieving revision 1.52
diff -u -p -r1.52 relay_http.c
--- relay_http.c 28 Jul 2015 10:24:26 -0000 1.52
+++ relay_http.c 8 Aug 2015 14:09:12 -0000
@@ -365,7 +365,8 @@ relay_read_http(struct bufferevent *bev,
relay_close(con, "filter rule failed");
return;
} else if (action != RES_PASS) {
- relay_abort_http(con, 403, "Forbidden", con->se_label);
+ if (cre->con)
+ relay_abort_http(cre->con, 403, "Forbidden",
con->se_label);
return;
}
@@ -733,7 +734,7 @@ relay_lookup_url(struct ctl_relay_event
if (canonicalize_host(host, ph, sizeof(ph)) == NULL) {
relay_abort_http(con, 400, "invalid host name", 0);
- return (RES_FAIL);
+ goto fail;
}
bzero(hi, sizeof(hi));
@@ -749,7 +750,7 @@ relay_lookup_url(struct ctl_relay_event
if ((pp = strdup(desc->http_path)) == NULL) {
relay_abort_http(con, 500, "failed to allocate path", 0);
- return (RES_FAIL);
+ goto fail;
}
for (i = (RELAY_MAXLOOKUPLEVELS - 1); i >= 0; i--) {
if (hi[i] == NULL)
@@ -785,6 +786,9 @@ relay_lookup_url(struct ctl_relay_event
done:
free(pp);
return (ret);
+ fail:
+ cre->con = NULL;
+ return (RES_FAIL);
}
int
@@ -1653,6 +1657,7 @@ relay_test(struct protocol *proto, struc
u_int action = RES_PASS;
struct kvlist actions, matches;
struct kv *kv;
+ int ret;
con = cre->con;
TAILQ_INIT(&actions);
@@ -1686,9 +1691,11 @@ relay_test(struct protocol *proto, struc
RELAY_GET_NEXT_STEP;
else if (relay_httppath_test(cre, r, &matches) != 0)
RELAY_GET_NEXT_STEP;
- else if (relay_httpurl_test(cre, r, &matches) != 0)
+ else if ((ret = relay_httpurl_test(cre, r, &matches)) != 0) {
+ if (ret == -1)
+ return (RES_DROP);
RELAY_GET_NEXT_STEP;
- else if (relay_httpcookie_test(cre, r, &matches) != 0)
+ } else if (relay_httpcookie_test(cre, r, &matches) != 0)
RELAY_GET_NEXT_STEP;
else {
DPRINTF("%s: session %d: matched rule %d",
