re-work PK crypto im- & export #302

sjaeckel · 2017-09-29T16:28:58Z

well this turned out to become a bigger one with some real bugfixes as well...

This mostly removes MAX_RSA_SIZE from the code, only rsa_make_key() still checks for it.

After some thinking I'd even propose to remove MIN_RSA_SIZEand MAX_RSA_SIZE entirely! Somebody against it?

buggywhip · 2017-09-29T18:26:39Z

On 29Sep, 2017, at 9:29 AM, Steffen Jaeckel ***@***.***> wrote: After some thinking I'd even propose to remove MIN_RSA_SIZEand MAX_RSA_SIZE entirely! Somebody against it?

Not really, but note that it is currently set to 4096. Given recent interest in post-quantum resistance with longer key lengths, folks may want to play with 8192 and 15360. ...but given space and time, not for long. ;-) That said, checking inputs for reasonableness is not a bad idea. The question then becomes what should be the limit? Everybody will have a different opinion but I consider 8192 another "reasonable" number. (A higher or lower number can always set it at compile time.)

…

----- Also on the subject of key lengths, a few lines further down in tomcrypt_custom.h, LTC_DER_MAX_PUBKEY_SIZE is set to 1024. FIPS.186-4 now defines longer DSA key lengths. http://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-4.pdf This might cause DSA key import problems in pk/dsa/dsa_import.c

karel-m · 2017-09-29T19:08:07Z

I have doubts about `der_decode_raw_bit_string`: 1/ Ad `blen/8 > *outlen` - what if `blen` is not multiple of 8? And the part `*outlen = blen;` perhaps also needs a fix. 2/ To me it seems like we are (falsely) assuming that `out[]` is zero-initialized, we are setting just 1-bits.

sjaeckel · 2017-09-29T23:32:28Z

Not really, but note that it is currently set to 4096. Given recent interest in post-quantum resistance with longer key lengths, folks may want to play with 8192 and 15360. ...but given space and time, not for long. ;-)

yeah, I would basically just remove the limits, so someone could create arbitrary sized RSA keys

That said, checking inputs for reasonableness is not a bad idea. The question then becomes what should be the limit? Everybody will have a different opinion but I consider 8192 another "reasonable" number. (A higher or lower number can always set it at compile time.)

I also think that sanity checking is useful at one point but I'm not sure if this library is the correct point where someone should hit an error because he chose an insane (from the library's view) value.

sjaeckel · 2017-09-29T23:48:26Z

probably keeping MIN_RSA_SIZE but increasing it to 2048 is a good idea

karel-m · 2017-09-30T05:14:17Z

Ad (MAX|MIN)_RSA_SIZE I am for removing them completely. Looking at der_decode_raw_bit_string once again: outlen [in/out] The number of *bits* stored So perhaps the original comparison with just `blen` was correct.

rofl0r · 2017-09-30T10:19:45Z

probably keeping MIN_RSA_SIZE but increasing it to 2048 is a good idea

wouldn't that prevent usage of 1024 bit keys ? i can imagine there are still some users of such keys, which may not have the option to change them.

sjaeckel · 2017-09-30T11:01:43Z

wouldn't that prevent usage of 1024 bit keys ? i can imagine there are still some users of such keys, which may not have the option to change them.

well it would prevent the creation of fresh 1024 bit keys but importing would still be feasible.

sjaeckel · 2017-09-30T11:04:24Z

So perhaps the original comparison with just blen was correct.

yeah you're right... now the API is completely inconsistent..

karel-m · 2017-09-30T11:13:54Z

>> wouldn't that prevent usage of 1024 bit keys ? i can imagine there are >> still some users of such keys, which may not have the option to change them. > > well it would prevent the creation of fresh 1024 bit keys but importing > would still be feasible.

We do not have an ambition to prevent ltc users from weak crypo at many other places. The recommended minimum should be mentioned with a loud warning in the doc, not compiled into the library. Enforcing min/max limits make sense to me only if those are limits of our RSA make key implementation.

rswindell · 2017-10-01T02:03:55Z

For what it's worth, rsa_import() is now working in a (very) memory-constrained environment. Thanks for all your fixes!

karel-m · 2017-10-01T12:21:41Z

According to my testing this PR introduces the following valgrind warnings:

