LTC_ECCSIG_RFC5656 SSH+ECDSA signature format #438

2018-10-02T16:22:21-05:00

rmw42 commented

2018-10-02 16:22:21 -05:00

(Migrated from github.com)

Implementation of SSH+ECDSA signature format for ECC signatures as defined in RFC5656.

Includes a SSH data model encode/decode (gated on #define LTC_SSH), and OID string<->Curve comparison helper function (also used for secp256k1 checks for LTC_ECCSIG_ETH27)

Checklist

documentation is added or updated
tests are added or updated

Implementation of SSH+ECDSA signature format for ECC signatures as defined in RFC5656. Includes a SSH data model encode/decode (gated on #define LTC_SSH), and OID string<->Curve comparison helper function (also used for secp256k1 checks for LTC_ECCSIG_ETH27) ### Checklist * [x] documentation is added or updated * [x] tests are added or updated

rmw42 commented

2018-10-03 02:25:45 -05:00

(Migrated from github.com)

@karel-m As promised :)

karel-m (Migrated from github.com) requested changes

2018-10-03 05:17:47 -05:00

src/headers/tomcrypt_pk.h

Please move this part to tomcrypt_private.h

Please move this part to `tomcrypt_private.h`

src/misc/ssh/ssh_test.c

Please move src/misc/ssh/ssh_test.c to tests/ssh_test.c (+ do not forget to run ./updatemakes.sh)

Please move `src/misc/ssh/ssh_test.c` to `tests/ssh_test.c` (+ do not forget to run `./updatemakes.sh`)

Once we move this stuff to tests/ssh_test.c there is no need for #ifndef LTC_TEST

Once we move this stuff to `tests/ssh_test.c` there is no need for `#ifndef LTC_TEST`

src/pk/ecc/ecc_cmp_oid_str.c

Isn't it too small? I am for oid[64].

Isn't it too small? I am for `oid[64]`.

I prefer to use explicit: XSTRCMP(x, y) != 0 or XSTRCMP(x, y) == 0 (applies also to other places)

I prefer to use explicit: `XSTRCMP(x, y) != 0` or `XSTRCMP(x, y) == 0` (applies also to other places)

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c

					
				@ -0,0 +1,70 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Maybe rather buflen <= 20 - according to man 3 strcpy: The strcpy() function copies the string pointed to by src, including the terminating null byte.

Maybe rather `buflen <= 20` - according to `man 3 strcpy`: The `strcpy()` function copies the string pointed to by src, **including the terminating null byte**.

buflen <= 20

`buflen <= 20`

buflen <= 20

`buflen <= 20`

I thing it should be nistp521

I thing it should be `nistp521`

Perhaps if (buflen <= strlen(oidstr)+12) or if (buflen <= oidlen + 12) - sprintf AFAIK writes terminating nul byte

Perhaps `if (buflen <= strlen(oidstr)+12)` or `if (buflen <= oidlen + 12)` - sprintf AFAIK writes terminating nul byte

Isn't it too small? I am for oidstr[64].

To keep the style used on other places in libtomcrypt it should be unsigned long *buflen (but as it is a private function maybe we can leave it as it is, @sjaeckel ?)

To keep the style used on other places in libtomcrypt it should be `unsigned long *buflen` (but as it is a private function maybe we can leave it as it is, @sjaeckel ?)

karel-m commented

2018-10-03 05:18:11 -05:00

(Migrated from github.com)

Great job!

karel-m commented

2018-10-03 05:26:40 -05:00

(Migrated from github.com)

And 2 warnings:

src/misc/ssh/ssh_test.c:59:5: error: no previous prototype for ‘_ssh_encoding_test’ [-Werror=missing-prototypes]
 int _ssh_encoding_test(void)
     ^~~~~~~~~~~~~~~~~~
src/misc/ssh/ssh_test.c:154:5: error: no previous prototype for ‘_ssh_decoding_test’ [-Werror=missing-prototypes]
 int _ssh_decoding_test(void)
     ^~~~~~~~~~~~~~~~~~

And 2 warnings: ``` src/misc/ssh/ssh_test.c:59:5: error: no previous prototype for ‘_ssh_encoding_test’ [-Werror=missing-prototypes] int _ssh_encoding_test(void) ^~~~~~~~~~~~~~~~~~ src/misc/ssh/ssh_test.c:154:5: error: no previous prototype for ‘_ssh_decoding_test’ [-Werror=missing-prototypes] int _ssh_decoding_test(void) ^~~~~~~~~~~~~~~~~~ ```

karel-m (Migrated from github.com) reviewed

2018-10-03 05:31:43 -05:00

src/misc/ssh/ssh_test.c

static int _ssh_encoding_test(void)

`static int _ssh_encoding_test(void)`

karel-m (Migrated from github.com) reviewed

2018-10-03 05:32:02 -05:00

src/misc/ssh/ssh_test.c

static int _ssh_decoding_test(void)

`static int _ssh_decoding_test(void)`

karel-m (Migrated from github.com) reviewed

2018-10-03 05:56:52 -05:00

src/misc/ssh/ssh_test.c

if (XMEMCMP(buffer, uint32, buflen) != 0) + all similar places below

`if (XMEMCMP(buffer, uint32, buflen) != 0)` + all similar places below

karel-m (Migrated from github.com) reviewed

2018-10-03 06:12:39 -05:00

src/pk/ecc/ecc_cmp_oid_str.c

and clang-tidy complains here:

src/pk/ecc/ecc_cmp_oid_str.c:30:4: error: do not use 'else' after 'return' [readability-else-after-return,-warnings-as-errors]
   else if (!XSTRCMP(oidstr, oid)) return CRYPT_OK;
   ^~~~~~~~

So maybe change it to: if (XSTRCMP(oidstr, oid) == 0) return CRYPT_OK;

and clang-tidy complains here: ``` src/pk/ecc/ecc_cmp_oid_str.c:30:4: error: do not use 'else' after 'return' [readability-else-after-return,-warnings-as-errors] else if (!XSTRCMP(oidstr, oid)) return CRYPT_OK; ^~~~~~~~ ``` So maybe change it to: `if (XSTRCMP(oidstr, oid) == 0) return CRYPT_OK;`

karel-m (Migrated from github.com) reviewed

2018-10-03 06:13:00 -05:00

src/pk/ecc/ecc_cmp_oid_str.c

without else

without `else`

karel-m (Migrated from github.com) reviewed

2018-10-03 06:18:15 -05:00

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c

					
				@ -0,0 +1,70 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

clang-tidy complains here (+ few other places in this file):

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c:32:7: error: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy,-warnings-as-errors]
      strcpy(buffer, "ecdsa-sha2-nistp384");
      ^

So perhaps use something else than strcpy e.g. strncpy / XMEMCPY

clang-tidy complains here (+ few other places in this file): ``` src/pk/ecc/ecc_ssh_ecdsa_encode_name.c:32:7: error: Call to function 'strcpy' is insecure as it does not provide bounding of the memory buffer. Replace unbounded copy functions with analogous functions that support length arguments such as 'strlcpy'. CWE-119 [clang-analyzer-security.insecureAPI.strcpy,-warnings-as-errors] strcpy(buffer, "ecdsa-sha2-nistp384"); ^ ``` So perhaps use something else than `strcpy` e.g. `strncpy` / `XMEMCPY`

rmw42 (Migrated from github.com) reviewed

2018-10-03 13:23:46 -05:00

src/headers/tomcrypt_pk.h

OK

rmw42 (Migrated from github.com) reviewed

2018-10-03 13:23:54 -05:00

src/misc/ssh/ssh_test.c

OK

rmw42 (Migrated from github.com) reviewed

2018-10-03 13:25:33 -05:00

src/pk/ecc/ecc_cmp_oid_str.c

