fireblocks / mpc-lib

GNU General Public License v3.0
147 stars 86 forks source link

`is_coprime_fast` check can be skipped due to OpenSSL allocation failure #10

Closed tjade273 closed 7 months ago

tjade273 commented 7 months ago

Upon failure of BN_get or BN_copy, is_coprime_fast returns -1. These can fail if OpenSSL fails to allocate the necessary memory (e.g. due to OOM conditions on the OS).

https://github.com/fireblocks/mpc-lib/blob/84b7fb83502a998703b6ceb113273fc20fa55b7b/src/common/crypto/paillier/paillier.c#L9-L18

https://github.com/fireblocks/mpc-lib/blob/84b7fb83502a998703b6ceb113273fc20fa55b7b/src/common/crypto/paillier/paillier.c#L40-L43

Many instances throughout the code use a if(!is_coprime_fast(...)) pattern, which will succeed even if the inputs are not in fact coprime when the OOM condition mentioned above is hit. Some (nonexhaustive) examples:

https://github.com/fireblocks/mpc-lib/blob/84b7fb83502a998703b6ceb113273fc20fa55b7b/src/common/cosigner/mta.cpp#L825 https://github.com/fireblocks/mpc-lib/blob/84b7fb83502a998703b6ceb113273fc20fa55b7b/src/common/crypto/zero_knowledge_proof/range_proofs.c#L450

https://github.com/fireblocks/mpc-lib/blob/84b7fb83502a998703b6ceb113273fc20fa55b7b/src/common/crypto/paillier/paillier_zkp.c#L432-L436

While these conditions are difficult to trigger in practice, in theory skipping some of these checks may lead to key disclosure.

The most robust solution is to simply panic immediately if these allocations occur. Alternatively, returning 0 in these cases rather than -1 will prevent introduction of similar problematic callsites in the future.

nadav-fireblocks commented 7 months ago

Thanks, as you said we also don't think it is relevant in real world scenario (in any real world scenario the app will crash on OOM before it's exploitable). In any case, we fixed it.