LTC_ECCSIG_RFC5656 SSH+ECDSA signature format #438

rmw42 · 2018-10-02T21:22:21Z

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

rmw42 · 2018-10-03T07:25:45Z

@karel-m As promised :)

src/headers/tomcrypt_pk.h

src/misc/ssh/ssh_test.c

src/pk/ecc/ecc_cmp_oid_str.c

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c

karel-m · 2018-10-03T10:18:11Z

Great job!

karel-m · 2018-10-03T10:26:40Z

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)
     ^~~~~~~~~~~~~~~~~~

src/misc/ssh/ssh_test.c

src/pk/ecc/ecc_cmp_oid_str.c

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c

rmw42 · 2018-10-03T18:28:07Z

Great job!

Cheers! :)

rmw42 · 2018-10-03T19:35:46Z

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

karel-m · 2018-10-03T21:24:36Z

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 · 2018-10-03T22:29:37Z

clang-tidy is without warnings 👍

rmw42 · 2018-10-04T07:12:19Z

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

Looks good!

👍

doc/crypt.tex

src/headers/tomcrypt_misc.h

src/misc/ssh/ssh_decode_sequence_multi.c

tests/ecc_test.c

tests/ssh_test.c

src/misc/ssh/ssh_decode_sequence_multi.c

rmw42 · 2018-10-04T18:19:39Z

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).

helper.pl

src/misc/ssh/ssh_decode_sequence_multi.c

rmw42 · 2018-10-05T09:49:11Z

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 · 2018-10-05T09:55:18Z

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 😄

sjaeckel

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

src/headers/tomcrypt_pk.h

sjaeckel · 2018-10-06T11:25:40Z

src/misc/ssh/ssh_decode_sequence_multi.c

+      }
+
+      /* Check we've not read too far */
+      if (in > sentinel) {


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

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.

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?

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...

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.

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;

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...

How's the triple-switch version?

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

src/misc/ssh/ssh_encode_sequence_multi.c

src/pk/ecc/ecc_cmp_oid_str.c

tests/common.h

rmw42 · 2018-10-08T08:00:28Z

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 · 2018-10-08T09:34:04Z

@mkj did you have a look at this?

mkj

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

rmw42 · 2018-10-08T19:07:42Z

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 · 2018-10-09T07:53:39Z

@mkj the idea was mine. During my (unfinished) work - karel-m/dropbear@4530ff6 - 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 · 2018-10-09T13:43:06Z

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 · 2018-10-09T21:15:22Z

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?

Wrap signature format in #ifdef LTC_SSH Update docs Code review fixes Replace strcmp/memcmp with XSTRCMP/XMEMCMP for check-source Fix for check-defines XSTRCMP/XMEMCMP != 0 GCC7.3 wants only literal strings for sprintf format Code review changes Rework SSH decoding and tests Fix encoding and tests COMPARE_TESTVECTOR macro Single return point in ssh_decode_sequence_multi Actually use XSTRNCPY rather than just defining it More code review fixes Code review tweaks Ensure it's not possible to read past buffer end Keep track of size remaining, not end pointer

rmw42 · 2018-10-12T09:23:59Z

Rebased to current develop branch

karel-m · 2018-10-13T12:38:36Z

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 · 2018-10-13T12:45:37Z

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 · 2018-10-13T12:57:59Z

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 · 2018-10-14T07:36:03Z

Cheers! 👍

karel-m requested changes Oct 3, 2018

View reviewed changes

karel-m reviewed Oct 3, 2018

View reviewed changes

src/misc/ssh/ssh_test.c Outdated Show resolved Hide resolved

karel-m reviewed Oct 3, 2018

View reviewed changes

src/misc/ssh/ssh_test.c Outdated Show resolved Hide resolved

karel-m reviewed Oct 3, 2018

View reviewed changes

src/misc/ssh/ssh_test.c Outdated Show resolved Hide resolved

karel-m reviewed Oct 3, 2018

View reviewed changes

src/pk/ecc/ecc_cmp_oid_str.c Outdated Show resolved Hide resolved

karel-m reviewed Oct 3, 2018

View reviewed changes

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c Outdated Show resolved Hide resolved

rmw42 force-pushed the feature/ssh-ecdsa branch from fd83e8f to 1d64c68 Compare October 3, 2018 19:23

sjaeckel requested changes Oct 4, 2018

View reviewed changes

sjaeckel reviewed Oct 4, 2018

View reviewed changes

helper.pl Show resolved Hide resolved

rmw42 force-pushed the feature/ssh-ecdsa branch 2 times, most recently from 6d7945d to 6d0d58c Compare October 4, 2018 21:19

sjaeckel reviewed Oct 4, 2018

View reviewed changes

src/misc/ssh/ssh_decode_sequence_multi.c Outdated Show resolved Hide resolved

rmw42 force-pushed the feature/ssh-ecdsa branch 2 times, most recently from 1522c69 to 80ff52a Compare October 5, 2018 09:51

rmw42 force-pushed the feature/ssh-ecdsa branch from 80ff52a to c64e4f4 Compare October 5, 2018 10:39

karel-m approved these changes Oct 6, 2018

View reviewed changes

karel-m mentioned this pull request Oct 6, 2018

Improved pkcs8 support #404

Merged

4 tasks

sjaeckel reviewed Oct 6, 2018

View reviewed changes

rmw42 force-pushed the feature/ssh-ecdsa branch from 617993f to e3ec46d Compare October 8, 2018 07:59

mkj reviewed Oct 8, 2018

View reviewed changes

src/misc/ssh/ssh_decode_sequence_multi.c Show resolved Hide resolved

src/misc/ssh/ssh_decode_sequence_multi.c Show resolved Hide resolved

sjaeckel approved these changes Oct 9, 2018

View reviewed changes

rmw42 added 4 commits October 12, 2018 10:22

Update makefiles

9706aa6

Fixes suggested by Matt Johnston

7c4d7cb

GCC 4.8 -Wmaybe-uninitialized is *really* dumb

4ee5bfc

rmw42 force-pushed the feature/ssh-ecdsa branch from 05cdf36 to 4ee5bfc Compare October 12, 2018 09:23

karel-m mentioned this pull request Oct 13, 2018

ecc_sign_hash_ex + ecc_verify_hash_ex forces linking DER/ASN.1 code #449

Closed

karel-m merged commit dec99ed into libtom:develop Oct 13, 2018

rmw42 deleted the feature/ssh-ecdsa branch October 14, 2018 07:36

LTC_ECCSIG_RFC5656 SSH+ECDSA signature format #438

LTC_ECCSIG_RFC5656 SSH+ECDSA signature format #438

Uh oh!

Conversation

rmw42 commented Oct 2, 2018

Checklist

Uh oh!

rmw42 commented Oct 3, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

karel-m commented Oct 3, 2018

Uh oh!

karel-m commented Oct 3, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rmw42 commented Oct 3, 2018

Uh oh!

rmw42 commented Oct 3, 2018

Uh oh!

karel-m commented Oct 3, 2018

Uh oh!

karel-m commented Oct 3, 2018

Uh oh!

rmw42 commented Oct 4, 2018

Uh oh!

sjaeckel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rmw42 commented Oct 4, 2018

Uh oh!

Uh oh!

Uh oh!

rmw42 commented Oct 5, 2018

Uh oh!

rmw42 commented Oct 5, 2018

Uh oh!

sjaeckel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!