If an OID can be 64 chars then the SSH 64-char limit on the "ecdsa-sha2-XXXX" string would need to be higher. Happy to change it if there are super-long OIDs out there.

rmw42 (Migrated from github.com) reviewed

2018-10-03 13:26:32 -05:00

src/pk/ecc/ecc_cmp_oid_str.c

Will do. And I'll go with the "goto error / single return" pattern so clang-tidy is happy.

rmw42 (Migrated from github.com) reviewed

2018-10-03 13:26:57 -05:00

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c

					
				@ -0,0 +1,70 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Yeah... my fingers are too familiar with powers of 2 ^_^;;

rmw42 commented

2018-10-03 13:28:07 -05:00

(Migrated from github.com)

Great job!

Cheers! :)

> Great job! Cheers! :)

rmw42 (Migrated from github.com) reviewed

2018-10-03 13:28:26 -05:00

src/misc/ssh/ssh_test.c

OK

rmw42 (Migrated from github.com) reviewed

2018-10-03 13:31:58 -05:00

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c

					
				@ -0,0 +1,70 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Yeah, the string is 19 chars long, so we need 19+1 to store it... if the buffer is 19 or fewer, it's too small. I figured "<= 19" was slightly clearer than "< 20" or "< 19 + 1", but none are particularly great.

What about:

   const char *name = "ecdsa-sha2-nistp256";
   if (buflen <= strlen(name)) { err = ... }
   XSTRCPY(buffer, name);

Yeah, the string is 19 chars long, so we need 19+1 to store it... if the buffer is 19 or fewer, it's too small. I figured "<= 19" was slightly clearer than "< 20" or "< 19 + 1", but none are particularly great. What about: ``` const char *name = "ecdsa-sha2-nistp256"; if (buflen <= strlen(name)) { err = ... } XSTRCPY(buffer, name); ```

rmw42 (Migrated from github.com) reviewed

2018-10-03 13:37:38 -05:00

src/misc/ssh/ssh_test.c

Done.

rmw42 (Migrated from github.com) reviewed

2018-10-03 14:02:29 -05:00

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c

					
				@ -0,0 +1,70 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Completely rewritten to use snprintf() and not need any hand-counted string lengths

rmw42 (Migrated from github.com) reviewed

2018-10-03 14:03:24 -05:00

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c

					
				@ -0,0 +1,70 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Replaced with snprintf() and combined with buffer length checking, so no more magic numbers.

rmw42 (Migrated from github.com) reviewed

2018-10-03 14:09:04 -05:00

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c

					
				@ -0,0 +1,70 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Done, and it writes out the length into *buflen.

rmw42 (Migrated from github.com) reviewed

2018-10-03 14:10:55 -05:00

src/pk/ecc/ecc_cmp_oid_str.c

Changed to 64

rmw42 (Migrated from github.com) reviewed

2018-10-03 14:11:59 -05:00

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c

					
				@ -0,0 +1,70 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Changed to 64

rmw42 commented

2018-10-03 14:35:46 -05:00

(Migrated from github.com)

Fixes in, and building now... hopefully that's all acceptable. Let me know if I missed anything! :)

karel-m commented

2018-10-03 16:24:36 -05:00

(Migrated from github.com)

gcc-7.3 with -Wformat=2

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c: In function ‘ecc_ssh_ecdsa_encode_name’:
src/pk/ecc/ecc_ssh_ecdsa_encode_name.c:47:4: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
    size = snprintf(buffer, *buflen, pattern, oidstr);
    ^~~~

Can we somehow avoid this warning?

gcc-7.3 with `-Wformat=2` ``` src/pk/ecc/ecc_ssh_ecdsa_encode_name.c: In function ‘ecc_ssh_ecdsa_encode_name’: src/pk/ecc/ecc_ssh_ecdsa_encode_name.c:47:4: error: format not a string literal, argument types not checked [-Werror=format-nonliteral] size = snprintf(buffer, *buflen, pattern, oidstr); ^~~~ ``` Can we somehow avoid this warning?

karel-m commented

2018-10-03 17:29:37 -05:00

(Migrated from github.com)

clang-tidy is without warnings 👍

rmw42 commented

2018-10-04 02:12:19 -05:00

(Migrated from github.com)

gcc-7.3 with -Wformat=2

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c: In function ‘ecc_ssh_ecdsa_encode_name’:
src/pk/ecc/ecc_ssh_ecdsa_encode_name.c:47:4: error: format not a string literal, argument types not checked [-Werror=format-nonliteral]
    size = snprintf(buffer, *buflen, pattern, oidstr);
    ^~~~

Can we somehow avoid this warning?

Ah, damn, an I thought I was being so clever changing the format string... well, if it has to be a literal, I can get rid of pattern entirely, use four snprintf() calls in the different branches of the if but leave the buffer length check as-is at the end...

Give me 5 minutes ;-)

> gcc-7.3 with `-Wformat=2` > > ``` > src/pk/ecc/ecc_ssh_ecdsa_encode_name.c: In function ‘ecc_ssh_ecdsa_encode_name’: > src/pk/ecc/ecc_ssh_ecdsa_encode_name.c:47:4: error: format not a string literal, argument types not checked [-Werror=format-nonliteral] > size = snprintf(buffer, *buflen, pattern, oidstr); > ^~~~ > ``` > Can we somehow avoid this warning? Ah, damn, an I thought I was being so clever changing the format string... well, if it has to be a literal, I can get rid of `pattern` entirely, use four `snprintf()` calls in the different branches of the `if` but leave the buffer length check as-is at the end... Give me 5 minutes ;-)

sjaeckel (Migrated from github.com) requested changes

2018-10-04 03:56:03 -05:00

sjaeckel (Migrated from github.com) left a comment

Looks good!

👍

Looks good! :+1:

doc/crypt.tex

please don't make this public API or do you see a future requirement from a user PoV that this is needed?

If you agree, please remove this part and move the function declaration from tomcrypt_pk.h to tomcrypt_private.h

please don't make this public API or do you see a future requirement from a user PoV that this is needed? If you agree, please remove this part and move the function declaration from `tomcrypt_pk.h` to `tomcrypt_private.h`

src/headers/tomcrypt_misc.h

					
				@ -155,2 +155,4 @@

				#endif  /* LTC_PADDING */

				#ifdef LTC_SSH

				typedef enum ssh_data_type_ {

~~This should go to tomcrypt_private.h~~

Oh no, it shouldn't.

~~This should go to `tomcrypt_private.h`~~ Oh no, it shouldn't.

src/headers/tomcrypt_misc.h

					
				@ -157,0 +167,4 @@

				} ssh_data_type;

				/* VA list handy helpers with tuples of <type, data> */

				int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...);

instead this should probably be as follows?

int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, int type, void* data, ...);

not sure though if it would clutter the implementation too much!?

(same applies to ssh_decode_sequence_multi())

instead this should probably be as follows? `int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, int type, void* data, ...);` not sure though if it would clutter the implementation too much!? (same applies to `ssh_decode_sequence_multi()`)

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

string terminator is '\0'

string terminator is `'\0'`

same

I doubt this works on all architectures *cough* ;)

