Fixes large heap over-allocation in rsa_import

rswindell commented

2017-09-27 19:27:44 -05:00

(Migrated from github.com)

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 of heap memory (32 Kbytes instead of 512 bytes) is readily apparent in memory-constrained environments.

👍 2

mattkelly commented

2017-09-27 20:30:24 -05:00

(Migrated from github.com)

Hah, nice find!

sjaeckel commented

2017-09-28 06:08:30 -05:00

(Migrated from github.com)

especially since der_decode_subject_public_key_info() will also allocate 32k ;)

Edit: ...which is also nonsense IIUC

especially since `der_decode_subject_public_key_info()` will also allocate 32k ;) Edit: ...which is also nonsense IIUC

sjaeckel commented

2017-09-28 06:12:38 -05:00

(Migrated from github.com)

LTC_DER_MAX_PUBKEY_SIZE isn't enough in der_decode_subject_public_key_info(), but at least we shouldn't need LTC_DER_MAX_PUBKEY_SIZE * 8... there's probably a way to find out what we really need to allocate...

`LTC_DER_MAX_PUBKEY_SIZE` isn't enough in `der_decode_subject_public_key_info()`, but at least we shouldn't need `LTC_DER_MAX_PUBKEY_SIZE * 8`... there's probably a way to find out what we really need to allocate...

sjaeckel commented

2017-09-28 06:13:35 -05:00

(Migrated from github.com)

there's probably a way to find out what we really need to allocate

I guess the easiest would be to use inlen

> there's probably a way to find out what we really need to allocate I guess the easiest would be to use `inlen`

karel-m commented

2017-09-28 07:19:35 -05:00

(Migrated from github.com)

Could you please add a test case to tests/rsa_test.c which will make a key
of size MAX_RSA_SIZE/8 and then export-priv, import-priv, export-pub,
import-pub.

Could you please add a test case to tests/rsa_test.c which will make a key of size MAX_RSA_SIZE/8 and then export-priv, import-priv, export-pub, import-pub.

sjaeckel commented

2017-09-28 09:00:17 -05:00

(Migrated from github.com)

export-priv, import-priv, export-pub, import-pub

+ export-pub&PK_STD, import-pub

> export-priv, import-priv, export-pub, import-pub \+ export-pub&PK_STD, import-pub

karel-m commented

2017-09-28 09:11:14 -05:00

(Migrated from github.com)

As for the actual fix, Steffen's inlen idea is IMO better e.g.

tmpbuf = XMALLOC(inlen);

As for the actual fix, Steffen's inlen idea is IMO better e.g. tmpbuf = XMALLOC(inlen);

sjaeckel commented

2017-09-28 10:32:56 -05:00

(Migrated from github.com)

Btw I tried it out and needs some more tweaking! MALLOC(inlen) isn't enough, that fails

Btw I tried it out and needs some more tweaking! `MALLOC(inlen)` isn't enough, that fails

sjaeckel commented

2017-09-29 03:31:32 -05:00

(Migrated from github.com)

I've pushed the fix to the fix/pr/301 branch, tests still missing.

I've pushed the fix to the [fix/pr/301](https://github.com/libtom/libtomcrypt/tree/fix/pr/301) branch, tests still missing.

sjaeckel commented

2017-09-29 11:29:25 -05:00

(Migrated from github.com)

Closed in favor of #302

Pull request closed

Please reopen this pull request to perform a merge.

Rows
Columns

Fixes large heap over-allocation in rsa_import #301

Pull request closed