gprs: Add testcases for the APN string/octet conversion and fix it

Create a testcase for the gprs_str_to_apn and gprs_apn_to_str
routines. While writing the testcase we noticed it is possible to
write more bytes than should have been allowed. This is fixed by
checking that the max_len is at least 1 (needed to write the first
length octet) and to do the size check before writing to the output.

Modify the signature of gprs_str_to_apn to put the length/size next
to the parameter that requires a size.

Done with Jacob

6 files changed, 119 insertions, 9 deletions

diff --git a/openbsc/include/openbsc/gprs_utils.h b/openbsc/include/openbsc/gprs_utils.h
index 1feb0a2e3..2ad5fe490 100644
--- a/openbsc/include/openbsc/gprs_utils.h
+++ b/openbsc/include/openbsc/gprs_utils.h
@@ -30,4 +30,4 @@ struct msgb *gprs_msgb_copy(const struct msgb *msg, const char *name);
 int gprs_msgb_resize_area(struct msgb *msg, uint8_t *area,
 			    size_t old_size, size_t new_size);
 char *gprs_apn_to_str(char *out_str, const uint8_t *apn_enc, size_t rest_chars);
-int gprs_str_to_apn(uint8_t *apn_enc, const char *str, size_t max_chars);
+int gprs_str_to_apn(uint8_t *apn_enc, size_t max_len, const char *str);
diff --git a/openbsc/src/gprs/gb_proxy_vty.c b/openbsc/src/gprs/gb_proxy_vty.c
index 29494c6a8..d40d9b394 100644
--- a/openbsc/src/gprs/gb_proxy_vty.c
+++ b/openbsc/src/gprs/gb_proxy_vty.c
@@ -230,7 +230,7 @@ static int set_core_apn(struct vty *vty, const char *apn, const char *filter)
 		g_cfg->core_apn =
 			talloc_realloc_size(NULL, g_cfg->core_apn, apn_len + 1);
 		g_cfg->core_apn_size =
-			gprs_str_to_apn(g_cfg->core_apn, apn, apn_len + 1);
+			gprs_str_to_apn(g_cfg->core_apn, apn_len + 1, apn);
 	}
 
 	return CMD_SUCCESS;
diff --git a/openbsc/src/gprs/gprs_utils.c b/openbsc/src/gprs/gprs_utils.c
index 8b82f2506..1a8fa4316 100644
--- a/openbsc/src/gprs/gprs_utils.c
+++ b/openbsc/src/gprs/gprs_utils.c
@@ -103,6 +103,9 @@ int gprs_msgb_resize_area(struct msgb *msg, uint8_t *area,
 }
 
 /* TODO: Move these conversion functions to a utils file. */
+/**
+ * out_str needs to have rest_chars amount of bytes or 1 whatever is bigger.
+ */
 char * gprs_apn_to_str(char *out_str, const uint8_t *apn_enc, size_t rest_chars)
 {
 	char *str = out_str;
@@ -125,13 +128,24 @@ char * gprs_apn_to_str(char *out_str, const uint8_t *apn_enc, size_t rest_chars)
 	return out_str;
 }
 
