Fix CVE-2019-18934, shell execution in ipsecmod.

configure

@@ -1,6 +1,6 @@
 #! /bin/sh
 # Guess values for system-dependent variables and create Makefiles.
-# Generated by GNU Autoconf 2.69 for unbound 1.9.4.
+# Generated by GNU Autoconf 2.69 for unbound 1.9.5.
 #
 # Report bugs to <unbound-bugs@nlnetlabs.nl or https://github.com/NLnetLabs/unbound/issues>.
 #
@@ -591,8 +591,8 @@ MAKEFLAGS=
 # Identity of this package.
 PACKAGE_NAME='unbound'
 PACKAGE_TARNAME='unbound'
-PACKAGE_VERSION='1.9.4'
-PACKAGE_STRING='unbound 1.9.4'
+PACKAGE_VERSION='1.9.5'
+PACKAGE_STRING='unbound 1.9.5'
 PACKAGE_BUGREPORT='unbound-bugs@nlnetlabs.nl or https://github.com/NLnetLabs/unbound/issues'
 PACKAGE_URL=''
 
@@ -1445,7 +1445,7 @@ if test "$ac_init_help" = "long"; then
   # Omit some internal or obsolete options to make the list less imposing.
   # This message is too long to be a string in the A/UX 3.1 sh.
   cat <<_ACEOF
