lynxis lazus has submitted this change. ( https://gerrit.osmocom.org/c/simtrace2/+/43185?usp=email )
Change subject: Add osmo_apdu_segment_in2() to ensure correct parsing of GP GET DATA ......................................................................
Add osmo_apdu_segment_in2() to ensure correct parsing of GP GET DATA
osmo_apdu_segment_in() doesn't have the context of the previous APDU, for nearly all calls this is fine, except for GP GET DATA, which can be a case 2 or case 4 APDU. GP GET DATA defines the Le field as 0x00 which is used by osmo_apdu_segment_in to detect if it is a case 2 or case 4. However a card can respond with the SW 6cXX, which means the previous command must be resend with a Le field XX, which got misinterpreted by the osmo_apdu_segment_in() as a case 4 resulting in a timeout waiting for more data.
Modem <-> Card 81cadf2000 -> <- 6c0b 81cadf200b -> -- Simtrace waits for more data because it wrongly decodes it as APDU case 4 instead of case 2.
Related: SYS#8147 Change-Id: Ie238662e7e6a10dd4283ede0c8d8d73bc375c247 --- M TODO-RELEASE M host/Makefile.am M host/configure.ac M host/include/osmocom/simtrace2/apdu_dispatch.h M host/lib/apdu_dispatch.c A host/tests/Makefile.am A host/tests/apdu_dispatch/Makefile.am A host/tests/apdu_dispatch/apdu_dispatch_test.c A host/tests/apdu_dispatch/apdu_dispatch_test.ok A host/tests/testsuite.at 10 files changed, 197 insertions(+), 1 deletion(-)
Approvals: dexter: Looks good to me, approved; Verified Jenkins Builder: Verified
diff --git a/TODO-RELEASE b/TODO-RELEASE index 0ed7189..5922210 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -7,3 +7,4 @@ # If any interfaces have been added since the last public release: c:r:a + 1. # If any interfaces have been removed or changed since the last public release: c:r:0. #library what description / commit summary line +libosmo-simtrace2 added osmo_apdu_segment_in2() diff --git a/host/Makefile.am b/host/Makefile.am index 338a46b..465a4e0 100644 --- a/host/Makefile.am +++ b/host/Makefile.am @@ -1,7 +1,7 @@ AUTOMAKE_OPTIONS = foreign dist-bzip2 1.6
AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include -SUBDIRS = include lib src contrib #tests examples doc +SUBDIRS = include lib src contrib tests # examples doc
EXTRA_DIST = .version
diff --git a/host/configure.ac b/host/configure.ac index 83a2b23..84b4147 100644 --- a/host/configure.ac +++ b/host/configure.ac @@ -100,4 +100,6 @@ src/Makefile lib/Makefile contrib/Makefile + tests/Makefile + tests/apdu_dispatch/Makefile Makefile) diff --git a/host/include/osmocom/simtrace2/apdu_dispatch.h b/host/include/osmocom/simtrace2/apdu_dispatch.h index 62ff762..f3c0ada 100644 --- a/host/include/osmocom/simtrace2/apdu_dispatch.h +++ b/host/include/osmocom/simtrace2/apdu_dispatch.h @@ -45,3 +45,6 @@
int osmo_apdu_segment_in(struct osmo_apdu_context *ac, const uint8_t *apdu_buf, unsigned int apdu_len, bool new_apdu); + +int osmo_apdu_segment_in2(struct osmo_apdu_context *ac, struct osmo_apdu_context *prev_ac, + const uint8_t *apdu_buf, unsigned int apdu_len, bool new_apdu); diff --git a/host/lib/apdu_dispatch.c b/host/lib/apdu_dispatch.c index f2fa64e..8233507 100644 --- a/host/lib/apdu_dispatch.c +++ b/host/lib/apdu_dispatch.c @@ -78,14 +78,52 @@ int osmo_apdu_segment_in(struct osmo_apdu_context *ac, const uint8_t *apdu_buf, unsigned int apdu_len, bool new_apdu) { + return osmo_apdu_segment_in2(ac, NULL, apdu_buf, apdu_len, new_apdu); +} + +/*! \brief input function for APDU segmentation + * \param ac APDU context across successive calls + * \param prev_ac Previous APDU context across successive calls + * \param[in] apdu_buf APDU input data buffer + * \param[in] apdu_len Length of apdu_buf + * \param[in] new_apdu Is this the beginning of a new APDU? + * + * The function returns APDU_ACT_TX_CAPDU_TO_CARD once there is + * sufficient data of the APDU received to transmit the command-APDU to + * the actual card. + * + * The function returns APDU_ACT_RX_MORE_CAPDU_FROM_READER when there + * is more data to be received from the card reader (GSM Phone). + */ +int osmo_apdu_segment_in2(struct osmo_apdu_context *ac, struct osmo_apdu_context *prev_ac, + const uint8_t *apdu_buf, unsigned int apdu_len, + bool new_apdu) +{ int rc = 0;
if (new_apdu) { + if (prev_ac) + memcpy(prev_ac, ac, sizeof(*ac)); /* initialize the apdu context structure */ memset(ac, 0, sizeof(*ac)); /* copy APDU header over */ memcpy(&ac->hdr, apdu_buf, sizeof(ac->hdr)); ac->apdu_case = osim_determine_apdu_case(&osim_uicc_sim_cic_profile, apdu_buf); + + /* Corner case when card returns SW 6CXX (resend previous command with Le = XX). + * Especial GP APDU defines Le to be 0x00, except if card returned SW 6cXX, + * Le must be set to XX. osim_determine_apdu_case() might return the wrong + * APDU case because osim_determine_apdu_case() uses the Le field to determine if the APDU + */ + if (prev_ac && prev_ac->sw[0] == 0x6c && /* check if Le was invalid and must be set to sw[1] */ + (prev_ac->apdu_case >= 1 && prev_ac->apdu_case < 4) && /* handling only valid for apdu case 1..3 */ + ac->hdr.cla == prev_ac->hdr.cla && /* check if CLA, INS, P1, P2 are equal */ + ac->hdr.ins == prev_ac->hdr.ins && + ac->hdr.p1 == prev_ac->hdr.p1 && + ac->hdr.p2 == prev_ac->hdr.p2) { + ac->apdu_case = prev_ac->apdu_case; + } + switch (ac->apdu_case) { case 1: /* P3 == 0, No Lc/Le */ ac->le.tot = ac->lc.tot = 0; diff --git a/host/tests/Makefile.am b/host/tests/Makefile.am new file mode 100644 index 0000000..e58a286 --- /dev/null +++ b/host/tests/Makefile.am @@ -0,0 +1,40 @@ +SUBDIRS = apdu_dispatch + +# The `:;' works around a Bash 3.2 bug when the output is not writeable. +$(srcdir)/package.m4: $(top_srcdir)/configure.ac + :;{ \ + echo '# Signature of the current package.' && \ + echo 'm4_define([AT_PACKAGE_NAME],' && \ + echo ' [$(PACKAGE_NAME)])' && \ + echo 'm4_define([AT_PACKAGE_TARNAME],' && \ + echo ' [$(PACKAGE_TARNAME)])' && \ + echo 'm4_define([AT_PACKAGE_VERSION],' && \ + echo ' [$(PACKAGE_VERSION)])' && \ + echo 'm4_define([AT_PACKAGE_STRING],' && \ + echo ' [$(PACKAGE_STRING)])' && \ + echo 'm4_define([AT_PACKAGE_BUGREPORT],' && \ + echo ' [$(PACKAGE_BUGREPORT)])'; \ + echo 'm4_define([AT_PACKAGE_URL],' && \ + echo ' [$(PACKAGE_URL)])'; \ + } >'$(srcdir)/package.m4' + +EXTRA_DIST = testsuite.at $(srcdir)/package.m4 $(TESTSUITE) +TESTSUITE = $(srcdir)/testsuite +DISTCLEANFILES = atconfig + +check-local: atconfig $(TESTSUITE) + $(SHELL) '$(TESTSUITE)' $(TESTSUITEFLAGS) + +installcheck-local: atconfig $(TESTSUITE) + $(SHELL) '$(TESTSUITE)' AUTOTEST_PATH='$(bindir)' \ + $(TESTSUITEFLAGS) + +clean-local: + test ! -f '$(TESTSUITE)' || \ + $(SHELL) '$(TESTSUITE)' --clean + +AUTOM4TE = $(SHELL) $(top_srcdir)/missing --run autom4te +AUTOTEST = $(AUTOM4TE) --language=autotest +$(TESTSUITE): $(srcdir)/testsuite.at $(srcdir)/package.m4 + $(AUTOTEST) -I '$(srcdir)' -o $@.tmp $@.at + mv $@.tmp $@ diff --git a/host/tests/apdu_dispatch/Makefile.am b/host/tests/apdu_dispatch/Makefile.am new file mode 100644 index 0000000..06a07b5 --- /dev/null +++ b/host/tests/apdu_dispatch/Makefile.am @@ -0,0 +1,13 @@ +AM_LDFLAGS = -no-install +AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include +AM_CFLAGS=-Wall $(LIBOSMONETIF_CFLAGS) $(LIBOSMOCORE_CFLAGS) $(LIBOSMOVTY_CFLAGS) +LDADD = $(top_builddir)/lib/.libs/libosmo-simtrace2.la \ + $(LIBOSMOCORE_LIBS) $(LIBOSMOGSM_LIBS) $(LIBOSMOVTY_LIBS) $(LIBOSMONETIF_LIBS) $(LIBSCTP_LIBS) + +EXTRA_DIST = \ + apdu_dispatch_test.ok \ + $(NULL) + +check_PROGRAMS = apdu_dispatch_test + +apdu_dispatch_test_SOURCES = apdu_dispatch_test.c diff --git a/host/tests/apdu_dispatch/apdu_dispatch_test.c b/host/tests/apdu_dispatch/apdu_dispatch_test.c new file mode 100644 index 0000000..280736f --- /dev/null +++ b/host/tests/apdu_dispatch/apdu_dispatch_test.c @@ -0,0 +1,83 @@ +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <stdint.h> + +#include <osmocom/simtrace2/apdu_dispatch.h> + +const uint8_t get_data_c2_ca[] = { 0x81, 0xCA, 0x00, 0x5A, 0x00 }; +const uint8_t get_data_c2_cb[] = { 0x81, 0xCB, 0x00, 0x5A, 0x00 }; + +/* teset data for 6cXX test */ +const uint8_t get_data_c2_ca_le_23[] = { 0x81, 0xCA, 0x00, 0x5A, 0x23 }; +const uint8_t get_data_c2_cb_le_23[] = { 0x81, 0xCb, 0x00, 0x5A, 0x23 }; + +#define APDU_SEGMENT_IN(apdu, exp_rc) \ + do { \ + memset(&ac, 0, sizeof(ac)); \ + printf("Testing " #apdu "\n"); \ + int rc = osmo_apdu_segment_in(&ac, apdu, ARRAY_SIZE(apdu), true); \ + if (rc != exp_rc) \ + printf("%d (actual) != %d (expected)\n", rc, exp_rc);\ + OSMO_ASSERT(rc == exp_rc); \ + } while (0) + +#define APDU_SEGMENT_IN2(apdu, exp_rc) \ + do { \ + memset(&ac, 0, sizeof(ac)); \ + memset(&prev_ac, 0, sizeof(prev_ac)); \ + printf("Testing " #apdu "\n"); \ + int rc = osmo_apdu_segment_in2(&ac, &prev_ac, apdu, ARRAY_SIZE(apdu), true); \ + if (rc != exp_rc) \ + printf("%d (actual) != %d (expected)\n", rc, exp_rc);\ + OSMO_ASSERT(rc == exp_rc); \ + } while (0) + +void test_apdu_dispatch_simple(void) +{ + struct osmo_apdu_context ac, prev_ac; + APDU_SEGMENT_IN(get_data_c2_ca, APDU_ACT_TX_CAPDU_TO_CARD); + APDU_SEGMENT_IN(get_data_c2_cb, APDU_ACT_TX_CAPDU_TO_CARD); + + APDU_SEGMENT_IN2(get_data_c2_ca, APDU_ACT_TX_CAPDU_TO_CARD); + APDU_SEGMENT_IN2(get_data_c2_cb, APDU_ACT_TX_CAPDU_TO_CARD); +} + +void test_apdu_dispatch_context(void) +{ + struct osmo_apdu_context ac, prev_ac; + int rc; + + printf("Testing GET DATA / 0xCA with SW 6Cxx\n"); + memset(&ac, 0, sizeof(ac)); + memset(&prev_ac, 0, sizeof(prev_ac)); + rc = osmo_apdu_segment_in2(&ac, &prev_ac, get_data_c2_ca, ARRAY_SIZE(get_data_c2_ca), 1); + OSMO_ASSERT(rc == APDU_ACT_TX_CAPDU_TO_CARD); + OSMO_ASSERT(ac.apdu_case == 2) + ac.sw[0] = 0x6c; + ac.sw[1] = 0x23; + rc = osmo_apdu_segment_in2(&ac, &prev_ac, get_data_c2_ca_le_23, ARRAY_SIZE(get_data_c2_ca_le_23), 1); + OSMO_ASSERT(rc == APDU_ACT_TX_CAPDU_TO_CARD); + OSMO_ASSERT(ac.apdu_case == 2) + + printf("Testing GET DATA / 0xCB with SW 6Cxx\n"); + memset(&ac, 0, sizeof(ac)); + memset(&prev_ac, 0, sizeof(prev_ac)); + rc = osmo_apdu_segment_in2(&ac, &prev_ac, get_data_c2_cb, ARRAY_SIZE(get_data_c2_cb), 1); + OSMO_ASSERT(rc == APDU_ACT_TX_CAPDU_TO_CARD); + OSMO_ASSERT(ac.apdu_case == 2) + ac.sw[0] = 0x6c; + ac.sw[1] = 0x23; + rc = osmo_apdu_segment_in2(&ac, &prev_ac, get_data_c2_cb_le_23, ARRAY_SIZE(get_data_c2_cb_le_23), 1); + OSMO_ASSERT(rc == APDU_ACT_TX_CAPDU_TO_CARD); + OSMO_ASSERT(ac.apdu_case == 2) +} + +int main(int argc, char **argv) +{ + test_apdu_dispatch_simple(); + test_apdu_dispatch_context(); + + printf("All tests passed.\n"); + return 0; +} diff --git a/host/tests/apdu_dispatch/apdu_dispatch_test.ok b/host/tests/apdu_dispatch/apdu_dispatch_test.ok new file mode 100644 index 0000000..108450f --- /dev/null +++ b/host/tests/apdu_dispatch/apdu_dispatch_test.ok @@ -0,0 +1,7 @@ +Testing get_data_c2_ca +Testing get_data_c2_cb +Testing get_data_c2_ca +Testing get_data_c2_cb +Testing GET DATA / 0xCA with SW 6Cxx +Testing GET DATA / 0xCB with SW 6Cxx +All tests passed. diff --git a/host/tests/testsuite.at b/host/tests/testsuite.at new file mode 100644 index 0000000..22fcdc4 --- /dev/null +++ b/host/tests/testsuite.at @@ -0,0 +1,9 @@ +AT_INIT +AT_BANNER([Regression tests.]) + +AT_SETUP([apdu_dispatch]) +AT_KEYWORDS([apdu_dispatch]) +cat $abs_srcdir/apdu_dispatch/apdu_dispatch_test.ok > expout +AT_CHECK([$abs_top_builddir/tests/apdu_dispatch/apdu_dispatch_test], [], [expout], [ignore]) +AT_CLEANUP +