I doubt this works on all architectures [\*cough\*](https://github.com/libtom/libtomcrypt/issues/306#issuecomment-333765095) ;)

First I thought why we can't use strlcpy()... then I remembered this standardization thingy... as we stay with this please introduce a macro XSTRNCPY in tomcrypt_custom.h (and also add it to the list in crypt.tex and helper.pl please)

First I thought why we can't use `strlcpy()`... then I remembered this standardization thingy... as we stay with this please introduce a macro `XSTRNCPY` in `tomcrypt_custom.h` (and also add it to the list in `crypt.tex` and `helper.pl` please)

can't we use LOAD32H() here?

The same applies to all the other cases where we load some unsigned long (use LOAD64H where appropriate) resp. we could use STORE{32,64}H() when storing those values.

can't we use `LOAD32H()` here? The same applies to all the other cases where we load some `unsigned long` (use `LOAD64H` where appropriate) resp. we could use `STORE{32,64}H()` when storing those values.

Btw. we also have types for exactly that purpose, ulong32 and ulong64 (see tomcrypt_cfg.h for details)

Btw. we also have types for exactly that purpose, `ulong32` and `ulong64` (see `tomcrypt_cfg.h` for details)

why is a byte stored as unsigned long?

why is a byte stored as `unsigned long`?

~~I just realized that the src argument in unsigned_read() in tomcrypt_math.h could be changed to const unsigned char *src as both ltm and tfm nowadays have const* arguments where applicable~~ oops ... ltm isn't released yet with those changes... *cough* https://github.com/libtom/libtommath/issues/122 *cough*

~~I just realized that the `src` argument in `unsigned_read()` in `tomcrypt_math.h` could be changed to `const unsigned char *src` as both ltm and tfm nowadays have `const*` arguments where applicable~~ oops ... ltm isn't released yet with those changes... \*cough\* https://github.com/libtom/libtommath/issues/122 \*cough\*

src/misc/ssh/ssh_encode_sequence_multi.c

					
				@ -0,0 +110,4 @@

				            STORE32H(u32data, out);

				            out += 4;

				            break;

				         case LTC_SSHDATA_UINT64:

c.f. comment regarding LTC_SSHDATA_UINT64 in ssh_decode_sequence_multi()

c.f. comment regarding `LTC_SSHDATA_UINT64` in `ssh_decode_sequence_multi()`

tests/ecc_test.c

					
				@ -37,6 +37,93 @@ static unsigned int sizes[] = {

				#endif

static and a more meaningful name please

`static` and a more meaningful name please

tests/ssh_test.c

					
				@ -0,0 +1,310 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

You can also use DO(do_compare_testvector(..., this prints some more information on why it failed.

... we could probably also add a macro ...

#define COMPARE_TESTVECTOR(i, il, s, sl, wa, wi) do { DO(do_compare_testvector((i), (il), (s), (sl), (wa), (wi))); } while(0)

You can also use `DO(do_compare_testvector(...`, this prints some more information on why it failed. ... we could probably also add a macro ... ``` #define COMPARE_TESTVECTOR(i, il, s, sl, wa, wi) do { DO(do_compare_testvector((i), (il), (s), (sl), (wa), (wi))); } while(0) ```

rmw42 (Migrated from github.com) reviewed

2018-10-04 13:08:51 -05:00

doc/crypt.tex

I put it in the same places as ecc_get_oid_str(), but this probably shouldn't be public - it's unlikely that a user application will have created a key but not know what type it is, so this only really makes sense for internal checks.

Will move it and remove the docs.

I put it in the same places as `ecc_get_oid_str()`, but this probably shouldn't be public - it's unlikely that a user application will have created a key but not know what type it is, so this only really makes sense for internal checks. Will move it and remove the docs.

rmw42 (Migrated from github.com) reviewed

2018-10-04 13:09:11 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

OK

rmw42 (Migrated from github.com) reviewed

2018-10-04 13:11:10 -05:00

tests/ecc_test.c

					
				@ -37,6 +37,93 @@ static unsigned int sizes[] = {

				#endif

That's what it was called when it was function-local ;-)
I'll rename it to curvenames and make it static.

That's what it was called when it was function-local ;-) I'll rename it to `curvenames` and make it static.

rmw42 (Migrated from github.com) reviewed

2018-10-04 13:13:50 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Managed to outthink myself considering register passing conventions/what this would be on the 'stack'/that it's a cast of void * when encoding... basically being silly. I'll make it something sane - uint8?

Managed to outthink myself considering register passing conventions/what this would be on the 'stack'/that it's a cast of `void *` when encoding... basically being silly. I'll make it something sane - uint8?

rmw42 commented

2018-10-04 13:19:39 -05:00

(Migrated from github.com)

in case of CRYPT_BUFFER_OVERFLOW we should return required buffer length

Done. I've refactored so it always returns the buffer length (i.e. string length + 1).

> in case of `CRYPT_BUFFER_OVERFLOW` we should return required buffer length Done. I've refactored so it always returns the buffer length (i.e. string length + 1).

rmw42 (Migrated from github.com) reviewed

2018-10-04 13:21:18 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Ah, those were exactly what I was looking for. Cheers! Fixing now :)

rmw42 (Migrated from github.com) reviewed

2018-10-04 13:28:03 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Awesome. That's much tidier now :)

rmw42 (Migrated from github.com) reviewed

2018-10-04 13:41:06 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Done. I've also added a check for strcpy in helper.pl - not used anywhere in the code as far as I can tell, but presumably it should be alerted if someone introduces it.

Done. I've also added a check for `strcpy` in `helper.pl` - not used anywhere in the code as far as I can tell, but presumably it should be alerted if someone introduces it.

rmw42 (Migrated from github.com) reviewed

2018-10-04 13:42:07 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

unsigned char then

`unsigned char` then

rmw42 (Migrated from github.com) reviewed

2018-10-04 13:51:53 -05:00

src/headers/tomcrypt_misc.h

					
				@ -157,0 +167,4 @@

				} ssh_data_type;

				/* VA list handy helpers with tuples of <type, data> */

				int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...);

Not sure how well va_args lists handle restarting in the middle of the fixed parameters?

I'm going to alter the implementation so we don't have void *data being cast all over the place but explicitly collect the correct type from the stack.

Not sure how well va_args lists handle restarting in the middle of the fixed parameters? I'm going to alter the implementation so we don't have `void *data` being cast all over the place but explicitly collect the correct type from the stack.

sjaeckel (Migrated from github.com) reviewed

2018-10-04 14:00:57 -05:00

src/headers/tomcrypt_misc.h

					
				@ -157,0 +167,4 @@

				} ssh_data_type;

				/* VA list handy helpers with tuples of <type, data> */

				int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...);

how about something like:

diff --git a/src/misc/ssh/ssh_decode_sequence_multi.c b/src/misc/ssh/ssh_decode_sequence_multi.c
index 2507609..3ace026 100644
--- a/src/misc/ssh/ssh_decode_sequence_multi.c
+++ b/src/misc/ssh/ssh_decode_sequence_multi.c
@@ -25,7 +25,7 @@
 */
-int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
+int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, int type, void *data, ...)
 {
    int           err;
-   ssh_data_type type;
-   void          *data;
+   ssh_data_type t;
+   void          *d;
    va_list       args;
@@ -36,7 +36,6 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
    /* Decode values from buffer */
-   va_start(args, inlen);
+   va_start(args, data);
+   t = type;
+   d = data;
    for (;;) {
-      type = (ssh_data_type)va_arg(args, int);
-      data = va_arg(args, void*);
-      LTC_UNUSED_PARAM(data);
 
@@ -46,3 +45,3 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
 
-      switch (type) {
+      switch (t) {
          case LTC_SSHDATA_BYTE:
@@ -122,9 +121,12 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
          case LTC_SSHDATA_EOL: /* Should never get here */
-            va_end(args);
-            return CRYPT_INVALID_ARG;
+            err = CRYPT_INVALID_ARG;
+            goto out;
        }
+      t = (ssh_data_type)va_arg(args, int);
+      d = va_arg(args, void*);
    }
+out:
    va_end(args);
 
-   return CRYPT_OK;
+   return err;
 }

[edit]: FYI yes, that's an incomplete patch ;)

how about something like: ``` diff --git a/src/misc/ssh/ssh_decode_sequence_multi.c b/src/misc/ssh/ssh_decode_sequence_multi.c index 2507609..3ace026 100644 --- a/src/misc/ssh/ssh_decode_sequence_multi.c +++ b/src/misc/ssh/ssh_decode_sequence_multi.c @@ -25,7 +25,7 @@ */ -int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) +int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, int type, void *data, ...) { int err; - ssh_data_type type; - void *data; + ssh_data_type t; + void *d; va_list args; @@ -36,7 +36,6 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) /* Decode values from buffer */ - va_start(args, inlen); + va_start(args, data); + t = type; + d = data; for (;;) { - type = (ssh_data_type)va_arg(args, int); - data = va_arg(args, void*); - LTC_UNUSED_PARAM(data); @@ -46,3 +45,3 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) - switch (type) { + switch (t) { case LTC_SSHDATA_BYTE: @@ -122,9 +121,12 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) case LTC_SSHDATA_EOL: /* Should never get here */ - va_end(args); - return CRYPT_INVALID_ARG; + err = CRYPT_INVALID_ARG; + goto out; } + t = (ssh_data_type)va_arg(args, int); + d = va_arg(args, void*); } +out: va_end(args); - return CRYPT_OK; + return err; } ``` [edit]: FYI yes, that's an incomplete patch ;)

rmw42 (Migrated from github.com) reviewed

2018-10-04 14:08:07 -05:00

src/headers/tomcrypt_misc.h

					
				@ -157,0 +167,4 @@

				} ssh_data_type;

				/* VA list handy helpers with tuples of <type, data> */

				int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...);

