Re: [PATCH] sim-auth: Parse auth response according to TS 31.102
Hi Slava,
On 5/3/21 9:04 AM, Slava Monich wrote:
---
So what is this trying to fix? I mean I can understand 1 line patch fixes that
come without a description. These tend to be self-evident. 300+ line diff
needs a least some explanation.
src/sim-auth.c | 34 ++-
src/simutil.c | 146 +---
src/simutil.h | 13 ++--
unit/test-simutil.c | 125 -
4 files changed, 234 insertions(+), 84 deletions(-)
This really should be broken down into at least 2 commits. See HACKING,
'Submitting Patches' section, item 3.
diff --git a/src/sim-auth.c b/src/sim-auth.c
index 3c3f35e7..6dab52ee 100644
--- a/src/sim-auth.c
+++ b/src/sim-auth.c
@@ -207,14 +207,10 @@ static void handle_umts(struct ofono_sim_auth *sa, const
uint8_t *resp,
DBusMessage *reply = NULL;
DBusMessageIter iter;
DBusMessageIter dict;
- const uint8_t *res = NULL;
- const uint8_t *ck = NULL;
- const uint8_t *ik = NULL;
- const uint8_t *auts = NULL;
- const uint8_t *kc = NULL;
+ struct data_block res, ck, ik, auts, sres, kc;
if (!sim_parse_umts_authenticate(resp, len, , , ,
- , ))
+ , , ))
goto umts_end;
reply = dbus_message_new_method_return(sa->pending->msg);
@@ -224,15 +220,23 @@ static void handle_umts(struct ofono_sim_auth *sa, const
uint8_t *resp,
dbus_message_iter_open_container(, DBUS_TYPE_ARRAY,
"{say}", );
- if (auts) {
- append_dict_byte_array(, "AUTS", auts, 14);
- } else {
- append_dict_byte_array(, "RES", res, 8);
- append_dict_byte_array(, "CK", ck, 16);
- append_dict_byte_array(, "IK", ik, 16);
- if (kc)
- append_dict_byte_array(, "Kc", kc, 8);
- }
+ if (auts.data)
+ append_dict_byte_array(, "AUTS", auts.data, auts.len);
+
+ if (sres.data)
+ append_dict_byte_array(, "SRES", sres.data, sres.len);
You're modifying the behavior of the D-Bus API without actually updating the
relevant docs.
Also, this part looks suspect. SRES is only done in GSM context, so shouldn't
this be taken care sim_parse_gsm_authenticate()?
+
+ if (res.data)
+ append_dict_byte_array(, "RES", res.data, res.len);
+
+ if (ck.data)
+ append_dict_byte_array(, "CK", ck.data, ck.len);
+
+ if (ik.data)
+ append_dict_byte_array(, "IK", ik.data, ik.len);
+
+ if (kc.data)
+ append_dict_byte_array(, "Kc", kc.data, kc.len);
dbus_message_iter_close_container(, );
diff --git a/src/simutil.c b/src/simutil.c
index 5d2aa6a2..30b76f05 100644
--- a/src/simutil.c
+++ b/src/simutil.c
@@ -1672,63 +1672,135 @@ int sim_build_gsm_authenticate(unsigned char *buffer,
int len,
return build_authenticate(buffer, rand, NULL);
}
-gboolean sim_parse_umts_authenticate(const unsigned char *buffer,
- int len, const unsigned char **res, const unsigned char **ck,
- const unsigned char **ik, const unsigned char **auts,
- const unsigned char **kc)
+gboolean sim_parse_umts_authenticate(const unsigned char *buffer, int len,
+ struct data_block *res, struct data_block *ck,
+ struct data_block *ik, struct data_block *auts,
+ struct data_block *sres, struct data_block *kc)
At least by my reading of 33.102, lengths of ck, ik, kc and sres are set in
stone, are they not?
Also, RFC 4187 also sets in stone the definition of AUTS (and RES, AUTN, etc).
So you probably have to explain how and when AUTS length could be not in line
with that RFC? The authentication API is mostly meant to be used by
implementations of this RFC, so we would need to take appropriate steps that
nothing is broken by this change.
{
- if (len < 16 || !buffer)
+ const unsigned char *ptr = buffer;
+ const unsigned char *end = ptr + len;
+ unsigned int l;
+
+ if (!buffer || len < 2)
return FALSE;
- switch (buffer[0]) {
+ memset(res, 0, sizeof(*res));
+ memset(ck, 0, sizeof(*ck));
+ memset(ik, 0, sizeof(*ik));
+ memset(kc, 0, sizeof(*kc));
+ memset(auts, 0, sizeof(*auts));
+ memset(sres, 0, sizeof(*sres));
Generally we don't like side-effecting arguments on error. If you're going
through the trouble of introducing a new structure, might as well just have it
represent the result of the parse operation and cut down on arguments. Maybe
something like:
struct sim_auth_umts_result {
uint8_t success;
union {
struct {
const unsigned char *auts;
uint8_t auts_len;
}
struct {
const unsigned char
[PATCH] sim-auth: Parse auth response according to TS 31.102
---
src/sim-auth.c | 34 ++-
src/simutil.c | 146 +---
src/simutil.h | 13 ++--
unit/test-simutil.c | 125 -
4 files changed, 234 insertions(+), 84 deletions(-)
diff --git a/src/sim-auth.c b/src/sim-auth.c
index 3c3f35e7..6dab52ee 100644
--- a/src/sim-auth.c
+++ b/src/sim-auth.c
@@ -207,14 +207,10 @@ static void handle_umts(struct ofono_sim_auth *sa, const
uint8_t *resp,
DBusMessage *reply = NULL;
DBusMessageIter iter;
DBusMessageIter dict;
- const uint8_t *res = NULL;
- const uint8_t *ck = NULL;
- const uint8_t *ik = NULL;
- const uint8_t *auts = NULL;
- const uint8_t *kc = NULL;
+ struct data_block res, ck, ik, auts, sres, kc;
if (!sim_parse_umts_authenticate(resp, len, , , ,
- , ))
+ , , ))
goto umts_end;
reply = dbus_message_new_method_return(sa->pending->msg);
@@ -224,15 +220,23 @@ static void handle_umts(struct ofono_sim_auth *sa, const
uint8_t *resp,
dbus_message_iter_open_container(, DBUS_TYPE_ARRAY,
"{say}", );
- if (auts) {
- append_dict_byte_array(, "AUTS", auts, 14);
- } else {
- append_dict_byte_array(, "RES", res, 8);
- append_dict_byte_array(, "CK", ck, 16);
- append_dict_byte_array(, "IK", ik, 16);
- if (kc)
- append_dict_byte_array(, "Kc", kc, 8);
- }
+ if (auts.data)
+ append_dict_byte_array(, "AUTS", auts.data, auts.len);
+
+ if (sres.data)
+ append_dict_byte_array(, "SRES", sres.data, sres.len);
+
+ if (res.data)
+ append_dict_byte_array(, "RES", res.data, res.len);
+
+ if (ck.data)
+ append_dict_byte_array(, "CK", ck.data, ck.len);
+
+ if (ik.data)
+ append_dict_byte_array(, "IK", ik.data, ik.len);
+
+ if (kc.data)
+ append_dict_byte_array(, "Kc", kc.data, kc.len);
dbus_message_iter_close_container(, );
diff --git a/src/simutil.c b/src/simutil.c
index 5d2aa6a2..30b76f05 100644
--- a/src/simutil.c
+++ b/src/simutil.c
@@ -1672,63 +1672,135 @@ int sim_build_gsm_authenticate(unsigned char *buffer,
int len,
return build_authenticate(buffer, rand, NULL);
}
-gboolean sim_parse_umts_authenticate(const unsigned char *buffer,
- int len, const unsigned char **res, const unsigned char **ck,
- const unsigned char **ik, const unsigned char **auts,
- const unsigned char **kc)
+gboolean sim_parse_umts_authenticate(const unsigned char *buffer, int len,
+ struct data_block *res, struct data_block *ck,
+ struct data_block *ik, struct data_block *auts,
+ struct data_block *sres, struct data_block *kc)
{
- if (len < 16 || !buffer)
+ const unsigned char *ptr = buffer;
+ const unsigned char *end = ptr + len;
+ unsigned int l;
+
+ if (!buffer || len < 2)
return FALSE;
- switch (buffer[0]) {
+ memset(res, 0, sizeof(*res));
+ memset(ck, 0, sizeof(*ck));
+ memset(ik, 0, sizeof(*ik));
+ memset(kc, 0, sizeof(*kc));
+ memset(auts, 0, sizeof(*auts));
+ memset(sres, 0, sizeof(*sres));
+
+ /*
+* TS 31.102
+* 7.1.2.1 GSM/3G security context
+*/
+ switch (*ptr++) {
case 0xdb:
- /* 'DB' + '08' + RES(16) + '10' + CK(32) + '10' + IK(32) = 43 */
- if (len < 43)
- goto umts_end;
+ /*
+* Response parameters/data, case 1, 3G security context,
+* command successful:
+*
+* "Successful 3G authentication" tag = 'DB'
+* 'DB' + L3 + RES(L3) + L4 + CK(L4) + L5 + IK(L5) + 8 + Kc(8)
+*/
+ l = *ptr++; /* L3 */
+ if ((ptr + l) > end)
+ return FALSE;
- /* success */
- if (buffer[1] != 0x08)
- goto umts_end;
+ res->data = ptr;
+ res->len = l;
+ ptr += l;
- *res = buffer + 2;
+ if (ptr == end)
+ return FALSE;
- if (buffer[10] != 0x10)
- goto umts_end;
+ l = *ptr++; /* L4 */
+ if ((ptr + l) > end)
+ return FALSE;
- *ck = buffer + 11;
+ ck->data = ptr;
+ ck->len = l;
+ ptr += l;
- if (buffer[27] != 0x10)
- goto umts_end;
+ if (ptr == end)
+ return FALSE;
- *ik = buffer + 28;
+ l = *ptr++; /* L5 */
+
