osmocom-bb[master]: mobile/gsm480_ss.c: use secure gsm_7bit_(en|de)code_n_ussd

[Harald Welte]

Patch Set 2: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/4641
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If8a1983592f5800e3981f29962eb333ac9473f40
Gerrit-PatchSet: 2
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Owner: Vadim Yanitskiy 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-HasComments: No

[MERGED] osmocom-bb[master]: mobile/gsm480_ss.c: use secure gsm_7bit_(en|de)code_n_ussd

[Harald Welte]

Harald Welte has submitted this change and it was merged.

Change subject: mobile/gsm480_ss.c: use secure gsm_7bit_(en|de)code_n_ussd
..


mobile/gsm480_ss.c: use secure gsm_7bit_(en|de)code_n_ussd

Since some 'gsm_7bit_*' functions were deprecated and replaced by
more secure ones with the '_n_' suffix in names, it's better to
use the updated functions.

Change-Id: If8a1983592f5800e3981f29962eb333ac9473f40
---
M src/host/layer23/src/mobile/gsm480_ss.c
1 file changed, 2 insertions(+), 6 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/host/layer23/src/mobile/gsm480_ss.c 
b/src/host/layer23/src/mobile/gsm480_ss.c
index 8d025e9..ff90faa 100644
--- a/src/host/layer23/src/mobile/gsm480_ss.c
+++ b/src/host/layer23/src/mobile/gsm480_ss.c
@@ -532,7 +532,7 @@
}
 
/* Encode service request */
-   length = gsm_7bit_encode(msg->data, text);
+   gsm_7bit_encode_n_ussd(msg->data, msgb_tailroom(msg), text, );
msgb_put(msg, length);
 
/* Then wrap it as an Octet String */
@@ -772,11 +772,7 @@
return -EINVAL;
}
num_chars = tag_len * 8 / 7;
-   /* Prevent a mobile-originated buffer-overrun! */
-   if (num_chars > sizeof(text) - 1)
-   num_chars = sizeof(text) - 1;
-   text[sizeof(text) - 1] = '\0';
-   gsm_7bit_decode(text, tag_data, num_chars);
+   gsm_7bit_decode_n_ussd(text, sizeof(text), tag_data, num_chars);
 
for (i = 0; text[i]; i++) {
if (text[i] == '\r')

-- 
To view, visit https://gerrit.osmocom.org/4641
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: If8a1983592f5800e3981f29962eb333ac9473f40
Gerrit-PatchSet: 2
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Owner: Vadim Yanitskiy 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 

[PATCH] osmocom-bb[master]: mobile/gsm480_ss.c: use secure gsm_7bit_(en|de)code_n_ussd

[Vadim Yanitskiy]

Hello Max, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4641

to look at the new patch set (#2).

mobile/gsm480_ss.c: use secure gsm_7bit_(en|de)code_n_ussd

Since some 'gsm_7bit_*' functions were deprecated and replaced by
more secure ones with the '_n_' suffix in names, it's better to
use the updated functions.

Change-Id: If8a1983592f5800e3981f29962eb333ac9473f40
---
M src/host/layer23/src/mobile/gsm480_ss.c
1 file changed, 2 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/41/4641/2

diff --git a/src/host/layer23/src/mobile/gsm480_ss.c 
b/src/host/layer23/src/mobile/gsm480_ss.c
index 8d025e9..ff90faa 100644
--- a/src/host/layer23/src/mobile/gsm480_ss.c
+++ b/src/host/layer23/src/mobile/gsm480_ss.c
@@ -532,7 +532,7 @@
}
 
/* Encode service request */
-   length = gsm_7bit_encode(msg->data, text);
+   gsm_7bit_encode_n_ussd(msg->data, msgb_tailroom(msg), text, );
msgb_put(msg, length);
 
/* Then wrap it as an Octet String */
@@ -772,11 +772,7 @@
return -EINVAL;
}
num_chars = tag_len * 8 / 7;
-   /* Prevent a mobile-originated buffer-overrun! */
-   if (num_chars > sizeof(text) - 1)
-   num_chars = sizeof(text) - 1;
-   text[sizeof(text) - 1] = '\0';
-   gsm_7bit_decode(text, tag_data, num_chars);
+   gsm_7bit_decode_n_ussd(text, sizeof(text), tag_data, num_chars);
 
