While reading src/l1/a5.c I noticed that gmr1_a5(n, ...) silently falls through its switch default for n >= 2, leaving the caller's dl/ul buffers untouched. The only in-tree caller, at src/gmr1_rx.c:518 (TCH3 decode), passes n as a data-driven value (cd->tch3_state.ciph) into an uninitialized stack buffer ciph[208], which is then fed to gmr1_tch3_decode() as a keystream. For any n outside {0, 1} this means uninitialized stack memory becomes the "keystream" that the decoder XORs against the burst.
Patch 1/2 is the defensive fix: zero the output buffers in the default case, matching the n=0 "no ciphering" path so the function's postcondition (buffers are fully written whenever non-NULL) holds for every legal value of n. No algorithmic change to gmr1_a5_1 or the n=0 / n=1 paths.
Patch 2/2 is cosmetic: it clarifies the Doxygen block on gmr1_a5() to document the accepted range of n and the behavior per case, and it adds a \see reference on gmr1_a5_1() to the public algorithm description by Driessen, Hund, Willems, Paar, Holz ("Don't Trust Satellite Phones", IEEE S&P 2012) which underpins the existing implementation credit.
Both patches apply cleanly on top of master (2b98363, "avoid memory leaks in gsmtap error paths").
Dominik Bay (2): l1/a5: zero output buffers for unsupported A5 variants l1/a5: expand dispatcher doc and cite upstream reference
src/l1/a5.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
gmr1_a5(n, ...) silently fell through the default case for n >= 2, leaving caller-supplied dl/ul buffers uninitialized. The only in-tree caller in src/gmr1_rx.c passes n data-driven (cd->tch3_state.ciph) into a stack buffer ciph[208] at line 500, so a value other than 0 or 1 exposes uninitialized stack memory to gmr1_tch3_decode() as a supposed keystream.
Zero the output buffers in the default case, matching the n=0 'no ciphering' path, so the function's postcondition (buffers are fully written whenever non-NULL) holds for every legal value of n.
No algorithmic change to gmr1_a5_1 or the n=0 / n=1 paths.
Signed-off-by: Dominik Bay eimann@etherkiller.de --- src/l1/a5.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/l1/a5.c b/src/l1/a5.c index ad6ca8f..105dcb2 100644 --- a/src/l1/a5.c +++ b/src/l1/a5.c @@ -71,7 +71,12 @@ gmr1_a5(int n, uint8_t *key, uint32_t fn, int nbits, break;
default: - /* a5/[2...7] not supported/existent */ + /* a5/[2...7] reserved; zero the buffers so callers never + * observe uninitialized memory as keystream. */ + if (dl) + memset(dl, 0x00, nbits); + if (ul) + memset(ul, 0x00, nbits); break; }; }
Clarify the Doxygen block on gmr1_a5(): - Document the accepted range of n as the GMR-1 A5/x selector, with only n=0 (no ciphering) and n=1 (A5-GMR-1) defined by the standard. - Spell out the behavior for each case, including the defensive zeroing now done for reserved values.
Add a \see reference on gmr1_a5_1() to the public algorithm description (Driessen, Hund, Willems, Paar, Holz, IEEE S&P 2012).
Purely cosmetic, no behavioral change.
Signed-off-by: Dominik Bay eimann@etherkiller.de --- src/l1/a5.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/l1/a5.c b/src/l1/a5.c index 105dcb2..24a4299 100644 --- a/src/l1/a5.c +++ b/src/l1/a5.c @@ -43,14 +43,17 @@
/*! \brief Main method to generate a A5/x cipher stream - * \param[in] n Which A5/x method to use + * \param[in] n Selects the A5/x variant (GMR-1 only defines n=0 and n=1) * \param[in] key 8 byte array for the key (as received from the SIM) * \param[in] fn Frame number * \param[in] nbits How many bits to generate * \param[out] dl Pointer to array of ubits to return Downlink cipher stream * \param[out] ul Pointer to array of ubits to return Uplink cipher stream * - * Currently only A5/0 and A5/1. + * For n=0 no ciphering is applied and the output buffers are zeroed. + * For n=1 the GMR-1 A5/1 keystream is generated by gmr1_a5_1(). + * Other values of n are reserved and also result in zeroed output buffers. + * * Either (or both) of dl/ul can be NULL if not needed. */ void @@ -226,6 +229,10 @@ _a5_1_output(uint32_t *r) * \param[out] ul Pointer to array of ubits to return Uplink cipher stream * * Either (or both) of dl/ul can be NULL if not needed. + * + * \see B. Driessen, R. Hund, C. Willems, C. Paar, T. Holz, + * "Don't Trust Satellite Phones: A Security Analysis of Two + * Satphone Standards", IEEE Symposium on Security and Privacy, 2012. */ void gmr1_a5_1(uint8_t *key, uint32_t fn, int nbits, ubit_t *dl, ubit_t *ul)
-
Dominik Bay