New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support multi-prime RSA (RFC 8017) #4241
Conversation
eb50bcf
to
c8ccaf8
Compare
Interesting, I wan't aware there was such a thing as multi-prime RSA. Wanted to read up on it, but unfortunately the reference given by the RFC (Silverman, R., "A Cost-Based Security Analysis of Symmetric and Asymmetric Key Lengths", RSA Laboratories, Bulletin No. 13, 2000) does not appear to be available anymore. Do you maybe have a copy? |
A Google cached page is available here: Google cached SILVERMAN And I think you would be interested in this article: https://www.imperialviolet.org/2011/04/09/multiprime.html |
crypto/rsa/rsa_gen.c
Outdated
e_value, cb); | ||
} | ||
|
||
int RSA_generate_multi_prime_key(RSA *rsa, int bits, int primes, BIGNUM *e_value, BN_GENCB *cb) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style : please stay inside 80 cols ;)
crypto/rsa/rsa_asn1.c
Outdated
} | ||
return 1; | ||
} | ||
|
||
/* use customized new and free operations */ | ||
static int rsa_prime_info_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style : 80 cols
crypto/rsa/rsa_ossl.c
Outdated
} | ||
|
||
/* | ||
* This will help stop the size of r0 increasing, which does affect the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: 80 cols, just move the last word to next line
crypto/rsa/rsa_gen.c
Outdated
} | ||
rsa->prime_infos = prime_infos; | ||
|
||
/* prime_info from 2 to prime -1 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... to |primes| - 1
test/rsa_mp_test.c
Outdated
|
||
static int key4096p7(RSA *key) | ||
{ | ||
/* C90 requires string shoudl <= 509 bytes */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: should
test/rsa_mp_test.c
Outdated
BN_bin2bn(iqmp, sizeof(iqmp) - 1, NULL)), 1)) | ||
return 0; | ||
|
||
pris = OPENSSL_zalloc(sizeof(BIGNUM *) * 5); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you #define NUM_TEST_KEYS 5 , so you will avoid repeating this magic number, and allocating a pseudo-dynamic array of pointer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, a macro is better. But 5
is not the number of keys, it's the number of extra primes. So I will use the name NUM_EXTRA_PRIMS 5
instead.
Note: if we take this up the ASN.1 side can be simplified considerably and possibly the accessors too. |
My understanding is that Intel has submitted multiple multi-prime RSA implementations, some of which have highly-optimized SIMD code. Cloudflare also has a highly-optimized implementation: https://github.com/vkrasnov/multiprime. |
This paper describes the previous Intel/Cloudflare contribution to OpenSSL and links to at least some of the implementations they contributed: Speed Records for Multi-prime RSA Using AVX2 Architectures. |
@briansmith , yes there are patches before (for 1.0.2 and 1.1.0 branches). I didn't look carefully into those patches, but I think the optimizations there are for some special cases (such as 3 or 4 primes, for some fixed length of each prime). This PR aims to provide a generic C implementation for the master branch, thus if OpenSSL 1.1.1 is released, one can get a usable patch immediately. This is just another attempt to include this feature into master of OpenSSL. |
ping @openssl for other comments on this... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this needs a note in CHANGES.
@@ -27,10 +27,40 @@ static int rsa_cb(int operation, ASN1_VALUE **pval, const ASN1_ITEM *it, | |||
RSA_free((RSA *)*pval); | |||
*pval = NULL; | |||
return 2; | |||
} else if (operation == ASN1_OP_D2I_POST) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment at the start of this function probably needs updating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@@ -108,6 +110,10 @@ int genrsa_main(int argc, char **argv) | |||
if (!opt_cipher(opt_unknown(), &enc)) | |||
goto end; | |||
break; | |||
case OPT_PRIMES: | |||
if (!opt_int(opt_arg(), &primes)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to validate this parameter to check it is sane?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's no need to do so, since the primes
variable will be checked in the following RSA_generate_multi_prime_key
function
|
||
/* prime_info from 2 to |primes| -1 */ | ||
for (i = 2; i < primes; i++) { | ||
pinfo = rsa_multip_info_new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the return is not NULL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly
crypto/rsa/rsa_gen.c
Outdated
goto err; | ||
} | ||
if (primes == RSA_DEFAULT_PRIME_NUM && i == 1) { | ||
if (BN_cmp(rsa->p, rsa->q) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why this is necessary - and why it is not necessary in the multi-prime case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used to make sure this: https://github.com/InfoHunter/openssl/blob/5b5d37804656e59c2ef8928c0f1005ed9e8c054a/crypto/rsa/rsa_ossl.c#L808-L813
And this is left for staying consistent with the original 2-primes logical, doing so for multi-prime would be a little more complex and unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat counter-intuitive placement of this condition. You don't quite expect it to see inside this loop. And it's not obvious that this is correct. Indeed, consider that you swapped rsa->p
and rsa->q
, then figured that prime
needs to be extended. Which of the two will be extended? I'm not saying that it's incorrect (haven't thought of it closely enough), I'm rather pointing out the question reader would struggle with. So if this is actually correct placement for this check, then a commentary would probably be appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recent discussion effectively answers the question why this is necessary, but it also shows that this is not specific to two-prime case. However, I still find placement of the condition in the loop counter-intuitive and somewhat inappropriate. Suggestion is it take it out of the loop. I.e. generate all the primes and perform comparison at line 236.
crypto/rsa/rsa_gen.c
Outdated
goto err; | ||
goto redo; | ||
} | ||
/* save product of primes for furhter use, for multi-prime only */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: further
crypto/rsa/rsa_mp.c
Outdated
goto err; | ||
if ((pinfo->t = BN_secure_new()) == NULL) | ||
goto err; | ||
if ((pinfo->pp = BN_new()) == NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BN_secure_new() not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errr, seems needed...
crypto/rsa/rsa_ossl.c
Outdated
@@ -636,6 +644,28 @@ static int rsa_ossl_mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) | |||
BN_free(q); | |||
goto err; | |||
} | |||
if (primes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use primes > 0
. The variable primes
is not a boolean.
crypto/rsa/rsa_ossl.c
Outdated
@@ -705,6 +735,48 @@ static int rsa_ossl_mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) | |||
BN_free(dmp1); | |||
} | |||
|
|||
/* calculate m_i in multi-prime case */ | |||
if (primes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
crypto/rsa/rsa_ossl.c
Outdated
@@ -747,6 +819,67 @@ static int rsa_ossl_mod_exp(BIGNUM *r0, const BIGNUM *I, RSA *rsa, BN_CTX *ctx) | |||
if (!BN_add(r0, r1, m1)) | |||
goto err; | |||
|
|||
/* add m_i to m in multi-prime case */ | |||
if (primes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
doc/man3/RSA_get0_key.pod
Outdated
RSA_set0_multi_prime_params() return 1 on success or 0 on failure. | ||
|
||
RSA_get_multi_prime_num() returns the number of other primes or 0 if there | ||
is no other primes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there are no other primes
crypto/rsa/rsa_asn1.c
Outdated
ASN1_SIMPLE(RSA_PRIME_INFO, d, CBIGNUM), | ||
ASN1_SIMPLE(RSA_PRIME_INFO, t, CBIGNUM), | ||
} ASN1_SEQUENCE_END_cb(RSA_PRIME_INFO, RSA_PRIME_INFO) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of this low level stuff in rsa_prime_info_cb should be necessary. You should just be able to make the new fields optional and extend RSA_free() to free up any multiprimes present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand this. It seems we also need to handle the new
side things if there is no rsa_prime_info_cb
; otherwise, who allocates those multi-prime related stuffs' memories in the RSA structure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The normal ASN.1 functions provide their own new/free methods based on the structure. The defaults may be sufficient in this case. If so that would mean the callback is not necessary. There may be something I'm missing though; I'll check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... there is a pp field which isn't part of the ASN.1 which is calculated and handled separately. While it is possible to allocate pp as part of rsa_multip_calc_product() and delete rsa_prime_info_cb I don't see any advantage over doing this in the callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One advantage is rsa_multip_calc_product()
can do calculation only and allocation is done outside this function by the caller. This will make the code more clear to understand. But anyway, this time I put an allocation of pp
in rsa_multip_calc_product()
and removed the callback in ASN.1.
crypto/rsa/rsa_lib.c
Outdated
err: | ||
sk_RSA_PRIME_INFO_free(prime_infos); | ||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we can't automatically set the version number in this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because there is no easy way to 'unset' this if the RSA structure will be reused as:
key = RSA_new();
/* set a bundle of primes, params for multi-prime usages */
RSA_set_version(key, 1);
/* use the multi-prime key */
/* after a while, need to use the key as a 2-prime key and reset the params and factors */
RSA_set_version(key, 0);
/* use the 2-prime key */
If we set the version number to 0 automatically in RSA_set0_factors
, that would require the calling order must be: RSA_set0_factors
then RSA_set0_multi_prime_params
So let the user to set version number explicitly might be a simple choice...
CHANGES file was also updated in new commit. |
Looking good. A few outstanding issues from me in comments above. |
New commit pushed to address comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved subject to a couple of nits being fixed. Ping @snhenson for second review.
crypto/rsa/rsa_lib.c
Outdated
int RSA_set_version(RSA *r, int32_t ver) | ||
{ | ||
/* { two-prime(0), multi(1) } */ | ||
if (ver != 0 && ver != 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use RSA_ASN1_VERSION_DEFAULT
and RSA_ASN1_VERSION_MULTI
instead of 0 and 1
crypto/rsa/rsa_ossl.c
Outdated
} | ||
|
||
for (i = 0; i < primes; i++) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the blank line here
Put a temporary hold on this while I check the ASN.1 part. |
??? If they generate insecure keys, they can't be using OpenSSL (since it doesn't support it), and if they insist on doing so, what prevents them from sticking with that other software? |
1st rule of public-key cryptography is that you don't talk about private key. This means that there is only one parameter that defines interoperability with outer world, and it's length of public key. And this is a separate question, separate from context of this request. Namely, [as already pointed out several times here] questions are a) which public key lengths do we permit [to ensure interoperability]; b) how many private factors does public key consist of [which has no bearing on interoperability]. Or in other words, references to TLS, CMS or other libcrypto use-cases discuss rather a), while what we're trying to discuss here is b). To be more specific. Question is not whether or not to keep RSA1024, but if we keep it, how many primes would we permit given that we aim to maintain not worse security promise. |
Ping @openssl , what's next move? |
As for references to legacy weak algorithms and schemes. Keyword there is legacy and interoperability, or in other words it's desired to keep them as option for backward compatibility. While here, i.e. in context of this request, backward compatibility is of no concern. Indeed, multi-prime support has no effect on ability to verify old signatures, or ability to produce new signatures with old keys. There is no compelling/legitimate reason to permit arbitrary public key slicing, i.e. such that weakens any given scheme. And once again, question is not about choice of the scheme, i.e. not whether or not to permit specific public key length, be it 64, 512 or 1024 bits, but about not weakening any specific one, be it 64 or 16384. |
Which I read as: Make sure that ECM attacks against multi-prime RSA at any supported key size and prime count, ... is not cheaper than GNFS attack on the same modulus size for the two prime case. With a generous safety margin just to account for plausible uncertainty in the relative costs. So, for example not using primes shorter than |
[As already mentioned multiple times] it has already been referred to couple of times here, "On the Security of Multi-prime RSA", http://cacr.uwaterloo.ca/techreports/2006/cacr2006-16.pdf. It's argued that there is no common floor value suitable for all lengths. For example it's asserted that 2048-bit public key should not consist of more that 3 factors, 4096 - of more than 4, etc. |
/* | ||
* if |r1|, product of factors so far, is not as long as expected | ||
* (by checking the first 4 bits are less than 0x9 or greater than | ||
* 0xF). If so, re-generate the last prime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"first bits" is ambiguous term (not to mention kind of explosive in sense that it has potential to evoke spirits of endian war), "most significant" is one that is appropriate. Also the statement is confusing in sense that 4-bit value can't be larger than 0xF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I'm actually uneasy about this check for 9. Original rationale is to merely obscure the fact that key is multi-prime (which should not be viewed as private information in sense that adversary can figure it out anyway). But question is if by insisting on specific bit pattern in most significant 4 bits you indirectly insist on specific pattern in less significant bits. Because in such case you might leak more information than you're trying to conceal. For example you're trying to conceal the fact that it's multi-prime, but indirectly reveal that it's specifically 4-prime. Well, amount of primes should not be viewed as private information either, what I'm rather trying to say is that argument in favour of check for 9 might contradict itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "most significant" is also not really appropriate term either.
The product is scaled down by the expected number of bits - 4,
and that value can be less than 9 or even 8. It can only exceed 15 if adj = 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, initial objection was specifically about term "first bits". I mean what is "first bit", where do you start counting? "Most significant" is the only unambiguous way to describe the bit of interest here. Then yes, there is second ambiguity in comment, how many most significant bits are we looking at. As we indeed can end up with 4, 5, 3, and yes, "product [being] scaled down by the expected number of bits minus 4" eliminates both ambiguities...
* in 4 prime case to avoid long loop. Max retry times | ||
* is set to 4. | ||
*/ | ||
i = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no statistical advantage in discarding 1st prime. I.e. i = 0
would do. I also find comment confusing. It's sound It sounds like it will fail after four attempts to generate 4-prime key. While what happens is that it takes 4 attempts in order to start over, and failure is not an option.
And it should also be read as "argument that one can make about support of legacy weak algorithms and schemes don't apply in context of this request [because it is not about legacy]." |
Other than nitpicking on comments and a disagreement about technical enforcement of the maximum number of primes (or the minimum size of each prime) is there anything else left on this PR? |
This PR at various times has had @mattcaswell @t-j-h and @levitte approval. @mattcaswell - if your approval still stands mine certainly does so this is ready to merge (assuming you clean up the noted libcrypto.num conflict ...) |
* Introduce RSA_generate_multi_prime_key to generate multi-prime RSA private key. As well as the following functions: RSA_get_multi_prime_extra_count RSA_get0_multi_prime_factors RSA_get0_multi_prime_crt_params RSA_set0_multi_prime_params RSA_get_version * Support EVP operations for multi-prime RSA * Support ASN.1 operations for multi-prime RSA * Support multi-prime check in RSA_check_key_ex * Support multi-prime RSA in apps/genrsa and apps/speed * Support multi-prime RSA manipulation functions * Test cases and documentation are added * CHANGES is updated
Conflicts are resolved and all commits are squashed into one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved.
Note however I would still prefer to set
RSA_MIN_PRIME_SIZE to 256
and RSA_MAX_PRIME_NUM to 8.
Which is not perfectly safe, but makes
it harder to choose completely unsafe
settings.
I think this has moved on someway since my review - but since @bernd-edlinger and @t-j-h both approve I don't feel a need to reconfirm. Please go ahead without my review. |
Cool, seems it's pushed forward. |
@bernd-edlinger will you merge? |
* Introduce RSA_generate_multi_prime_key to generate multi-prime RSA private key. As well as the following functions: RSA_get_multi_prime_extra_count RSA_get0_multi_prime_factors RSA_get0_multi_prime_crt_params RSA_set0_multi_prime_params RSA_get_version * Support EVP operations for multi-prime RSA * Support ASN.1 operations for multi-prime RSA * Support multi-prime check in RSA_check_key_ex * Support multi-prime RSA in apps/genrsa and apps/speed * Support multi-prime RSA manipulation functions * Test cases and documentation are added * CHANGES is updated Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #4241)
Merged to master. Thanks! |
Thanks! |
This feature is very useful if someone wants to reduce the cost of hardware accelerators and can
betolerate with a less security key. [This implementation is based on RFC 8017]Some performance data by using
openssl speed
:2-prime: 7979
3-prime: 9908
5-prime: 18617
8-prime: 24878
15-prime: 31811
(quantity of RSA 2048-bit private key operations in 10s)
Also, the generation time of 4096-bit key:
2-prime: 1.237
3-prime: 0.467
5-prime: 0.237
8-prime: 0.098
15-prime: 0.080
(in seconds)
To be simplified, the maximum number of primes is limited to 16.
After I finished this patch, I just found this previous work accidentally: https://rt.openssl.org/Ticket/Display.html?id=3477&user=guest&pass=guest. This is a 3 years old patch and was closed in the 'bug bankruptcy ' movement last year.
Note: This patch might be buggy, consider it as an experimental feature. Let's focus on discussing if the feature is worth being included into master...
Patent Caution: If I understand correctly, all related patents are expired on Jan 2017. (US5848159 and US7231040)
RSA private key. As well as the following functions:
RSA_generate_multi_prime_key
RSA_get_multi_prime_num
RSA_get0_multi_prime_factors
RSA_get0_multi_prime_crt_params
RSA_set0_multi_prime_params
RSA_set_version
RSA_get_version
Checklist