nat: Bail out if the regexp fails to compile and avoid a crash

If the regexp fails to compile the internal dfa is NULL and a
regexec will crash nicely. Fail and free the string if the regexp
fails to compile.

4 files changed, 31 insertions, 13 deletions

diff --git a/openbsc/include/openbsc/bsc_nat.h b/openbsc/include/openbsc/bsc_nat.h
index af753cc40..d6f61a6f4 100644
--- a/openbsc/include/openbsc/bsc_nat.h
+++ b/openbsc/include/openbsc/bsc_nat.h
@@ -332,7 +332,7 @@ int bsc_write_msg(struct write_queue *queue, struct msgb *msg);
 int bsc_write_cb(struct bsc_fd *bfd, struct msgb *msg);
 
 /* IMSI allow/deny handling */
-void bsc_parse_reg(void *ctx, regex_t *reg, char **imsi, int argc, const char **argv);
+int bsc_parse_reg(void *ctx, regex_t *reg, char **imsi, int argc, const char **argv) __attribute__ ((warn_unused_result));
 struct bsc_nat_acc_lst *bsc_nat_acc_lst_find(struct bsc_nat *nat, const char *name);
 struct bsc_nat_acc_lst *bsc_nat_acc_lst_get(struct bsc_nat *nat, const char *name);
 void bsc_nat_acc_lst_delete(struct bsc_nat_acc_lst *lst);
diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat_utils.c b/openbsc/src/osmo-bsc_nat/bsc_nat_utils.c
index f83289b61..4258364fe 100644
--- a/openbsc/src/osmo-bsc_nat/bsc_nat_utils.c
+++ b/openbsc/src/osmo-bsc_nat/bsc_nat_utils.c
@@ -606,8 +606,11 @@ int bsc_nat_filter_dt(struct bsc_connection *bsc, struct msgb *msg,
 	}
 }
 
-void bsc_parse_reg(void *ctx, regex_t *reg, char **imsi, int argc, const char **argv)
+int bsc_parse_reg(void *ctx, regex_t *reg, char **imsi, int argc, const char **argv)
 {
+	int ret;
+
+	ret = 0;
 	if (*imsi) {
 		talloc_free(*imsi);
 		*imsi = NULL;
@@ -616,8 +619,16 @@ void bsc_parse_reg(void *ctx, regex_t *reg, char **imsi, int argc, const char **
 
 	if (argc > 0) {
 		*imsi = talloc_strdup(ctx, argv[0]);
-		regcomp(reg, argv[0], 0);
+		ret = regcomp(reg, argv[0], 0);
+
+		/* handle compilation failures */
+		if (ret != 0) {
+			talloc_free(*imsi);
+			*imsi = NULL;
+		}
 	}
+
+	return ret;
 }
 
 static const char *con_types [] = {
diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c b/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c
index 3158f3472..3a5068e26 100644
--- a/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c
+++ b/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c
@@ -467,7 +467,8 @@ DEFUN(cfg_nat_ussd_query,
       "Set the USSD query to match with the ussd-list-name\n"
       "The query to match")
 {
-	bsc_parse_reg(_nat, &_nat->ussd_query_re, &_nat->ussd_query, argc, argv);
+	if (bsc_parse_reg(_nat, &_nat->ussd_query_re, &_nat->ussd_query, argc, argv) != 0)
+		return CMD_WARNING;
 	return CMD_SUCCESS;
 }
 
@@ -580,7 +581,8 @@ DEFUN(cfg_lst_imsi_allow,
 	if (!entry)
 		return CMD_WARNING;
 
-	bsc_parse_reg(acc, &entry->imsi_allow_re, &entry->imsi_allow, argc - 1, &argv[1]);
+	if (bsc_parse_reg(acc, &entry->imsi_allow_re, &entry->imsi_allow, argc - 1, &argv[1]) != 0)
+		return CMD_WARNING;
 	return CMD_SUCCESS;
 }
 
@@ -602,7 +604,8 @@ DEFUN(cfg_lst_imsi_deny,
 	if (!entry)
 		return CMD_WARNING;
 
-	bsc_parse_reg(acc, &entry->imsi_deny_re, &entry->imsi_deny, argc - 1, &argv[1]);
+	if (bsc_parse_reg(acc, &entry->imsi_deny_re, &entry->imsi_deny, argc - 1, &argv[1]) != 0)
+		return CMD_WARNING;
 	return CMD_SUCCESS;
 }
 
@@ -710,7 +713,8 @@ DEFUN(test_regex, test_regex_cmd,
 	char *str = NULL;
 
 	memset(&reg, 0, sizeof(reg));
-	bsc_parse_reg(_nat, &reg, &str, 1, argv);
+	if (bsc_parse_reg(_nat, &reg, &str, 1, argv) != 0)
+		return CMD_WARNING;
 
 	vty_out(vty, "String matches allow pattern: %d%s",
 		regexec(&reg, argv[1], 0, NULL, 0) == 0, VTY_NEWLINE);
diff --git a/openbsc/tests/bsc-nat/bsc_nat_test.c b/openbsc/tests/bsc-nat/bsc_nat_test.c
index f5de953f1..50da1e904 100644
--- a/openbsc/tests/bsc-nat/bsc_nat_test.c
+++ b/openbsc/tests/bsc-nat/bsc_nat_test.c
@@ -761,15 +761,18 @@ static void test_cr_filter()
 		nat_lst = bsc_nat_acc_lst_get(nat, "nat");
 		bsc_lst = bsc_nat_acc_lst_get(nat, "bsc");
 
-		bsc_parse_reg(nat_entry, &nat_entry->imsi_deny_re, &nat_entry->imsi_deny,
+		if (bsc_parse_reg(nat_entry, &nat_entry->imsi_deny_re, &nat_entry->imsi_deny,
 			      cr_filter[i].nat_imsi_deny ? 1 : 0,
-			      &cr_filter[i].nat_imsi_deny);
-		bsc_parse_reg(bsc_entry, &bsc_entry->imsi_allow_re, &bsc_entry->imsi_allow,
+			      &cr_filter[i].nat_imsi_deny) != 0)
+			abort();
+		if (bsc_parse_reg(bsc_entry, &bsc_entry->imsi_allow_re, &bsc_entry->imsi_allow,
 			      cr_filter[i].bsc_imsi_allow ? 1 : 0,
-			      &cr_filter[i].bsc_imsi_allow);
-		bsc_parse_reg(bsc_entry, &bsc_entry->imsi_deny_re, &bsc_entry->imsi_deny,
+			      &cr_filter[i].bsc_imsi_allow) != 0)
+			abort();
+		if (bsc_parse_reg(bsc_entry, &bsc_entry->imsi_deny_re, &bsc_entry->imsi_deny,
 			      cr_filter[i].bsc_imsi_deny ? 1 : 0,
-			      &cr_filter[i].bsc_imsi_deny);
+			      &cr_filter[i].bsc_imsi_deny) != 0)
+			abort();
 
 		parsed = bsc_nat_parse(msg);
 		if (!parsed) {