[PATCH] Add A5/3 and 4 support

Signed-off-by: Max Suraev <Max.Suraev@fairwaves.co>
---
 include/osmocom/gsm/a5.h | 10 ++++---
 src/gsm/a5.c             | 69 ++++++++++++++++++++++++++++++++++++++++++++++--
 tests/Makefile.am        |  1 +
 tests/a5/a5_test.c       | 67 ++++++++++++++++++++++++++++++++++++----------
 tests/a5/a5_test.ok      | 24 +++++++++++++++++
 5 files changed, 152 insertions(+), 19 deletions(-)

diff --git a/include/osmocom/gsm/a5.h b/include/osmocom/gsm/a5.h
index c076734..e3e73f9 100644
--- a/include/osmocom/gsm/a5.h
+++ b/include/osmocom/gsm/a5.h
@@ -23,6 +23,7 @@
 #pragma once

 #include <stdint.h>
+#include <stdbool.h>

 #include <osmocom/core/bits.h>

@@ -48,13 +49,16 @@ osmo_a5_fn_count(uint32_t fn)
 }

 	/* Notes:
-	 *  - key must be 8 bytes long (or NULL for A5/0)
+	 *  - key must be 8/16 bytes long (or NULL for A5/0) depending on algorithm variant
 	 *  - the dl and ul pointer must be either NULL or 114 bits long
-	 *  - fn is the _real_ GSM frame number.
-	 *    (converted internally to fn_count)

Re: [PATCH] Add A5/3 and 4 support

Hi,

>         /* Notes:
> -        *  - key must be 8 bytes long (or NULL for A5/0)
> +        *  - key must be 8/16 bytes long (or NULL for A5/0) depending on algorithm variant
>          *  - the dl and ul pointer must be either NULL or 114 bits long
> -        *  - fn is the _real_ GSM frame number.
> -        *    (converted internally to fn_count)
> +        *  - fn is the _real_ GSM frame number (unless fn_correct is false),
> +        *    converted internally to fn_count
> +        *  - only top-level osmo_a5 should be considered as part of public API
>          */
>  int osmo_a5(int n, const uint8_t *key, uint32_t fn, ubit_t *dl, ubit_t *ul);
>  void osmo_a5_1(const uint8_t *key, uint32_t fn, ubit_t *dl, ubit_t *ul);
>  void osmo_a5_2(const uint8_t *key, uint32_t fn, ubit_t *dl, ubit_t *ul);
> +void _osmo_a5_3(const uint8_t *key, uint32_t fn, ubit_t *dl, ubit_t *ul, bool fn_correct);
> +void _osmo_a5_4(const uint8_t *key, uint32_t fn, ubit_t *dl, ubit_t *ul, bool fn_correct);

Why the _ prefix ? It's exported and in the header and none of the
other have it. Unnecessary inconsistency.

Same for fn_correct, none of the other have it. The only use for it
seem to be for the test ... but then the correct way is to make the
test compute the real fn from the fn_count before the call, not
pollute the API with a useless parameter.

>  a5_a5_test_SOURCES = a5/a5_test.c
>  a5_a5_test_LDADD = $(top_builddir)/src/libosmocore.la $(top_builddir)/src/gsm/libosmogsm.la
> +a5_a5_test_LDFLAGS = -static

Re: [PATCH] Add A5/3 and 4 support

16.06.2014 23:24, Sylvain Munaut пишет:

> Why the _ prefix ? It's exported and in the header and none of the
> other have it. Unnecessary inconsistency.
> 

Agree but I would rather hide implementation details of all osmo_a5_* from public API
completely into separate non-installable header. Generic osmo_a5 should be fine for
all use cases. Are there any reasons we expose those?

--

-- 
best regards,
Max, http://fairwaves.co

[PATCH] Add A5/3-4 cipher support

