Change in ...osmo-hlr[master]: rx_check_imei_req(): fix IMEI bounds checking
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/14399 ) Change subject: rx_check_imei_req(): fix IMEI bounds checking .. Patch Set 3: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6 Gerrit-Change-Number: 14399 Gerrit-PatchSet: 3 Gerrit-Owner: osmith Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Thu, 13 Jun 2019 15:34:35 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-hlr[master]: rx_check_imei_req(): fix IMEI bounds checking
laforge has submitted this change and it was merged. (
https://gerrit.osmocom.org/c/osmo-hlr/+/14399 )
Change subject: rx_check_imei_req(): fix IMEI bounds checking
..
rx_check_imei_req(): fix IMEI bounds checking
IMEIs (without the checksum) always have 14 digits. Replace the previous
check (length <= 14) with a proper one (length == 14) and set the buffer
to the right size. While at it, add the return code of
gsm48_decode_bc_number2() to the error log message.
I have tested with new TTCN3 tests, that the length check is working
properly now.
Related: OS#2541
Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6
---
M src/hlr.c
1 file changed, 17 insertions(+), 7 deletions(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, but someone else must approve
laforge: Looks good to me, approved
diff --git a/src/hlr.c b/src/hlr.c
index 33d2828..90cbac4 100644
--- a/src/hlr.c
+++ b/src/hlr.c
@@ -477,18 +477,28 @@
{
struct osmo_gsup_message gsup_reply = {0};
struct msgb *msg_out;
- char imei[GSM23003_IMEI_NUM_DIGITS+1] = {0};
+ char imei[GSM23003_IMEI_NUM_DIGITS_NO_CHK+1] = {0};
+ int rc;
- /* Encoded IMEI length check */
- if (!gsup->imei_enc || gsup->imei_enc_len < 1 || gsup->imei_enc[0] >=
sizeof(imei)) {
- LOGP(DMAIN, LOGL_ERROR, "%s: wrong encoded IMEI length\n",
gsup->imsi);
+ /* Require IMEI */
+ if (!gsup->imei_enc) {
+ LOGP(DMAIN, LOGL_ERROR, "%s: missing IMEI\n", gsup->imsi);
gsup_send_err_reply(conn, gsup->imsi, gsup->message_type,
GMM_CAUSE_INV_MAND_INFO);
return -1;
}
- /* Decode IMEI */
- if (gsm48_decode_bcd_number2(imei, sizeof(imei), gsup->imei_enc,
gsup->imei_enc_len, 0) < 0) {
- LOGP(DMAIN, LOGL_ERROR, "%s: failed to decode IMEI\n",
gsup->imsi);
+ /* Decode IMEI (fails if IMEI is too long) */
+ rc = gsm48_decode_bcd_number2(imei, sizeof(imei), gsup->imei_enc,
gsup->imei_enc_len, 0);
+ if (rc < 0) {
+ LOGP(DMAIN, LOGL_ERROR, "%s: failed to decode IMEI (rc: %i)\n",
gsup->imsi, rc);
+ gsup_send_err_reply(conn, gsup->imsi, gsup->message_type,
GMM_CAUSE_INV_MAND_INFO);
+ return -1;
+ }
+
+ /* Check if IMEI is too short */
+ if (strlen(imei) != GSM23003_IMEI_NUM_DIGITS_NO_CHK) {
+ LOGP(DMAIN, LOGL_ERROR, "%s: wrong encoded IMEI length (IMEI:
'%s', %lu, %i)\n", gsup->imsi, imei,
+strlen(imei), GSM23003_IMEI_NUM_DIGITS_NO_CHK);
gsup_send_err_reply(conn, gsup->imsi, gsup->message_type,
GMM_CAUSE_INV_MAND_INFO);
return -1;
}
--
To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6
Gerrit-Change-Number: 14399
Gerrit-PatchSet: 3
Gerrit-Owner: osmith
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: laforge
Gerrit-Reviewer: osmith
Gerrit-Reviewer: pespin
Gerrit-MessageType: merged
Change in ...osmo-hlr[master]: rx_check_imei_req(): fix IMEI bounds checking
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/14399 ) Change subject: rx_check_imei_req(): fix IMEI bounds checking .. Patch Set 3: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6 Gerrit-Change-Number: 14399 Gerrit-PatchSet: 3 Gerrit-Owner: osmith Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 11 Jun 2019 08:43:03 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-hlr[master]: rx_check_imei_req(): fix IMEI bounds checking
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/14399 ) Change subject: rx_check_imei_req(): fix IMEI bounds checking .. Patch Set 3: (1 comment) https://gerrit.osmocom.org/#/c/14399/1/src/hlr.c File src/hlr.c: https://gerrit.osmocom.org/#/c/14399/1/src/hlr.c@483 PS1, Line 483: gsm48_decode_bcd_number2 > > [...] we would need to add another if() block beforehand [...] […] Done -- To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6 Gerrit-Change-Number: 14399 Gerrit-PatchSet: 3 Gerrit-Owner: osmith Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 11 Jun 2019 06:47:28 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Comment-In-Reply-To: osmith Gerrit-MessageType: comment
Change in ...osmo-hlr[master]: rx_check_imei_req(): fix IMEI bounds checking
Hello fixeria, pespin, laforge, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399 to look at the new patch set (#3). Change subject: rx_check_imei_req(): fix IMEI bounds checking .. rx_check_imei_req(): fix IMEI bounds checking IMEIs (without the checksum) always have 14 digits. Replace the previous check (length <= 14) with a proper one (length == 14) and set the buffer to the right size. While at it, add the return code of gsm48_decode_bc_number2() to the error log message. I have tested with new TTCN3 tests, that the length check is working properly now. Related: OS#2541 Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6 --- M src/hlr.c 1 file changed, 17 insertions(+), 7 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-hlr refs/changes/99/14399/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6 Gerrit-Change-Number: 14399 Gerrit-PatchSet: 3 Gerrit-Owner: osmith Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-MessageType: newpatchset
Change in ...osmo-hlr[master]: rx_check_imei_req(): fix IMEI bounds checking
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/14399 ) Change subject: rx_check_imei_req(): fix IMEI bounds checking .. Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6 Gerrit-Change-Number: 14399 Gerrit-PatchSet: 2 Gerrit-Owner: osmith Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Sat, 08 Jun 2019 09:59:37 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-hlr[master]: rx_check_imei_req(): fix IMEI bounds checking
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/14399 ) Change subject: rx_check_imei_req(): fix IMEI bounds checking .. Patch Set 1: (1 comment) https://gerrit.osmocom.org/#/c/14399/1/src/hlr.c File src/hlr.c: https://gerrit.osmocom.org/#/c/14399/1/src/hlr.c@483 PS1, Line 483: gsm48_decode_bcd_number2 > [...] we would need to add another if() block beforehand [...] Yep, so then you could print a better message than "failed to decode", if there was actually nothing to decode ;) -- To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6 Gerrit-Change-Number: 14399 Gerrit-PatchSet: 1 Gerrit-Owner: osmith Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Fri, 07 Jun 2019 13:20:41 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Comment-In-Reply-To: osmith Gerrit-MessageType: comment
Change in ...osmo-hlr[master]: rx_check_imei_req(): fix IMEI bounds checking
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/14399 ) Change subject: rx_check_imei_req(): fix IMEI bounds checking .. Patch Set 2: (2 comments) https://gerrit.osmocom.org/#/c/14399/1/src/hlr.c File src/hlr.c: https://gerrit.osmocom.org/#/c/14399/1/src/hlr.c@a483 PS1, Line 483: > AFAIR, gsm48_decode_bcd_number2() is not NULL-safe, so I would keep this > check. […] Good catch, fixed. https://gerrit.osmocom.org/#/c/14399/1/src/hlr.c@483 PS1, Line 483: gsm48_decode_bcd_number2 > Cosmetic, not critical: it would be good to store the returned value, and > print it in the error mess […] Agreed, but together with the !gsup->imei_enc before, we would need to add another if() block beforehand, so I'll leave it like this for now. -- To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6 Gerrit-Change-Number: 14399 Gerrit-PatchSet: 2 Gerrit-Owner: osmith Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Fri, 07 Jun 2019 13:16:02 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Gerrit-MessageType: comment
Change in ...osmo-hlr[master]: rx_check_imei_req(): fix IMEI bounds checking
Hello fixeria, pespin, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399 to look at the new patch set (#2). Change subject: rx_check_imei_req(): fix IMEI bounds checking .. rx_check_imei_req(): fix IMEI bounds checking IMEIs (without the checksum) always have 14 digits. Replace the previous check (length <= 14) with a proper one (length == 14) and set the buffer to the right size. I have tested with new TTCN3 tests, that the length check is working properly now. Related: OS#2541 Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6 --- M src/hlr.c 1 file changed, 8 insertions(+), 7 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-hlr refs/changes/99/14399/2 -- To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6 Gerrit-Change-Number: 14399 Gerrit-PatchSet: 2 Gerrit-Owner: osmith Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-MessageType: newpatchset
Change in ...osmo-hlr[master]: rx_check_imei_req(): fix IMEI bounds checking
fixeria has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-hlr/+/14399 )
Change subject: rx_check_imei_req(): fix IMEI bounds checking
..
Patch Set 1:
(2 comments)
https://gerrit.osmocom.org/#/c/14399/1/src/hlr.c
File src/hlr.c:
https://gerrit.osmocom.org/#/c/14399/1/src/hlr.c@a483
PS1, Line 483:
AFAIR, gsm48_decode_bcd_number2() is not NULL-safe, so I would keep this check.
Others can be safely removed, for sure.
https://gerrit.osmocom.org/#/c/14399/1/src/hlr.c@483
PS1, Line 483: gsm48_decode_bcd_number2
Cosmetic, not critical: it would be good to store the returned value, and print
it in the error message below. I find it helpful for trouble shooting.
rc = gsm48_decode_bcd_number2(...);
if (rc) { ... }
--
To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6
Gerrit-Change-Number: 14399
Gerrit-PatchSet: 1
Gerrit-Owner: osmith
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: osmith
Gerrit-Reviewer: pespin
Gerrit-Comment-Date: Fri, 07 Jun 2019 12:44:11 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Change in ...osmo-hlr[master]: rx_check_imei_req(): fix IMEI bounds checking
osmith has uploaded this change for review. (
https://gerrit.osmocom.org/c/osmo-hlr/+/14399
Change subject: rx_check_imei_req(): fix IMEI bounds checking
..
rx_check_imei_req(): fix IMEI bounds checking
IMEIs (without the checksum) always have 14 digits. Replace the previous
check (length <= 14) with a proper one (length == 14) and set the buffer
to the right size.
I have tested with new TTCN3 tests, that the length check is working
properly now.
Related: OS#2541
Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6
---
M src/hlr.c
1 file changed, 8 insertions(+), 7 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-hlr refs/changes/99/14399/1
diff --git a/src/hlr.c b/src/hlr.c
index 33d2828..32a584e 100644
--- a/src/hlr.c
+++ b/src/hlr.c
@@ -477,18 +477,19 @@
{
struct osmo_gsup_message gsup_reply = {0};
struct msgb *msg_out;
- char imei[GSM23003_IMEI_NUM_DIGITS+1] = {0};
+ char imei[GSM23003_IMEI_NUM_DIGITS_NO_CHK+1] = {0};
- /* Encoded IMEI length check */
- if (!gsup->imei_enc || gsup->imei_enc_len < 1 || gsup->imei_enc[0] >=
sizeof(imei)) {
- LOGP(DMAIN, LOGL_ERROR, "%s: wrong encoded IMEI length\n",
gsup->imsi);
+ /* Decode IMEI (fails if IMEI is too long) */
+ if (gsm48_decode_bcd_number2(imei, sizeof(imei), gsup->imei_enc,
gsup->imei_enc_len, 0) < 0) {
+ LOGP(DMAIN, LOGL_ERROR, "%s: failed to decode IMEI\n",
gsup->imsi);
gsup_send_err_reply(conn, gsup->imsi, gsup->message_type,
GMM_CAUSE_INV_MAND_INFO);
return -1;
}
- /* Decode IMEI */
- if (gsm48_decode_bcd_number2(imei, sizeof(imei), gsup->imei_enc,
gsup->imei_enc_len, 0) < 0) {
- LOGP(DMAIN, LOGL_ERROR, "%s: failed to decode IMEI\n",
gsup->imsi);
+ /* Check if IMEI is too short */
+ if (strlen(imei) != GSM23003_IMEI_NUM_DIGITS_NO_CHK) {
+ LOGP(DMAIN, LOGL_ERROR, "%s: wrong encoded IMEI length (IMEI:
'%s', %lu, %i)\n", gsup->imsi, imei,
+strlen(imei), GSM23003_IMEI_NUM_DIGITS_NO_CHK);
gsup_send_err_reply(conn, gsup->imsi, gsup->message_type,
GMM_CAUSE_INV_MAND_INFO);
return -1;
}
--
To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/14399
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Change-Id: I060a8db98fb882e4815d1709a5d85bc0143a73a6
Gerrit-Change-Number: 14399
Gerrit-PatchSet: 1
Gerrit-Owner: osmith
Gerrit-MessageType: newchange
