ussd: Add a test case, switch parsing to use a gsm48_hdr and len

The current USSD code is not doing any size checks, add a test
case to find out how easily we access the data out of bounds.
Begin to use the length in some places.

6 files changed, 98 insertions, 22 deletions

diff --git a/configure.in b/configure.in
index 4c827b06..a8e3f20f 100644
--- a/configure.in
+++ b/configure.in
@@ -112,4 +112,5 @@ AC_OUTPUT(
 	tests/timer/Makefile
 	tests/sms/Makefile
 	tests/msgfile/Makefile
+	tests/ussd/Makefile
 	Makefile)
diff --git a/include/osmocore/gsm0480.h b/include/osmocore/gsm0480.h
index ae92c1f9..94ac7174 100644
--- a/include/osmocore/gsm0480.h
+++ b/include/osmocore/gsm0480.h
@@ -2,6 +2,8 @@
 #define gsm0480_h
 
 #include "msgb.h"
+#include "protocol/gsm_04_08.h"
+#include "protocol/gsm_04_80.h"
 
 #define MAX_LEN_USSD_STRING	31
 
@@ -11,7 +13,7 @@ struct ussd_request {
 	uint8_t invoke_id;
 };
 
-int gsm0480_decode_ussd_request(const struct msgb *msg,
+int gsm0480_decode_ussd_request(const struct gsm48_hdr *hdr, uint16_t len,
 				struct ussd_request *request);
 
 struct msgb *gsm0480_create_unstructuredSS_Notify(int alertPattern, const char *text);
diff --git a/src/gsm0480.c b/src/gsm0480.c
index 7d607a95..4c1a12a7 100644
--- a/src/gsm0480.c
+++ b/src/gsm0480.c
@@ -192,25 +192,26 @@ struct msgb *gsm0480_create_notifySS(const char *text)
 }
 
 /* Forward declarations */