Signed-off-by: Max Suraev <Max.Suraev@fairwaves.co>
---
 include/Makefile.am       |  3 +-
 include/osmocom/gsm/a34.h | 45 ++++++++++++++++++++++++++++++
 src/gsm/a5.c              | 70 +++++++++++++++++++++++++++++++++++++++++++++--
 tests/Makefile.am         |  1 +
 tests/a5/a5_test.c        | 68 +++++++++++++++++++++++++++++++++++----------
 tests/a5/a5_test.ok       | 24 ++++++++++++++++
 6 files changed, 194 insertions(+), 17 deletions(-)
 create mode 100644 include/osmocom/gsm/a34.h

diff --git a/include/Makefile.am b/include/Makefile.am
index 74396de..f7d77c0 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -110,7 +110,8 @@ endif

 noinst_HEADERS = \
 	osmocom/core/timer_compat.h \
-	osmocom/gsm/kasumi.h
+	osmocom/gsm/kasumi.h \
+	osmocom/gsm/a34.h

 osmocom/core/bit%gen.h: osmocom/core/bitXXgen.h.tpl
 	$(AM_V_GEN)$(MKDIR_P) $(dir $@)
diff --git a/include/osmocom/gsm/a34.h b/include/osmocom/gsm/a34.h
new file mode 100644
index 0000000..39e32db
--- /dev/null
+++ b/include/osmocom/gsm/a34.h

Re: [PATCH] Add A5/3-4 cipher support

This version avoids polluting external API with implementation details by introducing
separate noinst header.

--

-- 
best regards,
Max, http://fairwaves.co

Re: [PATCH] Add A5/3-4 cipher support

As I stated in the previous comment, I don't even see why you need
that header. Just make your test go through the public API and test
the whole chain. It gets rid of the header, the need to static link,
that extraneous param and only add one new inline function in the test
...

On Tue, Jun 17, 2014 at 5:04 PM, ☎ <Max.Suraev@fairwaves.co> wrote:
> This version avoids polluting external API with implementation details by introducing
> separate noinst header.
>
> --
> best regards,
> Max, http://fairwaves.co
>

Re: [PATCH] Add A5/3-4 cipher support

17.06.2014 17:21, Sylvain Munaut пишет:
> As I stated in the previous comment, I don't even see why you need
> that header. Just make your test go through the public API and test
> the whole chain. It gets rid of the header, the need to static link,
> that extraneous param and only add one new inline function in the test

Well, that's a function which will only be used once - and it got to be written,
debugged and tested. And all that for the sake of? I do not consider extra non-public
header to be of such great burden that it justifies extra work necessary to get rid
of it.

--

-- 
best regards,
Max, http://fairwaves.co

Re: [PATCH] Add A5/3-4 cipher support

On Tue, Jun 17, 2014 at 5:30 PM, ☎ <Max.Suraev@fairwaves.co> wrote:
> 17.06.2014 17:21, Sylvain Munaut пишет:
>> As I stated in the previous comment, I don't even see why you need
>> that header. Just make your test go through the public API and test
>> the whole chain. It gets rid of the header, the need to static link,
>> that extraneous param and only add one new inline function in the test
>
> Well, that's a function which will only be used once - and it got to be written,
> debugged and tested.

static inline uint32_t
osmo_a5_fn(uint32_t fn_count)
{
        int t1 = fn_count >> 11;
        int t2 = fn_count & 0x1f;
        int t3 = (fn_count >> 5) & 0x3f;
        return (t1 * 26 * 51) + ((t3 - t2 + 26) % 26) * 51 + t3;
}

There, it's written. No need to debug it, it's correct. No need to
test it, it's going to be in the test path of the A5/[3,4] test and
tested automatically as part of them.

> And all that for the sake of? I do not consider extra non-public
> header to be of such great burden that it justifies extra work necessary to get rid
> of it.

Writing it in the first place was more typing that this function.

Also there is an issue in it anyway, you used the Doxygen defgroup

Re: [PATCH] Add A5/3-4 cipher support

Also forgot to say that the 'Notes:' in the public header should be
updated to reflect the key length. So should be the osmo_a5 doxygen
doc for the 'key' parameter.

