lynxis lazus has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/43178?usp=email )
Change subject: sim/class_tables: add a size attribute ......................................................................
sim/class_tables: add a size attribute
osim_determine_apdu_case() doesn't know the size of the APDU, this may result in reading invalid data outside the APDU.
Warning: this is an API breakage!
Instead adding a second function osim_determine_apdu_case_size() could allow it. However because the class_tables.h also defines the internal struct with the helper functions, which in turn has the same problem.
Change-Id: Iee50063399a0c3b29594e737f44aaa125fd06a2e --- M include/osmocom/sim/class_tables.h M src/sim/class_tables.c M tests/sim/sim_test.c 3 files changed, 30 insertions(+), 16 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/78/43178/1
diff --git a/include/osmocom/sim/class_tables.h b/include/osmocom/sim/class_tables.h index ec9ec49..2c7c206 100644 --- a/include/osmocom/sim/class_tables.h +++ b/include/osmocom/sim/class_tables.h @@ -20,7 +20,7 @@ struct osim_cla_ins_case { uint8_t cla; uint8_t cla_mask; - int (*helper)(const struct osim_cla_ins_case *cic, const uint8_t *hdr); + int (*helper)(const struct osim_cla_ins_case *cic, const uint8_t *apdu, size_t apdu_size); const uint8_t *ins_tbl; };
@@ -36,4 +36,4 @@ extern const struct osim_cla_ins_card_profile osim_uicc_sim_cic_profile;
int osim_determine_apdu_case(const struct osim_cla_ins_card_profile *prof, - const uint8_t *hdr); + const uint8_t *apdu, size_t apdu_size); diff --git a/src/sim/class_tables.c b/src/sim/class_tables.c index 7500fc5..0a792c6 100644 --- a/src/sim/class_tables.c +++ b/src/sim/class_tables.c @@ -16,6 +16,7 @@ */
#include <stdint.h> +#include <errno.h> #include <osmocom/core/utils.h> #include <osmocom/sim/class_tables.h>
@@ -139,13 +140,16 @@ };
static int uicc046_cla_ins_helper(const struct osim_cla_ins_case *cic, - const uint8_t *hdr) + const uint8_t *hdr, size_t hdr_size) { - uint8_t ins = hdr[1]; - uint8_t p1 = hdr[2]; - uint8_t p2 = hdr[3]; - uint8_t p2_cmd; + uint8_t ins, p1, p2, p2_cmd;
+ if (hdr_size < 4) + return -EINVAL; + + ins = hdr[1]; + p1 = hdr[2]; + p2 = hdr[3]; switch (ins) { case 0x73: /* MANAGE SECURE CHANNEL */ if (p1 == 0x00) /* Retrieve UICC Endpoints */ @@ -182,12 +186,16 @@ }
static int gp_cla_ins_helper(const struct osim_cla_ins_case *cic, - const uint8_t *hdr) + const uint8_t *hdr, size_t hdr_size) { - uint8_t ins = hdr[1]; - uint8_t p1 = hdr[2]; - uint8_t p3 = hdr[4]; + uint8_t ins, p1, p3;
+ if (hdr_size < 5) + return -EINVAL; + + ins = hdr[1]; + p1 = hdr[2]; + p3 = hdr[4]; switch (ins) { case 0xE2: /* STORE DATA */ switch (p1 & 0x01) { @@ -394,13 +402,17 @@ };
int osim_determine_apdu_case(const struct osim_cla_ins_card_profile *prof, - const uint8_t *hdr) + const uint8_t *apdu, size_t apdu_len) { - uint8_t cla = hdr[0]; - uint8_t ins = hdr[1]; + uint8_t cla, ins; int i; int rc;
+ if (apdu_len < 4) + return -EINVAL; + + cla = apdu[0]; + ins = apdu[1]; for (i = 0; i < prof->cic_arr_size; i++) { const struct osim_cla_ins_case *cic = &prof->cic_arr[i]; if ((cla & cic->cla_mask) != cic->cla) @@ -408,7 +420,8 @@ rc = cic->ins_tbl[ins]; switch (rc) { case 0x80: - return cic->helper(cic, hdr); + /* the length of the hdr is unknown */ + return cic->helper(cic, apdu, apdu_len); case 0x00: /* continue with further cic, rather than abort * now */ diff --git a/tests/sim/sim_test.c b/tests/sim/sim_test.c index ab5d2be..801c559 100644 --- a/tests/sim/sim_test.c +++ b/tests/sim/sim_test.c @@ -19,6 +19,7 @@ #include <string.h>
#include <osmocom/sim/sim.h> +#include <osmocom/core/utils.h> #include <osmocom/sim/class_tables.h>
const uint8_t sim_sel_mf[] = { 0xA0, 0xA4, 0x00, 0x00, 0x02, 0x3F, 0x00 }; @@ -35,7 +36,7 @@ #define APDU_CASE_ASSERT(x, y) \ do { \ printf("Testing " #x "\n"); \ - int rc = osim_determine_apdu_case(&osim_uicc_sim_cic_profile, x); \ + int rc = osim_determine_apdu_case(&osim_uicc_sim_cic_profile, x, ARRAY_SIZE(x)); \ if (rc != y) \ printf("%d (actual) != %d (intended)\n", rc, y); \ OSMO_ASSERT(rc == y); \
-
lynxis lazus