I've gone for something a bit more radical... just updating the tests to cover byte/boolean/uint64 now. Let me know what you think, I can always change it if you're not happy.

rmw42 (Migrated from github.com) reviewed

2018-10-04 14:52:48 -05:00

tests/ssh_test.c

					
				@ -0,0 +1,310 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Done

sjaeckel (Migrated from github.com) reviewed

2018-10-04 14:56:02 -05:00

helper.pl

					
				@ -53,6 +53,8 @@ sub check_source {

				      push @{$troubles->{unwanted_memmove}}, $lineno if $file =~ /^src\/.*\.c$/ && $l =~ /\bmemmove\s*\(/;

				      push @{$troubles->{unwanted_memcmp}},  $lineno if $file =~ /^src\/.*\.c$/ && $l =~ /\bmemcmp\s*\(/;

				      push @{$troubles->{unwanted_strcmp}},  $lineno if $file =~ /^src\/.*\.c$/ && $l =~ /\bstrcmp\s*\(/;

				      push @{$troubles->{unwanted_strcpy}},  $lineno if $file =~ /^src\/.*\.c$/ && $l =~ /\bstrcpy\s*\(/;

👍

:+1:

rmw42 (Migrated from github.com) reviewed

2018-10-04 16:15:08 -05:00

src/headers/tomcrypt_misc.h

					
				@ -157,0 +167,4 @@

				} ssh_data_type;

				/* VA list handy helpers with tuples of <type, data> */

				int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...);

Changed to a single return path and re-pushed.

sjaeckel (Migrated from github.com) reviewed

2018-10-04 16:21:13 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

TBH I prefer a switch-case over a long else-if chain for two reasons:

it's cleaner to read IMO
if you do the switch on an enum and don't add a default case, you get immediate feedback from the compiler that the switch has unhandled cases of the enum

TBH I prefer a switch-case over a long else-if chain for two reasons: 1. it's cleaner to read IMO 2. if you do the switch on an enum and don't add a default case, you get immediate feedback from the compiler that the switch has unhandled cases of the enum

rmw42 (Migrated from github.com) reviewed

2018-10-04 16:25:39 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

I was mostly concerned about the need for a bunch of variable definitions at the top of the function which would each only be used by a single case. What about switch-case with braced code blocks? e.g.:

case LTC_SSHDATA_BYTE:
{
   unsigned char *data = va_arg(args, unsigned char *);
...
}
break;

I was mostly concerned about the need for a bunch of variable definitions at the top of the function which would each only be used by a single case. What about switch-case with braced code blocks? e.g.: ``` case LTC_SSHDATA_BYTE: { unsigned char *data = va_arg(args, unsigned char *); ... } break; ```

sjaeckel (Migrated from github.com) reviewed

2018-10-04 16:29:22 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

I was mostly concerned about the need for a bunch of variable definitions at the top of the function which would each only be used by a single case. What about switch-case with braced code blocks?

I don't have a problem with both, but it's true - the braced code blocks worsen readability again

> I was mostly concerned about the need for a bunch of variable definitions at the top of the function which would each only be used by a single case. What about switch-case with braced code blocks? I don't have a problem with both, but it's true - the braced code blocks worsen readability again

rmw42 (Migrated from github.com) reviewed

2018-10-05 03:44:03 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

I guess there's not going to be any way to make it pretty :(

Switch-case with variable definitions at the top of the loop, then? And I can put type into the formal arg list, change the for to while (type != LTC_SSHDATA_EOL) and put the reading of type at the end of the block. Sound good?

Any other changes I should make after that?

I guess there's not going to be any way to make it pretty :( Switch-case with variable definitions at the top of the loop, then? And I can put `type` into the formal arg list, change the `for` to `while (type != LTC_SSHDATA_EOL)` and put the reading of `type` at the end of the block. Sound good? Any other changes I should make after that?

rmw42 (Migrated from github.com) reviewed

2018-10-05 04:18:17 -05:00

src/headers/tomcrypt_misc.h

					
				@ -157,0 +167,4 @@

				} ssh_data_type;

				/* VA list handy helpers with tuples of <type, data> */

				int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...);

Ah... warning: second argument to 'va_start' is not the last named parameter [-Wvarargs]. Since it has to go through the loop twice, type can't be in the formal parameters for ssh_encode_sequence_multi...

Ah... `warning: second argument to 'va_start' is not the last named parameter [-Wvarargs]`. Since it has to go through the loop twice, `type` can't be in the formal parameters for `ssh_encode_sequence_multi`...

sjaeckel (Migrated from github.com) reviewed

2018-10-05 04:20:56 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

I guess there's not going to be any way to make it pretty :(

unfortunately not :/

Switch-case with variable definitions at the top of the loop, then?

I'd say at the top of the function, not the loop

And I can put type into the formal arg list, change the for to while (type != LTC_SSHDATA_EOL) and put the reading of type at the end of the block. Sound good?

why not change the signature to the proposed int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, int type, void *data, ...)?

Should it really be a valid case to call ssh_decode_sequence_multi(foo, bar, LTC_SSHDATA_EOL)?

> I guess there's not going to be any way to make it pretty :( unfortunately not :/ > Switch-case with variable definitions at the top of the loop, then? I'd say at the top of the function, not the loop > And I can put `type` into the formal arg list, change the `for` to `while (type != LTC_SSHDATA_EOL)` and put the reading of `type` at the end of the block. Sound good? why not change the signature to the proposed `int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, int type, void *data, ...)`? Should it really be a valid case to call `ssh_decode_sequence_multi(foo, bar, LTC_SSHDATA_EOL)`?

sjaeckel (Migrated from github.com) reviewed

2018-10-05 04:30:28 -05:00

src/headers/tomcrypt_misc.h

					
				@ -157,0 +167,4 @@

				} ssh_data_type;

				/* VA list handy helpers with tuples of <type, data> */

				int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...);

I guess you forgot to update the va_start() argument...

diff --git a/src/misc/ssh/ssh_decode_sequence_multi.c b/src/misc/ssh/ssh_decode_sequence_multi.c
index ca088f9..e44162a 100644
--- a/src/misc/ssh/ssh_decode_sequence_multi.c
+++ b/src/misc/ssh/ssh_decode_sequence_multi.c
@@ -25,6 +25,6 @@
 */
-int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
+int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, int type_, ...)
 {
    int           err;
-   ssh_data_type type;
+   ssh_data_type type = type_;
    va_list       args;
@@ -34,5 +34,4 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
    /* Decode values from buffer */
-   va_start(args, inlen);
+   va_start(args, type_);
    for (;;) {
-      type = (ssh_data_type)va_arg(args, int);
 
@@ -103,2 +102,3 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
       }
+      type = (ssh_data_type)va_arg(args, int);
    }