Cheers,

    Sylvain

Re: [PATCH] Add A5/3-4 cipher support

17.06.2014 20:05, Sylvain Munaut пишет:

> static inline uint32_t
> osmo_a5_fn(uint32_t fn_count)
> {
>         int t1 = fn_count >> 11;
>         int t2 = fn_count & 0x1f;
>         int t3 = (fn_count >> 5) & 0x3f;
>         return (t1 * 26 * 51) + ((t3 - t2 + 26) % 26) * 51 + t3;
> }
> 
> There, it's written. No need to debug it, it's correct. No need to
> test it, it's going to be in the test path of the A5/[3,4] test and
> tested automatically as part of them.
> 

Surprisingly those tests fail for some of the test vectors for me.
How exactly you've applied and tested this function?
Could you share entire a5_test.c?

--

-- 
best regards,
Max, http://fairwaves.co

Re: [PATCH] Add A5/3-4 cipher support

#include <stdio.h>
#include <stdint.h>

static inline uint32_t
osmo_a5_fn_count(uint32_t fn)
{
        int t1 = fn / (26 * 51);
        int t2 = fn % 26;
        int t3 = fn % 51;
        return (t1 << 11) | (t3 << 5) | t2;
}

static inline uint32_t
osmo_a5_fn(uint32_t fn_count)
{
        int t1 = fn_count >> 11;
        int t2 = fn_count & 0x1f;
        int t3 = (fn_count >> 5) & 0x3f;
        return (t1 * 26 * 51) + ((t3 - t2 + 26) % 26) * 51 + t3;
}

#define FN_MAX 26*51*2048