for (i = 0; text[i]; i++) {
if (text[i] == '\r')

-- 
To view, visit https://gerrit.osmocom.org/4641
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If8a1983592f5800e3981f29962eb333ac9473f40
Gerrit-PatchSet: 2
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Owner: Vadim Yanitskiy 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 

osmocom-bb[master]: mobile/gsm480_ss.c: use secure gsm_7bit_(en|de)code_n_ussd

[Max]

Patch Set 1: Code-Review-1

(1 comment)

https://gerrit.osmocom.org/#/c/4641/1/src/host/layer23/src/mobile/gsm480_ss.c
File src/host/layer23/src/mobile/gsm480_ss.c:

Line 205:   if (response[i] == '\r')
That doesn't seem to be related to change to secure functions. And it also 
forces you to drop 'const' from response. Please clarify why this is necessary.


-- 
To view, visit https://gerrit.osmocom.org/4641
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If8a1983592f5800e3981f29962eb333ac9473f40
Gerrit-PatchSet: 1
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Owner: Vadim Yanitskiy 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-HasComments: Yes

[PATCH] osmocom-bb[master]: mobile/gsm480_ss.c: use secure gsm_7bit_(en|de)code_n_ussd

[Vadim Yanitskiy]

Review at  https://gerrit.osmocom.org/4641

mobile/gsm480_ss.c: use secure gsm_7bit_(en|de)code_n_ussd

Since some 'gsm_7bit_*' functions were deprecated and replaced by
more secure ones with the '_n_' suffix in names, it's better to
use the updated functions.

Change-Id: If8a1983592f5800e3981f29962eb333ac9473f40
---
M src/host/layer23/src/mobile/gsm480_ss.c
1 file changed, 14 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/41/4641/1

diff --git a/src/host/layer23/src/mobile/gsm480_ss.c 
b/src/host/layer23/src/mobile/gsm480_ss.c
index 8d025e9..6f06cf7 100644
--- a/src/host/layer23/src/mobile/gsm480_ss.c
+++ b/src/host/layer23/src/mobile/gsm480_ss.c
@@ -193,20 +193,23 @@
{ 0, NULL }
 };
 
-static int gsm480_ss_result(struct osmocom_ms *ms, const char *response,
+static int gsm480_ss_result(struct osmocom_ms *ms, char *response,
uint8_t error)
 {
vty_notify(ms, NULL);
if (response) {
-   char text[256], *t = text, *s;
+   char *line;
+   int i;
 
-   strncpy(text, response, sizeof(text) - 1);
-   text[sizeof(text) - 1] = '\0';
-   while ((s = strchr(text, '\r')))
-   *s = '\n';
-   while ((s = strsep(, "\n"))) {
-   vty_notify(ms, "Service response: %s\n", s);
-   }
+   for (i = 0; response[i]; i++)
+   if (response[i] == '\r')
+   response[i] = '\n';
+
+   if (response[0] && response[strlen(response) - 1] == '\n')
+   response[strlen(response) - 1] = '\0';
+
+   while ((line = strsep(, "\n")))
+   vty_notify(ms, "Service response: %s\n", line);
} else if (error)
vty_notify(ms, "Service request failed: %s\n",
get_value_string(gsm480_err_names, error));
@@ -532,7 +535,7 @@
}
 
/* Encode service request */
-   length = gsm_7bit_encode(msg->data, text);
+   gsm_7bit_encode_n_ussd(msg->data, msgb_tailroom(msg), text, );
msgb_put(msg, length);
 
/* Then wrap it as an Octet String */
@@ -730,7 +733,6 @@
 {
int num_chars;
char text[256];
-   int i;
const uint8_t *tag_data;
int tag_len;
 
@@ -772,19 +774,7 @@
return -EINVAL;
}
num_chars = tag_len * 8 / 7;
-   /* Prevent a mobile-originated buffer-overrun! */
-   if (num_chars > sizeof(text) - 1)
-   num_chars = sizeof(text) - 1;
-   text[sizeof(text) - 1] = '\0';
-   gsm_7bit_decode(text, tag_data, num_chars);
-
-   for (i = 0; text[i]; i++) {
-   if (text[i] == '\r')
-   text[i] = '\n';
-   }
-   /* remove last CR, if exists */
-   if (text[0] && text[strlen(text) - 1] == '\n')
-   text[strlen(text) - 1] = '\0';
+   gsm_7bit_decode_n_ussd(text, sizeof(text), tag_data, num_chars);
gsm480_ss_result(trans->ms, text, 0);
 
return 0;

-- 
To view, visit https://gerrit.osmocom.org/4641
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: If8a1983592f5800e3981f29962eb333ac9473f40
Gerrit-PatchSet: 1
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Owner: Vadim Yanitskiy