From 9357f4aa977b0d1366d14125b6a2856f1217f85b Mon Sep 17 00:00:00 2001 From: benno Date: Fri, 22 Jul 2016 09:30:36 +0000 Subject: [PATCH] fix some cases where we relay_abort_http() the connection too soon. instead, pass a more specific error back and handle the errors in relay_test() instead. reported by Arto Jonsson and Hiltjo Posthuma, thanks! ok bluhm@ reyk@ --- usr.sbin/relayd/relay_http.c | 75 +++++++++++++++++++++--------------- usr.sbin/relayd/relayd.h | 6 ++- 2 files changed, 48 insertions(+), 33 deletions(-) diff --git a/usr.sbin/relayd/relay_http.c b/usr.sbin/relayd/relay_http.c index 8ba875b327c..3d4066b71cd 100644 --- a/usr.sbin/relayd/relay_http.c +++ b/usr.sbin/relayd/relay_http.c @@ -1,4 +1,4 @@ -/* $OpenBSD: relay_http.c,v 1.55 2015/12/15 10:36:59 reyk Exp $ */ +/* $OpenBSD: relay_http.c,v 1.56 2016/07/22 09:30:36 benno Exp $ */ /* * Copyright (c) 2006 - 2015 Reyk Floeter @@ -349,10 +349,20 @@ relay_read_http(struct bufferevent *bev, void *arg) } action = relay_test(proto, cre); - if (action == RES_FAIL) { + switch (action) { + case RES_FAIL: relay_close(con, "filter rule failed"); return; - } else if (action != RES_PASS) { + case RES_BAD: + relay_abort_http(con, 400, "Bad Request", + con->se_label); + return; + case RES_INTERNAL: + relay_abort_http(con, 500, "Internal Server Error", + con->se_label); + return; + } + if (action != RES_PASS) { relay_abort_http(con, 403, "Forbidden", con->se_label); return; } @@ -698,7 +708,6 @@ _relay_lookup_url(struct ctl_relay_event *cre, char *host, char *path, int relay_lookup_url(struct ctl_relay_event *cre, const char *host, struct kv *kv) { - struct rsession *con = cre->con; struct http_descriptor *desc = (struct http_descriptor *)cre->desc; int i, j, dots; char *hi[RELAY_MAXLOOKUPLEVELS], *p, *pp, *c, ch; @@ -714,13 +723,12 @@ relay_lookup_url(struct ctl_relay_event *cre, const char *host, struct kv *kv) * developers_guide.html#PerformingLookups */ - DPRINTF("%s: session %d: host '%s', path '%s', query '%s'", - __func__, con->se_id, host, desc->http_path, + DPRINTF("%s: host '%s', path '%s', query '%s'", + __func__, host, desc->http_path, desc->http_query == NULL ? "" : desc->http_query); if (canonicalize_host(host, ph, sizeof(ph)) == NULL) { - relay_abort_http(con, 400, "invalid host name", 0); - return (RES_FAIL); + return (RES_BAD); } bzero(hi, sizeof(hi)); @@ -735,8 +743,7 @@ relay_lookup_url(struct ctl_relay_event *cre, const char *host, struct kv *kv) hi[dots] = ph; if ((pp = strdup(desc->http_path)) == NULL) { - relay_abort_http(con, 500, "failed to allocate path", 0); - return (RES_FAIL); + return (RES_INTERNAL); } for (i = (RELAY_MAXLOOKUPLEVELS - 1); i >= 0; i--) { if (hi[i] == NULL) @@ -778,13 +785,11 @@ int relay_lookup_cookie(struct ctl_relay_event *cre, const char *str, struct kv *kv) { - struct rsession *con = cre->con; char *val, *ptr, *key, *value; int ret; if ((val = strdup(str)) == NULL) { - relay_abort_http(con, 500, "failed to allocate cookie", 0); - return (RES_FAIL); + return (RES_INTERNAL); } for (ptr = val; ptr != NULL && strlen(ptr);) { @@ -810,9 +815,8 @@ relay_lookup_cookie(struct ctl_relay_event *cre, const char *str, if (value[strlen(value) - 1] == '"') value[strlen(value) - 1] = '\0'; - DPRINTF("%s: session %d: %s = %s, %s = %s : %d", - __func__, con->se_id, - key, value, kv->kv_key, kv->kv_value, + DPRINTF("%s: key %s = %s, %s = %s : %d", + __func__, key, value, kv->kv_key, kv->kv_value, strcasecmp(kv->kv_key, key)); if (strcasecmp(kv->kv_key, key) == 0 && @@ -842,8 +846,7 @@ relay_lookup_query(struct ctl_relay_event *cre, struct kv *kv) if (desc->http_query == NULL) return (-1); if ((val = strdup(desc->http_query)) == NULL) { - relay_abort_http(cre->con, 500, "failed to allocate query", 0); - return (-1); + return (RES_INTERNAL); } ptr = val; @@ -1228,13 +1231,14 @@ relay_httpquery_test(struct ctl_relay_event *cre, struct relay_rule *rule, struct http_descriptor *desc = cre->desc; struct kv *match = &desc->http_matchquery; struct kv *kv = &rule->rule_kv[KEY_TYPE_QUERY]; + int res = 0; if (cre->dir == RELAY_DIR_RESPONSE || kv->kv_type != KEY_TYPE_QUERY) return (0); else if (kv->kv_key == NULL) return (0); - else if (relay_lookup_query(cre, kv)) - return (-1); + else if ((res = relay_lookup_query(cre, kv)) != 0) + return (res); relay_match(actions, kv, match, NULL); @@ -1309,6 +1313,7 @@ relay_httpurl_test(struct ctl_relay_event *cre, struct relay_rule *rule, struct kv *host, key; struct kv *kv = &rule->rule_kv[KEY_TYPE_URL]; struct kv *match = &desc->http_pathquery; + int res; if (cre->dir == RELAY_DIR_RESPONSE || kv->kv_type != KEY_TYPE_URL || kv->kv_key == NULL) @@ -1323,9 +1328,8 @@ relay_httpurl_test(struct ctl_relay_event *cre, struct relay_rule *rule, kv->kv_option == KEY_OPTION_LOG && fnmatch(kv->kv_key, match->kv_key, FNM_CASEFOLD) != FNM_NOMATCH) { /* fnmatch url only for logging */ - } else if (relay_lookup_url(cre, host->kv_value, kv) != 0) - return (-1); - + } else if ((res = relay_lookup_url(cre, host->kv_value, kv)) != 0) + return (res); relay_match(actions, kv, match, NULL); return (0); @@ -1338,6 +1342,7 @@ relay_httpcookie_test(struct ctl_relay_event *cre, struct relay_rule *rule, struct http_descriptor *desc = cre->desc; struct kv *kv = &rule->rule_kv[KEY_TYPE_COOKIE], key; struct kv *match = NULL; + int res; if (kv->kv_type != KEY_TYPE_COOKIE) return (0); @@ -1364,8 +1369,9 @@ relay_httpcookie_test(struct ctl_relay_event *cre, struct relay_rule *rule, return (-1); if (kv->kv_key == NULL || match->kv_value == NULL) return (0); - else if (relay_lookup_cookie(cre, match->kv_value, kv) != 0) - return (-1); + else if ((res = relay_lookup_cookie(cre, match->kv_value, + kv)) != 0) + return (res); } relay_match(actions, kv, match, &desc->http_headers); @@ -1640,6 +1646,7 @@ relay_test(struct protocol *proto, struct ctl_relay_event *cre) u_int action = RES_PASS; struct kvlist actions, matches; struct kv *kv; + int res = 0; con = cre->con; TAILQ_INIT(&actions); @@ -1647,8 +1654,10 @@ relay_test(struct protocol *proto, struct ctl_relay_event *cre) r = TAILQ_FIRST(&proto->rules); while (r != NULL) { cnt++; + TAILQ_INIT(&matches); TAILQ_INIT(&r->rule_kvlist); + if (r->rule_dir && r->rule_dir != cre->dir) RELAY_GET_SKIP_STEP(RULE_SKIP_DIR); else if (proto->type != r->rule_proto) @@ -1669,13 +1678,13 @@ relay_test(struct protocol *proto, struct ctl_relay_event *cre) RELAY_GET_NEXT_STEP; else if (relay_httpheader_test(cre, r, &matches) != 0) RELAY_GET_NEXT_STEP; - else if (relay_httpquery_test(cre, r, &matches) != 0) + else if ((res = relay_httpquery_test(cre, r, &matches)) != 0) 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 ((res = relay_httpurl_test(cre, r, &matches)) != 0) RELAY_GET_NEXT_STEP; - else if (relay_httpcookie_test(cre, r, &matches) != 0) + else if ((res = relay_httpcookie_test(cre, r, &matches)) != 0) RELAY_GET_NEXT_STEP; else { DPRINTF("%s: session %d: matched rule %d", @@ -1708,12 +1717,16 @@ relay_test(struct protocol *proto, struct ctl_relay_event *cre) nextrule: /* Continue to find last matching policy */ - r = TAILQ_NEXT(r, rule_entry); + DPRINTF("%s: session %d, res %d", __func__, + con->se_id, res); + if (res == RES_BAD || res == RES_INTERNAL) + return(res); + res = 0; + r = TAILQ_NEXT(r, rule_entry); } } - if (rule != NULL && - relay_match_actions(cre, rule, NULL, &actions) != 0) { + if (rule != NULL && relay_match_actions(cre, rule, NULL, &actions) != 0) { /* Something bad happened, drop */ action = RES_DROP; } diff --git a/usr.sbin/relayd/relayd.h b/usr.sbin/relayd/relayd.h index 42e76042dd1..2430617a16f 100644 --- a/usr.sbin/relayd/relayd.h +++ b/usr.sbin/relayd/relayd.h @@ -1,4 +1,4 @@ -/* $OpenBSD: relayd.h,v 1.222 2016/01/11 21:31:42 benno Exp $ */ +/* $OpenBSD: relayd.h,v 1.223 2016/07/22 09:30:36 benno Exp $ */ /* * Copyright (c) 2006 - 2015 Reyk Floeter @@ -583,7 +583,9 @@ enum prototype { enum relay_result { RES_DROP = 0, RES_PASS = 1, - RES_FAIL = -1 + RES_FAIL = -1, + RES_BAD = -2, + RES_INTERNAL = -3 }; enum rule_action { -- 2.20.1