int main(int argc, char *argv[])
{
        int i;
        for (i=0; i<FN_MAX; i++)
                if (i != osmo_a5_fn(osmo_a5_fn_count(i)))
                        printf("%d\n", i);

Re: [PATCH] Add A5/3-4 cipher support

Bunch of fucking idiots who chose as test data completely invalid FN
count values that can't possibly happen in a GSM system ...

- Either just use different test vector you generate
- Or use a marker bit like (1<<31) to indicate to _a5_{3,4} not to
convert the values for testing

On Tue, Jun 17, 2014 at 9:35 PM, Sylvain Munaut <246tnt@gmail.com> wrote:
> #include <stdio.h>
> #include <stdint.h>
>
>
> static inline uint32_t
> osmo_a5_fn_count(uint32_t fn)
> {
>         int t1 = fn / (26 * 51);
>         int t2 = fn % 26;
>         int t3 = fn % 51;
>         return (t1 << 11) | (t3 << 5) | t2;
> }
>
> static inline uint32_t
> osmo_a5_fn(uint32_t fn_count)
> {
>         int t1 = fn_count >> 11;
>         int t2 = fn_count & 0x1f;
>         int t3 = (fn_count >> 5) & 0x3f;
>         return (t1 * 26 * 51) + ((t3 - t2 + 26) % 26) * 51 + t3;
> }
>

Re: [PATCH] Add A5/3-4 cipher support

17.06.2014 21:46, Sylvain Munaut пишет:
> Bunch of fucking idiots who chose as test data completely invalid FN
> count values that can't possibly happen in a GSM system ...

I presume the idea was not to test gsm use cases but to cover all the internal states
of the cipher.

> 
> - Either just use different test vector you generate

I would rather stick to official test vectors published in standard.

> - Or use a marker bit like (1<<31) to indicate to _a5_{3,4} not to
> convert the values for testing
> 

I really like my initial proposal:
- it works already
- it's simpler

Could you emphasize - why exactly introducing private header is such a disaster that
we have to waste time and efforts trying to not let it happen?

--

-- 
best regards,
Max, http://fairwaves.co

Re: [PATCH] Add A5/3-4 cipher support

> Could you emphasize - why exactly introducing private header is such a disaster that
> we have to waste time and efforts trying to not let it happen?

- It's a useless file that's used at exactly 1 place and not even for
code used in installed libs but just for the test. At that point you
might just as well just pre-declare them in the test source file
directly.
- It prevents making those function static
- It requires static linking for the test
- For the A5/[3/4] you bypass the osmo_a5 wrapper and thus don't cover
a breakage that would happen there.

The alternative solves all of the above and requires exactly 1 lines
changes in the original _a5_4 function :

uint32_t fn_count = (fn_correct) ? osmo_a5_fn_count(fn) : fn;

becomes

uint32_t fn_count = (fn & (1<<31)) ? (fn & ~(1<<31)) : osmo_a5_fn_count(fn);

btw, in your test I also see

+       osmo_a5(4, key, osmo_a5_fn(count), dlout, NULL);
+       osmo_a5(4, key, osmo_a5_fn(count), NULL, ulout);

Why separate ? The case where both dlout and ulout are requested at
the same time should work as well.

Cheers,

Re: [PATCH] Add A5/3-4 cipher support

18.06.2014 00:15, Sylvain Munaut пишет:
>> Could you emphasize - why exactly introducing private header is such a disaster that
>> we have to waste time and efforts trying to not let it happen?
> 
> - It's a useless file that's used at exactly 1 place and not even for
> code used in installed libs but just for the test. At that point you
> might just as well just pre-declare them in the test source file
> directly.
> - It prevents making those function static
> - It requires static linking for the test
> - For the A5/[3/4] you bypass the osmo_a5 wrapper and thus don't cover
> a breakage that would happen there.
> 
> 
> The alternative solves all of the above and requires exactly 1 lines
> changes in the original _a5_4 function :
> 
> uint32_t fn_count = (fn_correct) ? osmo_a5_fn_count(fn) : fn;
> 
> becomes
> 
> uint32_t fn_count = (fn & (1<<31)) ? (fn & ~(1<<31)) : osmo_a5_fn_count(fn);
> 
> 

I probably apply it wrongly - test still fails for me.
Feel free to commit your variant though - I think it's less readable but as long as
it works and covered by test suit it's fine.

--

-- 

Re: [PATCH] Add A5/3-4 cipher support

Seems nice but it case of a5 test vectors it still fails.
Could you have a look into attached patch?

--

-- 
best regards,
Max, http://fairwaves.co

[PATCH] Add A5/3-4 cipher support

Signed-off-by: Max <max.suraev@fairwaves.co>
---
 include/osmocom/gsm/a5.h |  2 +-
 src/gsm/a5.c             | 72 ++++++++++++++++++++++++++++++++++++++++++++++--
 tests/Makefile.am        |  3 +-
 tests/a5/a5_test.c       | 67 ++++++++++++++++++++++++++++++++++----------
 tests/a5/a5_test.ok      | 24 ++++++++++++++++
 5 files changed, 149 insertions(+), 19 deletions(-)

diff --git a/include/osmocom/gsm/a5.h b/include/osmocom/gsm/a5.h
index c076734..d22cdbb 100644
--- a/include/osmocom/gsm/a5.h
+++ b/include/osmocom/gsm/a5.h
@@ -48,7 +48,7 @@ osmo_a5_fn_count(uint32_t fn)
 }

 	/* Notes:
-	 *  - key must be 8 bytes long (or NULL for A5/0)
+	 *  - key must be 8 or 16 (for a5/4) bytes long (or NULL for A5/0)
 	 *  - the dl and ul pointer must be either NULL or 114 bits long
 	 *  - fn is the _real_ GSM frame number.
 	 *    (converted internally to fn_count)
diff --git a/src/gsm/a5.c b/src/gsm/a5.c
index de821e8..a26253e 100644
--- a/src/gsm/a5.c
+++ b/src/gsm/a5.c
@@ -36,18 +36,76 @@

 #include <errno.h>
 #include <string.h>

Re: [PATCH] Add A5/3-4 cipher support

This version does not introduce any new headers but still got the same standard test
suite.

Please review.

--

-- 
best regards,
Max, http://fairwaves.co