this compiles w/o warning and tests are OK
(please don't call it type_ ;) )

I guess you forgot to update the `va_start()` argument... ``` diff --git a/src/misc/ssh/ssh_decode_sequence_multi.c b/src/misc/ssh/ssh_decode_sequence_multi.c index ca088f9..e44162a 100644 --- a/src/misc/ssh/ssh_decode_sequence_multi.c +++ b/src/misc/ssh/ssh_decode_sequence_multi.c @@ -25,6 +25,6 @@ */ -int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) +int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, int type_, ...) { int err; - ssh_data_type type; + ssh_data_type type = type_; va_list args; @@ -34,5 +34,4 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) /* Decode values from buffer */ - va_start(args, inlen); + va_start(args, type_); for (;;) { - type = (ssh_data_type)va_arg(args, int); @@ -103,2 +102,3 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) } + type = (ssh_data_type)va_arg(args, int); } ``` this compiles w/o warning and tests are OK (please don't call it `type_` ;) )

rmw42 (Migrated from github.com) reviewed

2018-10-05 04:30:55 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

I can't put type into the formal parameters because the encoding needs to go through the list twice, and can't be va_started from the middle of the formal parameters, so it would make the signatures of the two methods different :(
And data shouldn't really go in there because it then forces things to be void* pointers even when they should be integers :(

I can't put `type` into the formal parameters because the encoding needs to go through the list twice, and can't be `va_start`ed from the middle of the formal parameters, so it would make the signatures of the two methods different :( And `data` shouldn't really go in there because it then forces things to be `void*` pointers even when they should be integers :(

rmw42 (Migrated from github.com) reviewed

2018-10-05 04:35:01 -05:00

src/headers/tomcrypt_misc.h

					
				@ -157,0 +167,4 @@

				} ssh_data_type;

				/* VA list handy helpers with tuples of <type, data> */

				int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...);

That's different to how der_encode_sequence_multi etc handle varargs, but I can do it that way if you want?

That's different to how `der_encode_sequence_multi` etc handle varargs, but I can do it that way if you want?

sjaeckel (Migrated from github.com) reviewed

2018-10-05 04:37:17 -05:00

src/headers/tomcrypt_misc.h

					
				@ -157,0 +167,4 @@

				} ssh_data_type;

				/* VA list handy helpers with tuples of <type, data> */

				int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...);

OK, leave the signatures as they are, sorry for the noise :)

sjaeckel (Migrated from github.com) reviewed

2018-10-05 04:39:49 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Both valid points I didn't see!

Again, sorry for the noise!

Both valid points I didn't see! Again, sorry for the noise!

sjaeckel (Migrated from github.com) reviewed

2018-10-05 04:41:01 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

But please, still change back to switch-case :)

rmw42 (Migrated from github.com) reviewed

2018-10-05 04:46:26 -05:00

src/headers/tomcrypt_misc.h

					
				@ -157,0 +167,4 @@

				} ssh_data_type;

				/* VA list handy helpers with tuples of <type, data> */

				int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...);

No worries... it's annoying that there's no elegant solution, it's like an itch in the brain! :)

rmw42 (Migrated from github.com) reviewed

2018-10-05 04:46:59 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Done, and I've moved the params to the top of the function.

rmw42 commented

2018-10-05 04:49:11 -05:00

(Migrated from github.com)

I added a new check in ssh_decode_sequence_multi to ensure we don't read past the end of the buffer, which caught a bug in the name-list decoding tests (I'd used sizeof(string) instead of sizeof(nlist3) - copy/paste bug).

I added a new check in `ssh_decode_sequence_multi` to ensure we don't read past the end of the buffer, which caught a bug in the name-list decoding tests (I'd used `sizeof(string)` instead of `sizeof(nlist3)` - copy/paste bug).

rmw42 commented

2018-10-05 04:55:18 -05:00

(Migrated from github.com)

OK, I've rebased to current develop branch (might need to do so again with the ecc_verify_hash_ex reversion), and I think that's all the things we've discussed... if there are any more issues, please let me know 😄

OK, I've rebased to current develop branch (might need to do so again with the `ecc_verify_hash_ex` reversion), and I think that's all the things we've discussed... if there are any more issues, please let me know 😄

karel-m (Migrated from github.com) approved these changes

2018-10-06 05:35:06 -05:00

sjaeckel (Migrated from github.com) reviewed

2018-10-06 06:36:55 -05:00

sjaeckel (Migrated from github.com) left a comment

Sorry for more nitpicks, minor comments, but there's also one important thing I only spotted now (regarding the length-checks when decoding)

