Minor cleanup of the rsa mp limits code

[bernd-edlinger]

Reduce RSA_MAX_PRIME_NUM to 5.
Remove no longer used RSA_MIN_PRIME_SIZE.
Make rsa_multip_cap honor RSA_MAX_PRIME_NUM.

[levitte]

I'd be tempted to make a hard assert here. Seriously, if we have a cap value that's internally set to something higher than our own maximum, we've implemented it wrong.

[bernd-edlinger]

Yes, I thought about a static assert here, do we have any examples of that
in the code base?

[richsalz]

let's not add asserts; the code is simple and the clamp is doing what it should

[levitte]

git grep assert answers your question ;-)

I suggest using the standard assert() API here. I know that we have a few different assertion forms throughout the source, but for this case, this is my recommendation. It also seems to be what we use in algos, for example crypto/aes, crypto/aria, ...

[richsalz]

Ya know what? Just remove the d--n if statement. It serves no purpose. Just do this:

int rsa_multip_cap(int bits)
{   
    if (bits < 1024)
        return 2;
    if (bits < 4096)
        return 3;
    if (bits < 8192)
        return 4;
#if RSA_MAX_PRIME_NUM < 5
# error "NUM_PRIME inconsistent"
#endif
    return 5;
}   

[richsalz]

aasert makes a runtime failure. the #if makes it a compile-time failure. and the code is only eight lines long!

[bernd-edlinger]

Yes, this #if # error #endif works like a static assertion.
The idea of a static assertion is that it fails at compile time and not at run time.

[levitte]

Yeah, that's a kinda working middle ground. And then you have to remember to change the test when you change the max result. In this case, it's not too bad since they're 3 lines from each other, but usually, I'd say that's a maintainance problem waiting to happen...

I was thinking that an assert would be accompanied with a test thats designed to try and trigger the assertion (with bits being a gazillion, say...). Also, let's remember that assert() is debug build only.

... ah well, I feel I'm fighting an uphill battle here.

[bernd-edlinger]

I must admit Rich's solution looks a bit ugly to me,
maybe I sleep a night over it....

[bernd-edlinger]

Well, although this hunk is not strictly necessary, it makes it much easier to prove
that rsa_multip_cap(b)<=RSA_MAX_PRIME_NUM and that the array accesses
in rsa_builtin_keygen are within proper bounds.
And yes alternative solutions exist, but they have other drawbacks.
So unless there are objections I would like to go with Rich's original approval
and commit the original version as-is.

[InfoHunter]

So does this mean now it's even unable to use OpenSSL to use a 'weak' multi-prime key?

[bernd-edlinger]

So does this mean now it's even unable to use OpenSSL to use a 'weak' multi-prime key?

Yes, private keys with more than 5 primes will not work any more.

[richsalz]

Unless OpenSSL is configured with -DMAX_RSA_PRIMES=20 or such.

[InfoHunter]

Unless OpenSSL is configured with -DMAX_RSA_PRIMES=20 or such.

Well...

[richsalz]

I'd really rather we did what I suggested, but I won't remove my approval. Finding the "error" at compile time is much better than finding a coredump at runtime. And the code is so darn small. Shrug, whatever.

[bernd-edlinger]

Merged. Thanks!