-int gprs_str_to_apn(uint8_t *apn_enc, const char *str, size_t max_chars)
+int gprs_str_to_apn(uint8_t *apn_enc, size_t max_len, const char *str)
 {
-	uint8_t *last_len_field = apn_enc;
-	int len = 1;
+	uint8_t *last_len_field;
+	int len;
+
+	/* Can we even write the length field to the output? */
+	if (max_len == 0)
+		return -1;
+
+	/* Remember where we need to put the length once we know it */
+	last_len_field = apn_enc;
+	len = 1;
 	apn_enc += 1;
 
 	while (str[0]) {
+		if (len >= max_len)
+			return -1;
+
 		if (str[0] == '.') {
 			*last_len_field = (apn_enc - last_len_field) - 1;
 			last_len_field = apn_enc;
@@ -141,8 +155,6 @@ int gprs_str_to_apn(uint8_t *apn_enc, const char *str, size_t max_chars)
 		apn_enc += 1;
 		str += 1;
 		len += 1;
-		if (len > max_chars)
-			return -1;
 	}
 
 	*last_len_field = (apn_enc - last_len_field) - 1;
diff --git a/openbsc/tests/gbproxy/gbproxy_test.c b/openbsc/tests/gbproxy/gbproxy_test.c
index 0d14d2194..aa909ef42 100644
--- a/openbsc/tests/gbproxy/gbproxy_test.c
+++ b/openbsc/tests/gbproxy/gbproxy_test.c
@@ -945,7 +945,7 @@ static void test_gbproxy_ra_patching()
 	gbcfg.core_mcc = 123;
 	gbcfg.core_mnc = 456;
 	gbcfg.core_apn = talloc_zero_size(NULL, 100);
-	gbcfg.core_apn_size = gprs_str_to_apn(gbcfg.core_apn, "foo.bar", 100);
+	gbcfg.core_apn_size = gprs_str_to_apn(gbcfg.core_apn, 100, "foo.bar");
 
 	configure_sgsn_peer(&sgsn_peer);
 	configure_bss_peers(bss_peer, ARRAY_SIZE(bss_peer));
diff --git a/openbsc/tests/gprs/Makefile.am b/openbsc/tests/gprs/Makefile.am
index 904d567c1..193655fa4 100644
--- a/openbsc/tests/gprs/Makefile.am
+++ b/openbsc/tests/gprs/Makefile.am
@@ -5,4 +5,5 @@ EXTRA_DIST = gprs_test.ok
 
 noinst_PROGRAMS = gprs_test
 
-gprs_test_SOURCES = gprs_test.c
+gprs_test_SOURCES = gprs_test.c $(top_srcdir)/src/gprs/gprs_utils.c
+gprs_test_LDADD = $(LIBOSMOCORE_LIBS)
diff --git a/openbsc/tests/gprs/gprs_test.c b/openbsc/tests/gprs/gprs_test.c
index 7b0e64113..30e900f61 100644
--- a/openbsc/tests/gprs/gprs_test.c
+++ b/openbsc/tests/gprs/gprs_test.c
@@ -3,6 +3,7 @@
 #include <inttypes.h>
 
 #include <openbsc/gprs_llc.h>
+#include <openbsc/gprs_utils.h>
 
 #define ASSERT_FALSE(x) if (x)  { printf("Should have returned false.\n"); abort(); }
 #define ASSERT_TRUE(x)  if (!x) { printf("Should have returned true.\n"); abort(); }
@@ -42,9 +43,105 @@ static void test_8_4_2()
 	ASSERT_FALSE(nu_is_retransmission(479, 511)); // wrapped
 }
 
+static void apn_round_trip(const uint8_t *input, size_t len, const char *wanted_output)
+{
+	char output[len ? len : 1];
+	uint8_t encoded[len + 50];
+	char *out_str;
+	int enc_len;
+
+	/* decode and verify we have what we want */
+	out_str = gprs_apn_to_str(output, input, len);
+	OSMO_ASSERT(out_str);
+	OSMO_ASSERT(out_str == &output[0]);
+	OSMO_ASSERT(strlen(out_str) == strlen(wanted_output));
+	OSMO_ASSERT(strcmp(out_str, wanted_output) == 0);
+
+	/* encode and verify it */
+	if (len != 0) {
+		enc_len = gprs_str_to_apn(encoded, ARRAY_SIZE(encoded), wanted_output);
+		OSMO_ASSERT(enc_len == len);
+		OSMO_ASSERT(memcmp(encoded, input, enc_len) == 0);
+	} else {
+		enc_len = gprs_str_to_apn(encoded, 0, wanted_output);
+		OSMO_ASSERT(enc_len == -1);
+	}
+}
+
+static void test_gsm_03_03_apn(void)
+{
+
+	{
+		/* test invalid writes */
+		const uint8_t ref[10] = { 0xAB, 0xAC, 0xAD, 0xAE, 0xAF, 0xAB, 0xAC, 0xAD, 0xAE, 0xAF };
+		uint8_t output[10];
+		int enc_len;
+
+		memcpy(output, ref, ARRAY_SIZE(output));
+		enc_len = gprs_str_to_apn(output, 0, "");
+		OSMO_ASSERT(enc_len == -1);
+		OSMO_ASSERT(memcmp(ref, output, ARRAY_SIZE(ref)) == 0);
+
+		memcpy(output, ref, ARRAY_SIZE(output));
+		enc_len = gprs_str_to_apn(output, 0, "foo");
+		OSMO_ASSERT(enc_len == -1);
+		OSMO_ASSERT(memcmp(ref, output, ARRAY_SIZE(ref)) == 0);
+
+		memcpy(output, ref, ARRAY_SIZE(output));
+		enc_len = gprs_str_to_apn(output, 1, "foo");
+		OSMO_ASSERT(enc_len == -1);
+		OSMO_ASSERT(memcmp(ref + 1, output + 1, ARRAY_SIZE(ref) - 1) == 0);
+
+		memcpy(output, ref, ARRAY_SIZE(output));
+		enc_len = gprs_str_to_apn(output, 2, "foo");
+		OSMO_ASSERT(enc_len == -1);
+		OSMO_ASSERT(memcmp(ref + 2, output + 2, ARRAY_SIZE(ref) - 2) == 0);
+
+		memcpy(output, ref, ARRAY_SIZE(output));
+		enc_len = gprs_str_to_apn(output, 3, "foo");
+		OSMO_ASSERT(enc_len == -1);
+		OSMO_ASSERT(memcmp(ref + 3, output + 3, ARRAY_SIZE(ref) - 3) == 0);
+	}
+
+	{
+		/* single empty label */
+		uint8_t input[] = { 0x0 };
+		const char *output = "";
+		apn_round_trip(input, ARRAY_SIZE(input), output);
+	}
+
+	{
+		/* no label */
+		uint8_t input[] = { };
+		const char *output = "";
+		apn_round_trip(input, ARRAY_SIZE(input), output);
+	}
+
+	{
+		/* single label with A */
+		uint8_t input[] = { 0x1, 65 };
+		const char *output = "A";
+		apn_round_trip(input, ARRAY_SIZE(input), output);
+		OSMO_ASSERT(gprs_apn_to_str(NULL, input, ARRAY_SIZE(input) - 1) == NULL);
+	}
+
+	{
+		uint8_t input[] = { 0x3, 65, 66, 67, 0x2, 90, 122 };
+		const char *output = "ABC.Zz";
+		char tmp[strlen(output) + 1];
+		apn_round_trip(input, ARRAY_SIZE(input), output);
+		OSMO_ASSERT(gprs_apn_to_str(tmp, input, ARRAY_SIZE(input) - 1) == NULL);
+		OSMO_ASSERT(gprs_apn_to_str(tmp, input, ARRAY_SIZE(input) - 2) == NULL);
+		OSMO_ASSERT(gprs_apn_to_str(tmp, input, ARRAY_SIZE(input) - 4) == NULL);
+		OSMO_ASSERT(gprs_apn_to_str(tmp, input, ARRAY_SIZE(input) - 5) == NULL);
+		OSMO_ASSERT(gprs_apn_to_str(tmp, input, ARRAY_SIZE(input) - 6) == NULL);
+	}
+}
+
 int main(int argc, char **argv)
 {
 	test_8_4_2();
+	test_gsm_03_03_apn();
 
 	printf("Done.\n");
 	return EXIT_SUCCESS;