src/headers/tomcrypt_misc.h

					
				@ -154,6 +154,23 @@ int padding_pad(unsigned char *data, unsigned long length, unsigned long* padded

				int padding_depad(const unsigned char *data, unsigned long *length, unsigned long mode);

oops, just found that this doesn't belong here anymore but in tests/tomcrypt_test.h

oops, just found that this doesn't belong here anymore but in `tests/tomcrypt_test.h`

src/headers/tomcrypt_pk.h

					
				@ -251,7 +251,9 @@ typedef enum ecc_signature_type_ {

				   /* raw R, S values */

I'm not 100% sure if this shouldn't be within #ifdef LTC_SSH?

if we're already at it, can you please directly add a , at the end :)

I'm not 100% sure if this shouldn't be within `#ifdef LTC_SSH`? if we're already at it, can you please directly add a `,` at the end :)

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

❤️

:heart:

shouldn't this kind of check be in each case where we read more than 1 byte?

src/misc/ssh/ssh_encode_sequence_multi.c

					
				@ -0,0 +82,4 @@

				   /* Check we have sufficient space */

				   if (*outlen < size) {

				      *outlen = size;

				      err = CRYPT_BUFFER_OVERFLOW;

didn't we say we should return the required size in this error-case?

src/pk/ecc/ecc_cmp_oid_str.c

if we merge this after #404 there's going to be int pk_oid_cmp_with_ulong(const char *o1, const unsigned long *o2, unsigned long o2size); ;)

if we merge this after #404 there's going to be `int pk_oid_cmp_with_ulong(const char *o1, const unsigned long *o2, unsigned long o2size);` ;)

tests/common.h

that define is only required once after the #endif

that define is only required once after the `#endif`

rmw42 (Migrated from github.com) reviewed

2018-10-06 06:50:12 -05:00

src/headers/tomcrypt_misc.h

					
				@ -154,6 +154,23 @@ int padding_pad(unsigned char *data, unsigned long length, unsigned long* padded

				int padding_depad(const unsigned char *data, unsigned long *length, unsigned long mode);

Moved.

rmw42 (Migrated from github.com) reviewed

2018-10-06 06:53:04 -05:00

src/headers/tomcrypt_pk.h

					
				@ -251,7 +251,9 @@ typedef enum ecc_signature_type_ {

				   /* raw R, S values */

I think it makes sense to define the signature format in all cases, whether or not the implementation is built into the library. If LTC_SSH isn't defined, then it'll fall through to the "unknown format" errors in the functions.

I think a trailing comma in an enum results in a compiler warning. Not sure, though?

I think it makes sense to _define_ the signature format in all cases, whether or not the implementation is built into the library. If `LTC_SSH` isn't defined, then it'll fall through to the "unknown format" errors in the functions. I think a trailing comma in an enum results in a compiler warning. Not sure, though?

rmw42 (Migrated from github.com) reviewed

2018-10-06 06:58:13 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Probably, but it would make the implementation a lot uglier. It'd mean we couldn't use the LOAD32H macros as they don't do checks after each byte.

The main reason I could see for doing that is that there might be information leaked if it can read past the end of the buffer... but only by someone with the ability to change the code, who could read from that memory anyway.

It's in the loop, so it will be checked after each object read, whether that's 4 or 8 or however many bytes. Personally, I think that's OK, but I'm happy to move it if you disagree.

Probably, but it would make the implementation a lot uglier. It'd mean we couldn't use the `LOAD32H` macros as they don't do checks after each byte. The main reason I could see for doing that is that there might be information leaked if it can read past the end of the buffer... but only by someone with the ability to change the code, who could read from that memory anyway. It's in the loop, so it will be checked after each object read, whether that's 4 or 8 or however many bytes. Personally, I think that's OK, but I'm happy to move it if you disagree.

rmw42 (Migrated from github.com) reviewed

2018-10-06 06:58:44 -05:00

src/misc/ssh/ssh_encode_sequence_multi.c

					
				@ -0,0 +82,4 @@

				   /* Check we have sufficient space */

				   if (*outlen < size) {

				      *outlen = size;

				      err = CRYPT_BUFFER_OVERFLOW;

Oops. Changed another case (ecc_ssh_decode_name) but missed this one. Fixing it now.

Oops. Changed another case (`ecc_ssh_decode_name`) but missed this one. Fixing it now.

rmw42 (Migrated from github.com) reviewed

2018-10-06 07:01:47 -05:00

src/pk/ecc/ecc_cmp_oid_str.c

I can drop ecc_cmp_oid_str entirely, then. Turned out that it was only needed for the secp256k1 checks for LTC_ECCSIG_ETH27. Let me know if we want to do that.

I can drop `ecc_cmp_oid_str` entirely, then. Turned out that it was only needed for the secp256k1 checks for `LTC_ECCSIG_ETH27`. Let me know if we want to do that.

rmw42 (Migrated from github.com) reviewed

2018-10-06 07:01:57 -05:00

tests/common.h

OK

sjaeckel (Migrated from github.com) reviewed

2018-10-06 07:49:04 -05:00

src/headers/tomcrypt_pk.h

					
				@ -251,7 +251,9 @@ typedef enum ecc_signature_type_ {

				   /* raw R, S values */

I think it makes sense to define the signature format in all cases

agreed!

I think a trailing comma in an enum results in a compiler warning. Not sure, though?

shouldn't on modern compilers :) (e.g. the ltc_asn1_XX enums have it as well)

> I think it makes sense to _define_ the signature format in all cases agreed! > I think a trailing comma in an enum results in a compiler warning. Not sure, though? shouldn't on modern compilers :) (e.g. the `ltc_asn1_XX` enums have it as well)

sjaeckel (Migrated from github.com) reviewed

2018-10-06 07:53:07 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

it would make the implementation a lot uglier

sure, but it's required IMO as the following isn't the only bad thing that could happen IMO

The main reason I could see for doing that is that there might be information leaked if it can read past the end of the buffer...

... but if you put the buffer at the end of a page it could also segfault or not?

And isn't it even UB to read past an allocated buffer?

> it would make the implementation a lot uglier sure, but it's required IMO as the following isn't the only bad thing that could happen IMO > The main reason I could see for doing that is that there might be information leaked if it can read past the end of the buffer... ... but if you put the buffer at the end of a page it could also segfault or not? And isn't it even UB to read past an allocated buffer?

sjaeckel (Migrated from github.com) reviewed

2018-10-06 07:54:11 -05:00

src/pk/ecc/ecc_cmp_oid_str.c

We keep it for now as is I'd say as the other PR seems to take some more iterations as it's not entirely ready yet.

sjaeckel (Migrated from github.com) reviewed

2018-10-06 08:08:39 -05:00

src/pk/ecc/ecc_cmp_oid_str.c

Even better, simply cherry-pick those commits and whoever gets merged first will have them and the second will be rebased and the commits removed :) Git FTW

c0407783ac383f9509a62dde99de4b6606c4d4f7
c81e1cb1dc8b36286233605c3a10c47584ec0e79
6abfab4d03389ccfe2c31f7ead84c706befc3e60

Even better, simply cherry-pick those commits and whoever gets merged first will have them and the second will be rebased and the commits removed :) Git FTW c0407783ac383f9509a62dde99de4b6606c4d4f7 c81e1cb1dc8b36286233605c3a10c47584ec0e79 6abfab4d03389ccfe2c31f7ead84c706befc3e60

rmw42 (Migrated from github.com) reviewed

2018-10-06 10:38:11 -05:00

src/headers/tomcrypt_pk.h

					
				@ -251,7 +251,9 @@ typedef enum ecc_signature_type_ {

				   /* raw R, S values */

shouldn't on modern compilers

Yup. I thought I'd seen this somewhere recently, but apparently not - it compiles cleanly.

> shouldn't on modern compilers Yup. I thought I'd seen this somewhere recently, but apparently not - it compiles cleanly.

rmw42 (Migrated from github.com) reviewed

2018-10-06 10:53:55 -05:00

src/pk/ecc/ecc_cmp_oid_str.c

Took a while to figure out the syntax - thought I'd have to name the branch I was pulling from - but that was remarkably painless! 🍒

I'll get rid of ecc_cmp_oid_str now.

Took a while to figure out the syntax - thought I'd have to name the branch I was pulling from - but that was remarkably painless! 🍒 I'll get rid of `ecc_cmp_oid_str` now.

rmw42 (Migrated from github.com) reviewed

2018-10-06 11:17:43 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Well, the same would apply to anything that reads from a buffer if the specified length is greater than the size of the buffer... it shouldn't segfault unless someone has done something weird with the virtual memory page protection. Even if it does, if an application asks us to read 1,000,000 bytes from a 1,000 byte buffer, or passes us a pointer which cannot be read from at all, is a segfault not an appropriate response?

Instead of a sentinel at the end, I could keep track of the bytes available to read, and put a second switch-case in the loop to calculate the size which will be read? That's still going to be somewhat delicate around the variable-length string/mpint cases...

Well, the same would apply to _anything_ that reads from a buffer if the specified length is greater than the size of the buffer... it shouldn't segfault unless someone has done something weird with the virtual memory page protection. Even if it does, if an application asks us to read 1,000,000 bytes from a 1,000 byte buffer, or passes us a pointer which cannot be read from at all, is a segfault not an appropriate response? Instead of a sentinel at the end, I could keep track of the bytes available to read, and put a second switch-case in the loop to calculate the size which will be read? That's still going to be somewhat delicate around the variable-length string/mpint cases...

rmw42 (Migrated from github.com) reviewed

2018-10-06 11:33:55 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

OK, I've ended up with three switch-cases:

the length field of the object (if needed)
read/calculate the length of the object
read the object itself

Should now be impossible for ssh_decode_sequence_multi to read beyond the end of the input.

OK, I've ended up with three switch-cases: - the length field of the object (if needed) - read/calculate the length of the object - read the object itself Should now be impossible for `ssh_decode_sequence_multi` to read beyond the end of the input.

sjaeckel (Migrated from github.com) reviewed

2018-10-06 11:39:04 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

is a segfault not an appropriate response?

sure it is and if we can prevent it by properly checking indicators like the required vs available length we should do it IMO.

Instead of a sentinel at the end, I could keep track of the bytes available to read, and put a second switch-case in the loop to calculate the size which will be read? That's still going to be somewhat delicate around the variable-length string/mpint cases...

how about

diff --git a/src/misc/ssh/ssh_decode_sequence_multi.c b/src/misc/ssh/ssh_decode_sequence_multi.c
index 2375df8..aa171b3 100644
--- a/src/misc/ssh/ssh_decode_sequence_multi.c
+++ b/src/misc/ssh/ssh_decode_sequence_multi.c
@@ -37,14 +37,21 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
    unsigned long size, bufsize;
+   long len;
 
    LTC_ARGCHK(in    != NULL);
+   LTC_ARGCHK(inlen <= LONG_MAX);
 
    /* Decode values from buffer */
+   len = inlen;
    va_start(args, inlen);
    while ((type = (ssh_data_type)va_arg(args, int)) != LTC_SSHDATA_EOL) {
+      err = CRYPT_BUFFER_OVERFLOW;
       switch (type) {
          case LTC_SSHDATA_BYTE:
+            if (len < 1) goto error;
             cdata = va_arg(args, unsigned char*);
             *cdata = *in++;
+            len--;
             break;
          case LTC_SSHDATA_BOOLEAN:
+            if (len < 1) goto error;
             cdata = va_arg(args, unsigned char*);
@@ -55,4 +62,6 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
             *cdata = (*in++)?1:0;
+            len--;
             break;
          case LTC_SSHDATA_UINT32:
+            if (len < 4) goto error;
             u32data = va_arg(args, ulong32*);
@@ -60,4 +69,6 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
             in += 4;
+            len -= 4;
             break;
          case LTC_SSHDATA_UINT64:
+            if (len < 8) goto error;
             u64data = va_arg(args, ulong64*);
@@ -65,2 +76,3 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
             in += 8;
+            len -= 8;
             break;
@@ -73,6 +85,3 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
             if (size > 0) {
-               if (size >= bufsize) {
-                  err = CRYPT_BUFFER_OVERFLOW;
-                  goto error;
-               }
+               if (size >= bufsize) goto error;
                XSTRNCPY(sdata, (const char *)in, size);
@@ -83,4 +92,6 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
             in += size;
+            len -= size;
             break;
          case LTC_SSHDATA_MPINT:
+            if (len < 4) goto error;
             vdata = va_arg(args, void*);
@@ -91,2 +102,3 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
             } else {
+               if (len < size) goto error;
                if ((err = mp_read_unsigned_bin(vdata, (unsigned char *)in, size)) != CRYPT_OK)          { goto error; }
@@ -94,2 +106,3 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
             in += size;
+            len -= size;
             break;

> is a segfault not an appropriate response? sure it is and if we can prevent it by properly checking indicators like the required vs available length we should do it IMO. > Instead of a sentinel at the end, I could keep track of the bytes available to read, and put a second switch-case in the loop to calculate the size which will be read? That's still going to be somewhat delicate around the variable-length string/mpint cases... how about ``` diff --git a/src/misc/ssh/ssh_decode_sequence_multi.c b/src/misc/ssh/ssh_decode_sequence_multi.c index 2375df8..aa171b3 100644 --- a/src/misc/ssh/ssh_decode_sequence_multi.c +++ b/src/misc/ssh/ssh_decode_sequence_multi.c @@ -37,14 +37,21 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) unsigned long size, bufsize; + long len; LTC_ARGCHK(in != NULL); + LTC_ARGCHK(inlen <= LONG_MAX); /* Decode values from buffer */ + len = inlen; va_start(args, inlen); while ((type = (ssh_data_type)va_arg(args, int)) != LTC_SSHDATA_EOL) { + err = CRYPT_BUFFER_OVERFLOW; switch (type) { case LTC_SSHDATA_BYTE: + if (len < 1) goto error; cdata = va_arg(args, unsigned char*); *cdata = *in++; + len--; break; case LTC_SSHDATA_BOOLEAN: + if (len < 1) goto error; cdata = va_arg(args, unsigned char*); @@ -55,4 +62,6 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) *cdata = (*in++)?1:0; + len--; break; case LTC_SSHDATA_UINT32: + if (len < 4) goto error; u32data = va_arg(args, ulong32*); @@ -60,4 +69,6 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) in += 4; + len -= 4; break; case LTC_SSHDATA_UINT64: + if (len < 8) goto error; u64data = va_arg(args, ulong64*); @@ -65,2 +76,3 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) in += 8; + len -= 8; break; @@ -73,6 +85,3 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) if (size > 0) { - if (size >= bufsize) { - err = CRYPT_BUFFER_OVERFLOW; - goto error; - } + if (size >= bufsize) goto error; XSTRNCPY(sdata, (const char *)in, size); @@ -83,4 +92,6 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) in += size; + len -= size; break; case LTC_SSHDATA_MPINT: + if (len < 4) goto error; vdata = va_arg(args, void*); @@ -91,2 +102,3 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) } else { + if (len < size) goto error; if ((err = mp_read_unsigned_bin(vdata, (unsigned char *)in, size)) != CRYPT_OK) { goto error; } @@ -94,2 +106,3 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...) in += size; + len -= size; break; ```

rmw42 (Migrated from github.com) reviewed

2018-10-06 12:37:51 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

Yeah, that'd work too, but some of the variable-length ones would need extra checks.

How's the triple-switch version? It's a bit longer but maybe clearer intent...

Yeah, that'd work too, but some of the variable-length ones would need extra checks. How's the triple-switch version? It's a bit longer but maybe clearer intent...

sjaeckel (Migrated from github.com) reviewed

2018-10-06 12:46:18 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +1,161 @@

				/* LibTomCrypt, modular cryptographic library -- Tom St Denis

How's the triple-switch version?

I'm not too much a fan of it, @karel-m your thoughts?

> How's the triple-switch version? I'm not too much a fan of it, @karel-m your thoughts?

rmw42 commented

2018-10-08 03:00:28 -05:00

(Migrated from github.com)

I've updated the length-check to remove one of the switches and keep track of the length remaining, rather than the end pointer, and rebased to the develop branch to fix the merge issues around cherry-picking pk_oid_cmp_with_ulong.

I've updated the length-check to remove one of the switches and keep track of the length remaining, rather than the end pointer, and rebased to the `develop` branch to fix the merge issues around cherry-picking `pk_oid_cmp_with_ulong`.

sjaeckel commented

2018-10-08 04:34:04 -05:00

(Migrated from github.com)

@mkj did you have a look at this?

mkj (Migrated from github.com) reviewed

2018-10-08 09:25:12 -05:00

mkj (Migrated from github.com) left a comment

Looks OK, couple of small things I've noted.

Unrelated to the patch, I'm curious where SSH signature encoding is being used externally to the SSH protocol?

Looks OK, couple of small things I've noted. Unrelated to the patch, I'm curious where SSH signature encoding is being used externally to the SSH protocol?

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +123,4 @@

				               XSTRNCPY(sdata, (const char *)in, size);

				               sdata[size] = '\0'; /* strncpy doesn't NUL-terminate */

				            } else {

				               *sdata = '\0';

Invalid write if size==0 && bufsize==0

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +136,4 @@

				               err = CRYPT_INVALID_PACKET;

				               goto error;

				            } else {

				               if ((err = mp_read_unsigned_bin(vdata, (unsigned char *)in, size)) != CRYPT_OK)          { goto error; }

Check that the top bit isn't set, that implies a negative value
https://tools.ietf.org/html/rfc4251#page-8

Check that the top bit isn't set, that implies a negative value https://tools.ietf.org/html/rfc4251#page-8

rmw42 (Migrated from github.com) reviewed

2018-10-08 13:57:54 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +136,4 @@

				               err = CRYPT_INVALID_PACKET;

				               goto error;

				            } else {

				               if ((err = mp_read_unsigned_bin(vdata, (unsigned char *)in, size)) != CRYPT_OK)          { goto error; }

Good catch. Done.

rmw42 (Migrated from github.com) reviewed

2018-10-08 14:00:31 -05:00

src/misc/ssh/ssh_decode_sequence_multi.c

					
				@ -0,0 +123,4 @@

				               XSTRNCPY(sdata, (const char *)in, size);

				               sdata[size] = '\0'; /* strncpy doesn't NUL-terminate */

				            } else {

				               *sdata = '\0';

Moved the size >= bufsize check up, out of the if (size > 0) case. That way if the string is 0 bytes long - an empty string - we still need a buffer of at least 1 byte to output a C-style NUL-terminated string.

Moved the `size >= bufsize` check up, out of the `if (size > 0)` case. That way if the string is 0 bytes long - an empty string - we still need a buffer of at least 1 byte to output a C-style NUL-terminated string.

rmw42 commented

2018-10-08 14:07:42 -05:00

(Migrated from github.com)

Unrelated to the patch, I'm curious where SSH signature encoding is being used externally to the SSH protocol?

I don't think it is used anywhere else... @karel-m asked for this so you could avoid having to transform another signature format in Dropbear.

> Unrelated to the patch, I'm curious where SSH signature encoding is being used externally to the SSH protocol? I don't think it is used anywhere else... @karel-m asked for this so you could avoid having to transform another signature format in Dropbear.

karel-m commented

2018-10-09 02:53:39 -05:00

(Migrated from github.com)

@mkj the idea was mine. During my (unfinished) work - github.com/karel-m/dropbear@4530ff6897 - on a PR adopting Dropbear to the new libtomcrypt ecc_* stuff I saw the dark magic in dropbear's ecc.c related to ECDSA signatures which lead me to the idea that libtomcrypt should do a better job supporting our "prominent customer" (= dropbear).

@mkj the idea was mine. During my (unfinished) work - https://github.com/karel-m/dropbear/commit/4530ff68975932680d674a33ea477fa7afc79ade - on a PR adopting Dropbear to the new libtomcrypt ecc_* stuff I saw the dark magic in dropbear's `ecc.c` related to ECDSA signatures which lead me to the idea that libtomcrypt should do a better job supporting our "prominent customer" (= dropbear).

sjaeckel (Migrated from github.com) approved these changes

2018-10-09 06:00:16 -05:00

mkj commented

2018-10-09 08:43:06 -05:00

(Migrated from github.com)

Ah right. I'm not sure it really makes sense for libtomcrypt to be parsing SSH packets when it's alongside another parser anyway.

I think it would work fine if there were just a

int ecc_verify_hash_raw(void *r, void *s,
    const unsigned char *hash, unsigned long hashlen,
    int *stat, const ecc_key *key)

etc which could be called? Dropbear needs to do some work outside anyway to know which hash_desc to use.

Currently Dropbear's bundled libtomcrypt skips LTC_DER since it isn't necessary (I just treat them as opaque blobs in a few places) - it avoids some code size. It would be nice to avoid having to pull that in too, maybe the dropbear_ecc_curve.oid in the new branch should just be a long array to memcmp()?

Ah right. I'm not sure it really makes sense for libtomcrypt to be parsing SSH packets when it's alongside another parser anyway. I think it would work fine if there were just a ``` int ecc_verify_hash_raw(void *r, void *s, const unsigned char *hash, unsigned long hashlen, int *stat, const ecc_key *key) ``` etc which could be called? Dropbear needs to do some work outside anyway to know which hash_desc to use. Currently Dropbear's bundled libtomcrypt skips LTC_DER since it isn't necessary (I just treat them as opaque blobs in a few places) - it avoids some code size. It would be nice to avoid having to pull that in too, maybe the `dropbear_ecc_curve.oid` in the new branch should just be a long array to memcmp()?

sjaeckel commented

2018-10-09 16:15:22 -05:00

(Migrated from github.com)

maybe the dropbear_ecc_curve.oid in the new branch should just be a long array to memcmp()?

The plan is to make all OID appearances per default string-based (providing still the possibility to use the ulong-array approach to de-/encode via the DER API). Would that be fine for you as well?

> maybe the `dropbear_ecc_curve.oid` in the new branch should just be a long array to memcmp()? The plan is to make all OID appearances per default string-based (providing still the possibility to use the ulong-array approach to de-/encode via the DER API). Would that be fine for you as well?

rmw42 commented

2018-10-12 04:23:59 -05:00

(Migrated from github.com)

Rebased to current develop branch

karel-m commented

2018-10-13 07:38:36 -05:00

(Migrated from github.com)

Ad @mkj 's comments

I think it would work fine if there were just a int ecc_verify_hash_raw(void *r, void *s, ...)

I do not like this. This will expose internal libtomcrypt guts (pointers to bignum structures) which we do not have yet in any public API function. Plus there is of course a completely new bunch of troubles related to the fact that math provider is not always libtommath.

Currently Dropbear's bundled libtomcrypt skips LTC_DER since it isn't necessary...

This is a valid point. The current all-in-one ecc_sign_hash_ex and ecc_verify_hash_ex forces anyone using ECDSA to link the whole ASN.1 stuff to their binary even if they are using just RFC 7518 or RFC 5656 (ssh2) signature format. IMO it is worth rethinking in a separate PR.

@sjaeckel can we merge this PR? (to me it looks good)

Ad @mkj 's comments > I think it would work fine if there were just a `int ecc_verify_hash_raw(void *r, void *s, ...)` I do not like this. This will expose internal libtomcrypt guts (pointers to bignum structures) which we do not have yet in any public API function. Plus there is of course a completely new bunch of troubles related to the fact that math provider is not always libtommath. > Currently Dropbear's bundled libtomcrypt skips LTC_DER since it isn't necessary... This is a valid point. The current all-in-one `ecc_sign_hash_ex` and `ecc_verify_hash_ex` forces anyone using ECDSA to link the whole ASN.1 stuff to their binary even if they are using just RFC 7518 or RFC 5656 (ssh2) signature format. IMO it is worth rethinking in a separate PR. @sjaeckel can we merge this PR? (to me it looks good)

rmw42 commented

2018-10-13 07:45:37 -05:00

(Migrated from github.com)

The current all-in-one ecc_sign_hash_ex and ecc_verify_hash_ex forces anyone using ECDSA to link the whole ASN.1 stuff to their binary

We could put that branch of the conditional on #ifdef LTC_DER so LTC_ECCSIG_ANSIX962 is only available as a signature format if the library is built with DER support and returns an error otherwise.

> The current all-in-one `ecc_sign_hash_ex` and `ecc_verify_hash_ex` forces anyone using ECDSA to link the whole ASN.1 stuff to their binary We could put that branch of the conditional on `#ifdef LTC_DER` so `LTC_ECCSIG_ANSIX962` is only available as a signature format if the library is built with DER support and returns an error otherwise.

karel-m commented

2018-10-13 07:57:59 -05:00

(Migrated from github.com)

This will not handle a static linking scenario when you have a static library built with all ASN.1/DER support but you want to just use RFC 7518 ECDSA signatures. I think we should somehow split ecc_sign_hash_ex.c and ecc_verify_hash_ex.c into more .c files.

This will not handle a static linking scenario when you have a static library built with all ASN.1/DER support but you want to just use RFC 7518 ECDSA signatures. I think we should somehow split `ecc_sign_hash_ex.c` and `ecc_verify_hash_ex.c` into more `.c` files.

rmw42 commented

2018-10-14 02:36:03 -05:00

(Migrated from github.com)

Cheers! 👍

Sign in to join this conversation.

No reviewers

karel-m

sjaeckel

No labels

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-libtom/libtomcrypt!438

				`@ -0,0 +1,70 @@`
				`/* LibTomCrypt, modular cryptographic library -- Tom St Denis`

				`@ -0,0 +1,161 @@`
				`/* LibTomCrypt, modular cryptographic library -- Tom St Denis`

				`@ -0,0 +1,310 @@`
				`/* LibTomCrypt, modular cryptographic library -- Tom St Denis`

				`@ -154,6 +154,23 @@ int padding_pad(unsigned char data, unsigned long length, unsigned long padded`
				`int padding_depad(const unsigned char data, unsigned long length, unsigned long mode);`

				`@ -251,7 +251,9 @@ typedef enum ecc_signature_type_ {`
				`/* raw R, S values */`