-static int parse_ussd(uint8_t *ussd, struct ussd_request *req);
-static int parse_ussd_info_elements(uint8_t *ussd_ie,
+static int parse_ussd(const struct gsm48_hdr *hdr,
+		      uint16_t len, struct ussd_request *req);
+static int parse_ussd_info_elements(const uint8_t *ussd_ie, uint16_t len,
 					struct ussd_request *req);
-static int parse_facility_ie(uint8_t *facility_ie, uint8_t length,
+static int parse_facility_ie(const uint8_t *facility_ie, uint8_t length,
 					struct ussd_request *req);
-static int parse_ss_invoke(uint8_t *invoke_data, uint8_t length,
+static int parse_ss_invoke(const uint8_t *invoke_data, uint8_t length,
 					struct ussd_request *req);
-static int parse_process_uss_req(uint8_t *uss_req_data, uint8_t length,
+static int parse_process_uss_req(const uint8_t *uss_req_data, uint8_t length,
 					struct ussd_request *req);
 
 /* Decode a mobile-originated USSD-request message */
-int gsm0480_decode_ussd_request(const struct msgb *msg, struct ussd_request *req)
+int gsm0480_decode_ussd_request(const struct gsm48_hdr *hdr, uint16_t len,
+				struct ussd_request *req)
 {
 	int rc = 0;
-	uint8_t *parse_ptr = msgb_l3(msg);
 
-	if ((*parse_ptr & 0x0F) == GSM48_PDISC_NC_SS) {
-		req->transaction_id = *parse_ptr & 0x70;
-		rc = parse_ussd(parse_ptr+1, req);
+	if ((hdr->proto_discr & 0x0f) == GSM48_PDISC_NC_SS) {
+		req->transaction_id = hdr->proto_discr & 0x70;
+		rc = parse_ussd(hdr, len, req);
 	}
 
 	if (!rc)
@@ -219,10 +220,10 @@ int gsm0480_decode_ussd_request(const struct msgb *msg, struct ussd_request *req
 	return rc;
 }
 
-static int parse_ussd(uint8_t *ussd, struct ussd_request *req)
+static int parse_ussd(const struct gsm48_hdr *hdr, uint16_t len, struct ussd_request *req)
 {
 	int rc = 1;
-	uint8_t msg_type = ussd[0] & 0xBF;  /* message-type - section 3.4 */
+	uint8_t msg_type = hdr->msg_type & 0xBF;  /* message-type - section 3.4 */
 
 	switch (msg_type) {
 	case GSM0480_MTYPE_RELEASE_COMPLETE:
@@ -232,11 +233,11 @@ static int parse_ussd(uint8_t *ussd, struct ussd_request *req)
 		break;
 	case GSM0480_MTYPE_REGISTER:
 	case GSM0480_MTYPE_FACILITY:
-		rc &= parse_ussd_info_elements(ussd+1, req);
+		rc &= parse_ussd_info_elements(&hdr->data[0], len - sizeof(*hdr), req);
 		break;
 	default:
 		LOGP(0, LOGL_DEBUG, "Unknown GSM 04.80 message-type field 0x%02x\n",
-			ussd[0]);
+			hdr->msg_type);
 		rc = 0;
 		break;
 	}
@@ -244,12 +245,16 @@ static int parse_ussd(uint8_t *ussd, struct ussd_request *req)
 	return rc;
 }
 
-static int parse_ussd_info_elements(uint8_t *ussd_ie, struct ussd_request *req)
+static int parse_ussd_info_elements(const uint8_t *ussd_ie, uint16_t len,
+				    struct ussd_request *req)
 {
 	int rc = -1;
 	/* Information Element Identifier - table 3.2 & GSM 04.08 section 10.5 */
-	uint8_t iei = ussd_ie[0];
-	uint8_t iei_length = ussd_ie[1];
+	uint8_t iei;
+	uint8_t iei_length;
+
+	iei = ussd_ie[0];
+	iei_length = ussd_ie[1];
 
 	switch (iei) {
 	case GSM48_IE_CAUSE:
@@ -269,7 +274,7 @@ static int parse_ussd_info_elements(uint8_t *ussd_ie, struct ussd_request *req)
 	return rc;
 }
 
-static int parse_facility_ie(uint8_t *facility_ie, uint8_t length,
+static int parse_facility_ie(const uint8_t *facility_ie, uint8_t length,
 						struct ussd_request *req)
 {
 	int rc = 1;
@@ -305,7 +310,7 @@ static int parse_facility_ie(uint8_t *facility_ie, uint8_t length,
 }
 
 /* Parse an Invoke component - see table 3.3 */
-static int parse_ss_invoke(uint8_t *invoke_data, uint8_t length,
+static int parse_ss_invoke(const uint8_t *invoke_data, uint8_t length,
 						struct ussd_request *req)
 {
 	int rc = 1;
@@ -350,7 +355,7 @@ static int parse_ss_invoke(uint8_t *invoke_data, uint8_t length,
 }
 
 /* Parse the parameters of a Process UnstructuredSS Request */
-static int parse_process_uss_req(uint8_t *uss_req_data, uint8_t length,
+static int parse_process_uss_req(const uint8_t *uss_req_data, uint8_t length,
 					struct ussd_request *req)
 {
 	int rc = 0;
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 14c8ca87..0166b4f2 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1,5 +1,5 @@
 if ENABLE_TESTS
-SUBDIRS = timer sms
+SUBDIRS = timer sms ussd
 if ENABLE_MSGFILE
 SUBDIRS += msgfile
 endif
diff --git a/tests/ussd/Makefile.am b/tests/ussd/Makefile.am
new file mode 100644
index 00000000..d29506cc
--- /dev/null
+++ b/tests/ussd/Makefile.am
@@ -0,0 +1,5 @@
+INCLUDES = $(all_includes) -I$(top_srcdir)/include
+noinst_PROGRAMS = ussd_test
+
+ussd_test_SOURCES = ussd_test.c
+ussd_test_LDADD = $(top_builddir)/src/libosmocore.la
diff --git a/tests/ussd/ussd_test.c b/tests/ussd/ussd_test.c
new file mode 100644
index 00000000..4d125ffd
--- /dev/null
+++ b/tests/ussd/ussd_test.c
@@ -0,0 +1,63 @@
+/*
+ * (C) 2010 by Holger Hans Peter Freyther
+ * (C) 2010 by On-Waves
+ * All Rights Reserved
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include <osmocore/gsm0480.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+static const uint8_t ussd_request[] = {
+	0x0b, 0x7b, 0x1c, 0x15, 0xa1, 0x13, 0x02, 0x01,
+	0x03, 0x02, 0x01, 0x3b, 0x30, 0x0b, 0x04, 0x01,
+	0x0f, 0x04, 0x06, 0x2a, 0xd5, 0x4c, 0x16, 0x1b,
+	0x01, 0x7f, 0x01, 0x00
+};
+
+static int parse_ussd(const uint8_t *_data, int len)
+{
+	uint8_t *data;
+	int rc;
+	struct ussd_request req;
+	struct gsm48_hdr *hdr;
+
+	data = malloc(len);
+	memcpy(data, _data, len);
+	hdr = (struct gsm48_hdr *) &data[0];
+	rc = gsm0480_decode_ussd_request(hdr, len, &req);
+	free(data);
+
+	return rc;
+}
+
+int main(int argc, char **argv)
+{
+	const int size = sizeof(ussd_request);
+	int i;
+
+	printf("Testing parsing a USSD request and truncated versions\n");
+
+	for (i = size; i > sizeof(struct gsm48_hdr); --i) {
+		int rc = parse_ussd(&ussd_request[0], i);
+		printf("Result for %d is %d\n", rc, i);
+	}
+
+	return 0;
+}