From ce1b22e817584d512f82a711c02b67d12346c202 Mon Sep 17 00:00:00 2001 From: Holger Hans Peter Freyther Date: Mon, 4 Aug 2014 14:22:13 +0200 Subject: 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 --- openbsc/include/openbsc/gprs_utils.h | 2 +- openbsc/src/gprs/gb_proxy_vty.c | 2 +- openbsc/src/gprs/gprs_utils.c | 22 ++++++-- openbsc/tests/gbproxy/gbproxy_test.c | 2 +- openbsc/tests/gprs/Makefile.am | 3 +- openbsc/tests/gprs/gprs_test.c | 97 ++++++++++++++++++++++++++++++++++++ 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 #include +#include #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; -- cgit v1.2.3