-\`configure' configures unbound 1.9.4 to adapt to many kinds of systems.
+\`configure' configures unbound 1.9.5 to adapt to many kinds of systems.
 
 Usage: $0 [OPTION]... [VAR=VALUE]...
 
@@ -1510,7 +1510,7 @@ fi
 
 if test -n "$ac_init_help"; then
   case $ac_init_help in
-     short | recursive ) echo "Configuration of unbound 1.9.4:";;
+     short | recursive ) echo "Configuration of unbound 1.9.5:";;
    esac
   cat <<\_ACEOF
 
@@ -1729,7 +1729,7 @@ fi
 test -n "$ac_init_help" && exit $ac_status
 if $ac_init_version; then
   cat <<\_ACEOF
-unbound configure 1.9.4
+unbound configure 1.9.5
 generated by GNU Autoconf 2.69
 
 Copyright (C) 2012 Free Software Foundation, Inc.
@@ -2438,7 +2438,7 @@ cat >config.log <<_ACEOF
 This file contains any messages produced by compilers while
 running configure, to aid debugging if configure makes a mistake.
 
-It was created by unbound $as_me 1.9.4, which was
+It was created by unbound $as_me 1.9.5, which was
 generated by GNU Autoconf 2.69.  Invocation command line was
 
   $ $0 $@
@@ -2790,11 +2790,11 @@ UNBOUND_VERSION_MAJOR=1
 
 UNBOUND_VERSION_MINOR=9
 
-UNBOUND_VERSION_MICRO=4
+UNBOUND_VERSION_MICRO=5
 
 
 LIBUNBOUND_CURRENT=9
-LIBUNBOUND_REVISION=4
+LIBUNBOUND_REVISION=5
 LIBUNBOUND_AGE=1
 # 1.0.0 had 0:12:0
 # 1.0.1 had 0:13:0
@@ -2866,6 +2866,7 @@ LIBUNBOUND_AGE=1
 # 1.9.2 had 9:2:1
 # 1.9.3 had 9:3:1
 # 1.9.4 had 9:4:1
+# 1.9.5 had 9:5:1
 
 #   Current  -- the number of the binary API that we're implementing
 #   Revision -- which iteration of the implementation of the binary
@@ -21274,7 +21275,7 @@ _ACEOF
 
 
 
-version=1.9.4
+version=1.9.5
 
 date=`date +'%b %e, %Y'`
 
@@ -21793,7 +21794,7 @@ cat >>$CONFIG_STATUS <<\_ACEOF || ac_write_fail=1
 # report actual input values of CONFIG_FILES etc. instead of their
 # values after options handling.
 ac_log="
-This file was extended by unbound $as_me 1.9.4, which was
+This file was extended by unbound $as_me 1.9.5, which was
 generated by GNU Autoconf 2.69.  Invocation command line was
 
   CONFIG_FILES    = $CONFIG_FILES
@@ -21859,7 +21860,7 @@ _ACEOF
 cat >>$CONFIG_STATUS <<_ACEOF || ac_write_fail=1
 ac_cs_config="`$as_echo "$ac_configure_args" | sed 's/^ //; s/[\\""\`\$]/\\\\&/g'`"
 ac_cs_version="\\
-unbound config.status 1.9.4
+unbound config.status 1.9.5
 configured by $0, generated by GNU Autoconf 2.69,
   with options \\"\$ac_cs_config\\"
 

configure.ac

@@ -11,14 +11,14 @@ sinclude(dnscrypt/dnscrypt.m4)
 # must be numbers. ac_defun because of later processing
 m4_define([VERSION_MAJOR],[1])
 m4_define([VERSION_MINOR],[9])
-m4_define([VERSION_MICRO],[4])
+m4_define([VERSION_MICRO],[5])
 AC_INIT(unbound, m4_defn([VERSION_MAJOR]).m4_defn([VERSION_MINOR]).m4_defn([VERSION_MICRO]), unbound-bugs@nlnetlabs.nl or https://github.com/NLnetLabs/unbound/issues, unbound)
 AC_SUBST(UNBOUND_VERSION_MAJOR, [VERSION_MAJOR])
 AC_SUBST(UNBOUND_VERSION_MINOR, [VERSION_MINOR])
 AC_SUBST(UNBOUND_VERSION_MICRO, [VERSION_MICRO])
 
 LIBUNBOUND_CURRENT=9
-LIBUNBOUND_REVISION=4
+LIBUNBOUND_REVISION=5
 LIBUNBOUND_AGE=1
 # 1.0.0 had 0:12:0
 # 1.0.1 had 0:13:0
@@ -90,6 +90,7 @@ LIBUNBOUND_AGE=1
 # 1.9.2 had 9:2:1
 # 1.9.3 had 9:3:1
 # 1.9.4 had 9:4:1
+# 1.9.5 had 9:5:1
 
 #   Current  -- the number of the binary API that we're implementing
 #   Revision -- which iteration of the implementation of the binary

ipsecmod/ipsecmod.c

@@ -161,6 +161,71 @@ generate_request(struct module_qstate* qstate, int id, uint8_t* name,
 	return 1;
 }
 
+/**
+ * Check if the string passed is a valid domain name with safe characters to
+ * pass to a shell.
+ * This will only allow:
+ *  - digits
+ *  - alphas
+ *  - hyphen (not at the start)
+ *  - dot (not at the start, or the only character)
+ *  - underscore
+ * @param s: pointer to the string.
+ * @param slen: string's length.
+ * @return true if s only contains safe characters; false otherwise.
+ */
+static int
+domainname_has_safe_characters(char* s, size_t slen) {
+	size_t i;
+	for(i = 0; i < slen; i++) {
+		if(s[i] == '\0') return 1;
+		if((s[i] == '-' && i != 0)
+			|| (s[i] == '.' && (i != 0 || s[1] == '\0'))
+			|| (s[i] == '_') || (s[i] >= '0' && s[i] <= '9')
+			|| (s[i] >= 'A' && s[i] <= 'Z')
+			|| (s[i] >= 'a' && s[i] <= 'z')) {
+			continue;
+		}
+		return 0;
+	}
+	return 1;
+}
+
+/**
+ * Check if the stringified IPSECKEY RDATA contains safe characters to pass to
+ * a shell.
+ * This is only relevant for checking the gateway when the gateway type is 3
+ * (domainname).
+ * @param s: pointer to the string.
+ * @param slen: string's length.
+ * @return true if s contains only safe characters; false otherwise.
+ */
+static int
+ipseckey_has_safe_characters(char* s, size_t slen) {
+	int precedence, gateway_type, algorithm;
+	char* gateway;
+	gateway = (char*)calloc(slen, sizeof(char));
+	if(!gateway) {
+		log_err("ipsecmod: out of memory when calling the hook");
+		return 0;
+	}
+	if(sscanf(s, "%d %d %d %s ",
+			&precedence, &gateway_type, &algorithm, gateway) != 4) {
+		free(gateway);
+		return 0;
+	}
+	if(gateway_type != 3) {
+		free(gateway);
+		return 1;
+	}
+	if(domainname_has_safe_characters(gateway, slen)) {
+		free(gateway);
+		return 1;
+	}
+	free(gateway);
+	return 0;
+}
+
 /**
  *  Prepare the data and call the hook.
  *
@@ -175,7 +240,7 @@ call_hook(struct module_qstate* qstate, struct ipsecmod_qstate* iq,
 {
 	size_t slen, tempdata_len, tempstring_len, i;
 	char str[65535], *s, *tempstring;
-	int w;
+	int w = 0, w_temp, qtype;
 	struct ub_packed_rrset_key* rrset_key;
 	struct packed_rrset_data* rrset_data;
 	uint8_t *tempdata;
@@ -192,9 +257,9 @@ call_hook(struct module_qstate* qstate, struct ipsecmod_qstate* iq,
 	memset(s, 0, slen);
 
 	/* Copy the hook into the buffer. */
-	sldns_str_print(&s, &slen, "%s", qstate->env->cfg->ipsecmod_hook);
+	w += sldns_str_print(&s, &slen, "%s", qstate->env->cfg->ipsecmod_hook);
 	/* Put space into the buffer. */
-	sldns_str_print(&s, &slen, " ");
+	w += sldns_str_print(&s, &slen, " ");
 	/* Copy the qname into the buffer. */
 	tempstring = sldns_wire2str_dname(qstate->qinfo.qname,
 		qstate->qinfo.qname_len);
@@ -202,68 +267,96 @@ call_hook(struct module_qstate* qstate, struct ipsecmod_qstate* iq,
 		log_err("ipsecmod: out of memory when calling the hook");
 		return 0;
 	}
-	sldns_str_print(&s, &slen, "\"%s\"", tempstring);
+	if(!domainname_has_safe_characters(tempstring, strlen(tempstring))) {
+		log_err("ipsecmod: qname has unsafe characters");
+		free(tempstring);
+		return 0;
+	}
+	w += sldns_str_print(&s, &slen, "\"%s\"", tempstring);
 	free(tempstring);
 	/* Put space into the buffer. */
-	sldns_str_print(&s, &slen, " ");
+	w += sldns_str_print(&s, &slen, " ");
 	/* Copy the IPSECKEY TTL into the buffer. */
 	rrset_data = (struct packed_rrset_data*)iq->ipseckey_rrset->entry.data;
-	sldns_str_print(&s, &slen, "\"%ld\"", (long)rrset_data->ttl);
+	w += sldns_str_print(&s, &slen, "\"%ld\"", (long)rrset_data->ttl);
 	/* Put space into the buffer. */
-	sldns_str_print(&s, &slen, " ");
-	/* Copy the A/AAAA record(s) into the buffer. Start and end this section
-	 * with a double quote. */
+	w += sldns_str_print(&s, &slen, " ");
 	rrset_key = reply_find_answer_rrset(&qstate->return_msg->qinfo,
 		qstate->return_msg->rep);
+	/* Double check that the records are indeed A/AAAA.
+	 * This should never happen as this function is only executed for A/AAAA
+	 * queries but make sure we don't pass anything other than A/AAAA to the
+	 * shell. */
+	qtype = ntohs(rrset_key->rk.type);
+	if(qtype != LDNS_RR_TYPE_AAAA && qtype != LDNS_RR_TYPE_A) {
+		log_err("ipsecmod: Answer is not of A or AAAA type");
+		return 0;
+	}
 	rrset_data = (struct packed_rrset_data*)rrset_key->entry.data;
-	sldns_str_print(&s, &slen, "\"");
+	/* Copy the A/AAAA record(s) into the buffer. Start and end this section
+	 * with a double quote. */
+	w += sldns_str_print(&s, &slen, "\"");
 	for(i=0; i<rrset_data->count; i++) {
 		if(i > 0) {
 			/* Put space into the buffer. */
-			sldns_str_print(&s, &slen, " ");
+			w += sldns_str_print(&s, &slen, " ");
 		}
 		/* Ignore the first two bytes, they are the rr_data len. */
-		w = sldns_wire2str_rdata_buf(rrset_data->rr_data[i] + 2,
+		w_temp = sldns_wire2str_rdata_buf(rrset_data->rr_data[i] + 2,
 			rrset_data->rr_len[i] - 2, s, slen, qstate->qinfo.qtype);
-		if(w < 0) {
+		if(w_temp < 0) {
 			/* Error in printout. */
-			return -1;
-		} else if((size_t)w >= slen) {
+			log_err("ipsecmod: Error in printing IP address");
+			return 0;
+		} else if((size_t)w_temp >= slen) {
 			s = NULL; /* We do not want str to point outside of buffer. */
 			slen = 0;
-			return -1;
+			log_err("ipsecmod: shell command too long");
+			return 0;
 		} else {
-			s += w;
-			slen -= w;
+			s += w_temp;
+			slen -= w_temp;
+			w += w_temp;
 		}
 	}
-	sldns_str_print(&s, &slen, "\"");
+	w += sldns_str_print(&s, &slen, "\"");
 	/* Put space into the buffer. */
-	sldns_str_print(&s, &slen, " ");
+	w += sldns_str_print(&s, &slen, " ");
 	/* Copy the IPSECKEY record(s) into the buffer. Start and end this section
 	 * with a double quote. */
-	sldns_str_print(&s, &slen, "\"");
+	w += sldns_str_print(&s, &slen, "\"");
 	rrset_data = (struct packed_rrset_data*)iq->ipseckey_rrset->entry.data;
 	for(i=0; i<rrset_data->count; i++) {
 		if(i > 0) {
 			/* Put space into the buffer. */
-			sldns_str_print(&s, &slen, " ");
+			w += sldns_str_print(&s, &slen, " ");
 		}
 		/* Ignore the first two bytes, they are the rr_data len. */
 		tempdata = rrset_data->rr_data[i] + 2;
 		tempdata_len = rrset_data->rr_len[i] - 2;
 		/* Save the buffer pointers. */
 		tempstring = s; tempstring_len = slen;
-		w = sldns_wire2str_ipseckey_scan(&tempdata, &tempdata_len, &s, &slen,
-			NULL, 0);
+		w_temp = sldns_wire2str_ipseckey_scan(&tempdata, &tempdata_len, &s,
+			&slen, NULL, 0);
 		/* There was an error when parsing the IPSECKEY; reset the buffer
 		 * pointers to their previous values. */
-		if(w == -1){
+		if(w_temp == -1) {
 			s = tempstring; slen = tempstring_len;
+		} else if(w_temp > 0) {
+			if(!ipseckey_has_safe_characters(
+					tempstring, tempstring_len - slen)) {
+				log_err("ipsecmod: ipseckey has unsafe characters");
+				return 0;
+			}
+			w += w_temp;
 		}
 	}
-	sldns_str_print(&s, &slen, "\"");
-	verbose(VERB_ALGO, "ipsecmod: hook command: '%s'", str);
+	w += sldns_str_print(&s, &slen, "\"");
+	if(w >= (int)sizeof(str)) {
+		log_err("ipsecmod: shell command too long");
+		return 0;
+	}
+	verbose(VERB_ALGO, "ipsecmod: shell command: '%s'", str);
 	/* ipsecmod-hook should return 0 on success. */
 	if(system(str) != 0)
 		return 0;