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@

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 <reyk@openbsd.org>
@@ -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 <reyk@openbsd.org>
@@ -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 {