diff mbox

[4/4] nitb: Fix IMSI/IMEI buffer handling (Coverity)

Message ID 1428421790-3423-4-git-send-email-jerlbeck@sysmocom.de
State Accepted
Headers show

Commit Message

Jacob Erlbeck April 7, 2015, 3:49 p.m. UTC 
Currently the handling of the buffers is not done consistently. Some
code assumes that the whole buffer may be used to store the string
while at other places, the last buffer byte is left untouched in the
assumption that it contains a terminating NUL-character. The latter
is the correct behaviour.

This commit changes to code to not touch the last byte in the buffers
and to rely on the last byte being NUL. So the maximum IMSI/IMEI
length is GSM_IMSI_LENGTH-1/GSM_IMEI_LENGTH-1.

Fixes: Coverity CID 1206568, 1206567
Sponsored-by: On-Waves ehf
---
 openbsc/src/libcommon/gsm_subscriber_base.c | 3 +--
 openbsc/src/libmsc/db.c                     | 2 +-
 openbsc/src/osmo-bsc_nat/bsc_ussd.c         | 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

Comments

Holger Freyther April 7, 2015, 6:13 p.m. UTC | #1 
On Tue, Apr 07, 2015 at 05:49:50PM +0200, Jacob Erlbeck wrote:

> This commit changes to code to not touch the last byte in the buffers
> and to rely on the last byte being NUL. So the maximum IMSI/IMEI
> length is GSM_IMSI_LENGTH-1/GSM_IMEI_LENGTH-1.

For information: "rely" here means.. we assume that we allocate the
structure with talloc_zero. This means we have NULed the entire imsi
array and then only write sizeof - 1 characters to it. So the last
byte remains NUL
diff mbox

Patch

diff --git a/openbsc/src/libcommon/gsm_subscriber_base.c b/openbsc/src/libcommon/gsm_subscriber_base.c
index 3c56101..a455824 100644
--- a/openbsc/src/libcommon/gsm_subscriber_base.c
+++ b/openbsc/src/libcommon/gsm_subscriber_base.c
@@ -112,8 +112,7 @@  struct gsm_subscriber *subscr_get_or_create(struct gsm_subscriber_group *sgrp,
 	if (!subscr)
 		return NULL;
 
-	strncpy(subscr->imsi, imsi, GSM_IMSI_LENGTH);
-	subscr->imsi[GSM_IMSI_LENGTH - 1] = '\0';
+	strncpy(subscr->imsi, imsi, GSM_IMSI_LENGTH-1);
 	subscr->group = sgrp;
 	return subscr;
 }
diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c
index bdecbb4..ee678b6 100644
--- a/openbsc/src/libmsc/db.c
+++ b/openbsc/src/libmsc/db.c
@@ -802,7 +802,7 @@  static void db_set_from_query(struct gsm_subscriber *subscr, dbi_conn result)
 	const char *string;
 	string = dbi_result_get_string(result, "imsi");
 	if (string)
-		strncpy(subscr->imsi, string, GSM_IMSI_LENGTH);
+		strncpy(subscr->imsi, string, GSM_IMSI_LENGTH-1);
 
 	string = dbi_result_get_string(result, "tmsi");
 	if (string)
diff --git a/openbsc/src/osmo-bsc_nat/bsc_ussd.c b/openbsc/src/osmo-bsc_nat/bsc_ussd.c
index ac5a9f5..67844b8 100644
--- a/openbsc/src/osmo-bsc_nat/bsc_ussd.c
+++ b/openbsc/src/osmo-bsc_nat/bsc_ussd.c
@@ -399,7 +399,7 @@  int bsc_ussd_check(struct nat_sccp_connection *con, struct bsc_nat_parsed *parse
 	if (parsed->bssap != BSSAP_MSG_DTAP)
 		return 0;
 
-	if (strlen(con->imsi) > GSM_IMSI_LENGTH)
+	if (strlen(con->imsi) >= GSM_IMSI_LENGTH)
 		return 0;
 
 	hdr48 = bsc_unpack_dtap(parsed, msg, &len);