==2355== 256 bytes in 1 blocks are indirectly lost in loss record 1 of 13
==2355==    at 0x4C2BBAF: malloc (vg_replace_malloc.c:299)
==2355==    by 0x1C3688: mp_init (in /home/noroot/libtomcrypt/test)
==2355==    by 0x18B069: init (ltm_desc.c:53)
==2355==    by 0x18C421: ltc_init_multi (multi.c:22)
==2355==    by 0x1A6370: rsa_make_key (rsa_make_key.c:70)
==2355==    by 0x126E7B: _rsa_issue_301 (rsa_test.c:319)
==2355==    by 0x12719A: rsa_test (rsa_test.c:368)
==2355==    by 0x1295F8: main (test.c:392)

==2355== 768 bytes in 1 blocks are indirectly lost in loss record 3 of 13
==2355==    at 0x4C2DDCF: realloc (vg_replace_malloc.c:785)
==2355==    by 0x1C35AC: mp_grow (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C74BF: s_mp_add (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C5CC3: mp_sub (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C8A3C: fast_mp_invmod (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C3838: mp_invmod (in /home/noroot/libtomcrypt/test)
==2355==    by 0x18C001: invmod (ltm_desc.c:350)
==2355==    by 0x1A64CB: rsa_make_key (rsa_make_key.c:84)
==2355==    by 0x126E7B: _rsa_issue_301 (rsa_test.c:319)
==2355==    by 0x12719A: rsa_test (rsa_test.c:368)
==2355==    by 0x1295F8: main (test.c:392)

==2355== 768 bytes in 1 blocks are indirectly lost in loss record 4 of 13
==2355==    at 0x4C2DDCF: realloc (vg_replace_malloc.c:785)
==2355==    by 0x1C35AC: mp_grow (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C2453: mp_copy (in /home/noroot/libtomcrypt/test)
==2355==    by 0x18B19E: copy (ltm_desc.c:77)
==2355==    by 0x1A64EF: rsa_make_key (rsa_make_key.c:86)
==2355==    by 0x126E7B: _rsa_issue_301 (rsa_test.c:319)
==2355==    by 0x12719A: rsa_test (rsa_test.c:368)
==2355==    by 0x1295F8: main (test.c:392)

==2355== 768 bytes in 1 blocks are indirectly lost in loss record 5 of 13
==2355==    at 0x4C2DDCF: realloc (vg_replace_malloc.c:785)
==2355==    by 0x1C35AC: mp_grow (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C2453: mp_copy (in /home/noroot/libtomcrypt/test)
==2355==    by 0x18B19E: copy (ltm_desc.c:77)
==2355==    by 0x1A6513: rsa_make_key (rsa_make_key.c:87)
==2355==    by 0x126E7B: _rsa_issue_301 (rsa_test.c:319)
==2355==    by 0x12719A: rsa_test (rsa_test.c:368)
==2355==    by 0x1295F8: main (test.c:392)

==2355== 1,024 bytes in 1 blocks are indirectly lost in loss record 7 of 13
==2355==    at 0x4C2DDCF: realloc (vg_replace_malloc.c:785)
==2355==    by 0x1C35AC: mp_grow (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C74BF: s_mp_add (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C2046: mp_add (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C3BE4: mp_invmod_slow (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C3851: mp_invmod (in /home/noroot/libtomcrypt/test)
==2355==    by 0x18C001: invmod (ltm_desc.c:350)
==2355==    by 0x1A63C9: rsa_make_key (rsa_make_key.c:75)
==2355==    by 0x126E7B: _rsa_issue_301 (rsa_test.c:319)
==2355==    by 0x12719A: rsa_test (rsa_test.c:368)
==2355==    by 0x1295F8: main (test.c:392)

==2355== 1,024 bytes in 1 blocks are indirectly lost in loss record 8 of 13
==2355==    at 0x4C2DDCF: realloc (vg_replace_malloc.c:785)
==2355==    by 0x1C35AC: mp_grow (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C8FF9: fast_s_mp_mul_digs (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C4DCE: mp_mul (in /home/noroot/libtomcrypt/test)
==2355==    by 0x18B951: mul (ltm_desc.c:242)
==2355==    by 0x1A63F2: rsa_make_key (rsa_make_key.c:76)
==2355==    by 0x126E7B: _rsa_issue_301 (rsa_test.c:319)
==2355==    by 0x12719A: rsa_test (rsa_test.c:368)
==2355==    by 0x1295F8: main (test.c:392)

==2355== 1,024 bytes in 1 blocks are indirectly lost in loss record 9 of 13
==2355==    at 0x4C2BBAF: malloc (vg_replace_malloc.c:299)
==2355==    by 0x1C3737: mp_init_size (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C3637: mp_init_copy (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C27FA: mp_div (in /home/noroot/libtomcrypt/test)
==2355==    by 0x18BAA4: divide (ltm_desc.c:265)
==2355==    by 0x1A6470: rsa_make_key (rsa_make_key.c:82)
==2355==    by 0x126E7B: _rsa_issue_301 (rsa_test.c:319)
==2355==    by 0x12719A: rsa_test (rsa_test.c:368)
==2355==    by 0x1295F8: main (test.c:392)

==2355== 1,024 bytes in 1 blocks are indirectly lost in loss record 10 of 13
==2355==    at 0x4C2BBAF: malloc (vg_replace_malloc.c:299)
==2355==    by 0x1C3737: mp_init_size (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C3637: mp_init_copy (in /home/noroot/libtomcrypt/test)
==2355==    by 0x1C27FA: mp_div (in /home/noroot/libtomcrypt/test)
==2355==    by 0x18BAA4: divide (ltm_desc.c:265)
==2355==    by 0x1A64A2: rsa_make_key (rsa_make_key.c:83)
==2355==    by 0x126E7B: _rsa_issue_301 (rsa_test.c:319)
==2355==    by 0x12719A: rsa_test (rsa_test.c:368)
==2355==    by 0x1295F8: main (test.c:392)

==2355== 6,848 (192 direct, 6,656 indirect) bytes in 8 blocks are definitely lost in loss record 13 of 13
==2355==    at 0x4C2DBC5: calloc (vg_replace_malloc.c:711)
==2355==    by 0x18B03D: init (ltm_desc.c:48)
==2355==    by 0x18C421: ltc_init_multi (multi.c:22)
==2355==    by 0x1A6370: rsa_make_key (rsa_make_key.c:70)
==2355==    by 0x126E7B: _rsa_issue_301 (rsa_test.c:319)
==2355==    by 0x12719A: rsa_test (rsa_test.c:368)
==2355==    by 0x1295F8: main (test.c:392)

The remaining 4 are DSA related and are unfortunately already present in release/1.18.0.

Steps to reproduce:

make -f makefile.unix clean
make -f makefile.unix CFLAGS="-g -DUSE_LTM -DLTM_DESC -I../libtommath" EXTRALIBS="../libtommath/libtommath.a" test
valgrind --error-exitcode=666 --leak-check=full --show-leak-kinds=all --errors-for-leak-kinds=all ./test

karel-m · 2017-10-01T12:43:29Z

tests/rsa_test.c

+static int _rsa_issue_301(int prng_idx)
+{
+   rsa_key       key, key_in;
+   unsigned char buf[4096];


approx buf[2500] should be enough here

karel-m · 2017-10-01T12:44:00Z

tests/rsa_test.c

+   unsigned char buf[4096];
+   unsigned long len;
+
+   DO(rsa_make_key(&yarrow_prng, prng_idx, sizeof(buf)/8, 65537, &key));


4096/8 instead of sizeof(buf)/8 would be IMO better

karel-m · 2017-10-01T12:45:39Z

tests/rsa_test.c

@@ -308,6 +365,10 @@ int rsa_test(void)
      return 1;
   }

+   if (_rsa_issue_301(prng_idx) != 0) {


what about DO(_rsa_issue_301(prng_idx))?

karel-m · 2017-10-01T12:47:21Z

src/pk/rsa/rsa_make_key.c

@@ -32,10 +32,6 @@ int rsa_make_key(prng_state *prng, int wprng, int size, long e, rsa_key *key)
   LTC_ARGCHK(ltc_mp.name != NULL);
   LTC_ARGCHK(key         != NULL);

-   if ((size < (MIN_RSA_SIZE/8)) || (size > (MAX_RSA_SIZE/8))) {


considering the fact that size is int we should at least check LTC_ARGCHK(size > 0)

karel-m · 2017-10-01T12:48:27Z

tests/rsa_test.c

+   rsa_free(&key_in);
+
+   rsa_free(&key);
+   return 0;


what about return CRYPT_OK instead of return 0

Bug-fix: MAX_RSA_SIZE is the maximum RSA key size in *bits* (as commented in tomcrypt_custom.h), so the proper conversion to bytes (as the argument value to XCALLOC) would be to divide by 8 (bits per byte), not multiply by 8. This excessive allocation (32 Kbytes instead of 512 bytes) is readily apparent in memory-constrained environments.

The ASN1 encoded RSA key contains two MPI's therefore MAX_RSA_SIZE / 8 isn't enough.

[skip ci]

This reverts commit 548ee34.

This reverts commit 2b8d83f.

karel-m

👍

sjaeckel mentioned this pull request Sep 29, 2017

Fixes large heap over-allocation in rsa_import #301

Closed

sjaeckel requested review from karel-m and buggywhip September 29, 2017 16:29

sjaeckel force-pushed the fix/pr/301 branch from 56cbb84 to 130d657 Compare September 30, 2017 11:44

karel-m reviewed Oct 1, 2017

View reviewed changes

tests/rsa_test.c Outdated

rsa_free(&key_in);

rsa_free(&key);

return 0;

Copy link

Member

karel-m Oct 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about return CRYPT_OK instead of return 0

rswindell and others added 10 commits October 1, 2017 22:56

print info when doing a debug build

062fc3f

fix bit-length check in der_decode_raw_bit_string()

0b04279

don't over-allocate that much in der_decode_subject_public_key_info()

15eab97

fixup #290

efa089e

add tests for MAX_RSA_SIZE sized openssl-standard RSA keys

0500aae

fix rsa_import() of MAX_RSA_SIZE'ed keys

ab02d2e

The ASN1 encoded RSA key contains two MPI's therefore MAX_RSA_SIZE / 8 isn't enough.

mostly remove MAX_RSA_SIZE

9cfbaa8

update README

1d20c32

[skip ci]

also clear bits in der_decode_raw_bit_string()

3fef07c

sjaeckel and others added 9 commits October 1, 2017 22:56

catch case where blen%8 != 0

40e4a66

remove {MIN,MAX}_RSA_SIZE

30b3a9a

format code

d1d9566

Revert "catch case where blen%8 != 0"

e5de0a0

This reverts commit 548ee34.

Revert "fix bit-length check in der_decode_raw_bit_string()"

db7d7a8

This reverts commit 2b8d83f.

correctly fix decoding of SubjectPublicKeyInfo

ce7ae84

missing rsa_free in _rsa_issue_301 test

67f9064

improve rsa_test a bit

c702ac6

make sure size is valid

7f302da

sjaeckel force-pushed the fix/pr/301 branch from 886b04c to 7f302da Compare October 1, 2017 23:19

karel-m approved these changes Oct 2, 2017

View reviewed changes

sjaeckel merged commit 01a61e6 into release/1.18.0 Oct 2, 2017

sjaeckel deleted the fix/pr/301 branch October 2, 2017 12:51

sjaeckel added this to the v1.18.0 milestone Oct 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

re-work PK crypto im- & export #302

re-work PK crypto im- & export #302

sjaeckel commented Sep 29, 2017

buggywhip commented Sep 29, 2017 via email

karel-m commented Sep 29, 2017 via email

sjaeckel commented Sep 29, 2017

sjaeckel commented Sep 29, 2017

karel-m commented Sep 30, 2017 via email

rofl0r commented Sep 30, 2017

sjaeckel commented Sep 30, 2017

sjaeckel commented Sep 30, 2017

karel-m commented Sep 30, 2017 via email •

edited

Loading

rswindell commented Oct 1, 2017

karel-m commented Oct 1, 2017

karel-m Oct 1, 2017

karel-m Oct 1, 2017

karel-m Oct 1, 2017

karel-m Oct 1, 2017

karel-m Oct 1, 2017

karel-m left a comment

re-work PK crypto im- & export #302

re-work PK crypto im- & export #302

Conversation

sjaeckel commented Sep 29, 2017

buggywhip commented Sep 29, 2017 via email

karel-m commented Sep 29, 2017 via email

sjaeckel commented Sep 29, 2017

sjaeckel commented Sep 29, 2017

karel-m commented Sep 30, 2017 via email

rofl0r commented Sep 30, 2017

sjaeckel commented Sep 30, 2017

sjaeckel commented Sep 30, 2017

karel-m commented Sep 30, 2017 via email • edited Loading

rswindell commented Oct 1, 2017

karel-m commented Oct 1, 2017

karel-m Oct 1, 2017

Choose a reason for hiding this comment

karel-m Oct 1, 2017

Choose a reason for hiding this comment

karel-m Oct 1, 2017

Choose a reason for hiding this comment

karel-m Oct 1, 2017

Choose a reason for hiding this comment

karel-m Oct 1, 2017

Choose a reason for hiding this comment

karel-m left a comment

Choose a reason for hiding this comment

karel-m commented Sep 30, 2017 via email •

edited

Loading