support custom random data source via mp_rand_source

[minad]

• deprecate MP_PRNG_ENABLE_LTM_RNG
• custom mp_rand_source is used always if set, which should be more aligned with user expectations
• use custom source in tune.c
• don't call random number generator once per digit, which is slow

[minad]

To clarify the commit message a bit more - the reasons for this PR:

• faster rand, not reading single digits
• user might want to replace random source instead of providing a fallback only, this can also be used in tune.c
• custom rand source is always available, not only if MP_PRNG_ENABLE_LTM_RNG is defined
• ltm_rng api is a bit weird, since it expects this callback which is passed through etc. I have seen that it's aligned with ltc, but I think the api proposed in this PR is more useful.
• unifying the public api to use only mp_ and MP_ prefix

[sjaeckel]

I really like this PR!

TBH I'd prefer to keep the API signature of the "custom random source" closer to rng_get_bytes() i.e. more like ltm_rng() is implemented...
... on the other hand I never used the callback parameter so probably the API of rng_get_bytes() should be updated ...

[sjaeckel]

Btw. the reason why the "custom random source" was disabled by default is to prevent the user from shooting himself in the foot ...

[minad]

I just pushed again.

Btw. the reason why the "custom random source" was disabled by default is to prevent the user from shooting himself in the foot ...

Hmm, not really an argument isn't it? The user has to manually explicitly select a custom random source.

I think the lib could also provide multiple sources like:

• mp_rand_source_jenkins (from tune.c)
• mp_rand_source_platform

and those can be passed to mp_rand_source. The default is the safe one and this is important.

[sjaeckel]

Hmm, not really an argument isn't it? The user has to manually explicitly select a custom random source.

nope. now even less where it's not just a global variable assignment but a function call. I like it.

I think the lib could also provide multiple sources like:

* mp_rand_source_jenkins (from tune.c)

* mp_rand_source_platform

and those can be passed to mp_rand_source. The default is the safe one and this is important.

I'd leave this work to the user if he really wants it. mp_rand_source is enough

[minad]

TBH I'd prefer to keep the API signature of the "custom random source" closer to rng_get_bytes() i.e. more like ltm_rng() is implemented...
... on the other hand I never used the callback parameter so probably the API of rng_get_bytes() should be updated ...

You mean returning the size instead of an error value? I think it is more consistent returning the error but ymmv. The callback should go away.

[sjaeckel]

You mean returning the size instead of an error value? I think it is more consistent returning the error but ymmv. The callback should go away.

I meant both, but we should most likely update rng_get_bytes(). @karel-m what do you think?

[czurnieden]

Thanks for cleaning the random bigint generation up ( bn_mp_rand.c)!
How do you make the thumbs-up…uh, here it is 👍

Oh, and the RNG in tune.c is supposed to wander into the testbed in demo/once and if you reached an agreement about the testing of private functions. Yes, that was a not-so-hidden hint and it wasn't about the code-trekking;-)

[czurnieden]

Now, that the compiler complained, I remember: ssize_t is (in Posix, not ISO-C and) explicitly unsigned to be able to return the -1 from several syscalls. Should have seen that, sorry.

The failure of the test for is_square is probably caused by the random number being zero. I'm unsure about the problems with the montgomery_reduce test but because it fails for low_mp it's most likely the same.
Now the question is: reimplement the original "first digit is not zero" or change it to: let the user check?

BTW: I think all of the tests should print their input at failure, especially if it is non-repeatable random input. If feasible, of course, no giant numbers.

[minad]

@czurnieden I fixed the issues with the casts. But what is the reason for the other test failures? I don't think the reason is that the random int is zero. Maybe I made a mistake in the random number generator?

[minad]

@czurnieden Could you check please? I get errors in mp_is_square and mp_montgomery_reduce. I basically took your s_random implementation from tune and replaced the inefficient version in mp_rand. It seems correct but now we get those failures.

[czurnieden]

I get errors in mp_is_square and mp_montgomery_reduce.

With low_mp ( 8-bit, 16-bit )?
With what input?

If it is MP_8BIT is most probably a zero but that is easily checked. MP_16BIT on the other side is a bit too large for that.

(you have put mp_zero(a) inside if(digits <= 0) instead of at the start, is there a special reason for it besides speed?)

[czurnieden]

Yes, with MP_8BIT and is_square I get a zero as the input.
With MP_16BIT it is montgomery_reduce and also a zero.

Propose to reinstall the little loop that guarantees the first digit to be different from zero.

The way you do it can also cause leading zeros, propose a mp_clamp() at the very end.

[minad]

The way you do it can also cause leading zeros, propose a mp_clamp() at the very end.

Done. Problem persists.

(you have put mp_zero(a) inside if(digits <= 0) instead of at the start, is there a special reason for it besides speed?)

Moving this in the beginning fixes the issue with montgomery. Is it a guaranteed invariant that unused allocated digits must be zero?

[minad]

@czurnieden

Propose to reinstall the little loop that guarantees the first digit to be different from zero.

This is a bad idea. I don't want random numbers to have some properties like being nonzero at some bits always. The is_square and montgomery tests should be fixed instead.

[czurnieden]

I don't want random numbers to have some properties like being nonzero at some bits always.

I understand that but changing it would change the API quite, well, not drastically but not in a small amount, there is a huge difference between "can return zero" or "cannot return zero". It might brake a lot of user programs.

What's with LTC? I don't use it, so I cannot tell quickly but it might rely on "first digit non-zero" somewhere, please check.

The is_square and montgomery tests should be fixed instead.

There is nothing to fix, they were based on the current API, not your new one.
They need to be changed, though, if we decide to change the API.

Yeah, nitpicking, I know ;-)

[minad]

The documentation says The random number generated with these two functions is cryptographically secure.... No it is NOT.

The current API is horribly broken. If programs or tests relying on the broken behavior break it is good.

I don't know why I should assume that a random function has any kind of bias?

@czurnieden I am thinking a bit more about it. And it seems that the current functions does lshd after initializing the first digit to ensure that the generated number always has a nonzero highest digit. Such that the random number has the number of requested digits. Is that right?

But to be honest, I am out of my comfort zone here. Someone who knows more should tell. @sjaeckel

[minad]

@czurnieden @sjaeckel It seems that mp_rand was not meant for cryptographic in the beginning since it relied on rand(). That biased behaviour is a remnant from back then. @sjaeckel made the function safer in #103. The question is what we want here.

[czurnieden]

The question is what we want here.

If LTC has no problems with it: change the API. Well, it's not even the API as you said, the non-zero first digit behaviour is not documented. And if it is not documented, we can change it without much ado.

But may I humbly suppose that we warn the users before we do it?
It took you a moment to get the reason for the failures, didn't it? ;-)

And if LTC relies on it we should give them a chance to get prepared.

Oh, nearly forgotten: mp_rand() was meant as a source for prime generation, so the first digit must be odd, hence non-zero by definition or the whole thing will be dumped and precious entropy wasted..

[minad]

@czurnieden Ok I restored the old behavior for now and opened #237. This PR can then be merged without further changes and the behavior of mp_rand can be discussed separately.

[minad]

@sjaeckel Tests are green now. If you agree with the API, it is ready to merge.

[czurnieden]

Guaranteeing the MSD to be non-zero removes the need for the final mp_clamp().
I know you hated it anyways ;-)

Oh, nearly forgotten: mp_rand() was meant as a source for prime generation, so the first digit must be odd, hence non-zero by definition or the whole thing will be dumped and precious entropy wasted..

No, that doesn't apply to the original LTM, I have implemented it that way in my own (unpublished) version, sorry.

I still don't understand the problem here. You want a specified number of limbs filled with random bits and if the MSD is zero you don't get that specified number of limbs and, in the case of the generation of primes for cryptographic purposes, you won't get the needed number of bits as often as possible.

The cryptographic problem shows up only in the case of a single limb. That would be non-zero also. But if you want a single limb that might be zero, too, you can and should use mp_rand_digit() instead. A note in the documentation would be sufficient here, I guess, to emphasize the fact that the size n you give mp_rand() is the minimum size—the range of the result is \beta^(n-1) < x < \beta^n.

It would a bit too brutal, I think, to return MP_VAL in case somebody wants a single limb, wouldn't it? ;-)

Oh, I saw @sjaeckel was faster than me.

[minad]

Guaranteeing the MSD to be non-zero removes the need for the final mp_clamp().
I know you hated it anyways ;-)

I know. But I kept it intentionally for the case that we remove the while loop.

[minad]

@czurnieden My problem is mostly regarding to testing. I am not using tommath for anything cryptographical and I am not using tomcrypt. For testing the current behavior is definitely bad since we omit many test cases.

I think we should offer another function mp_rand_interval which can be used in tests. This function should generate numbers within a given interval including small numbers.

[sjaeckel]

The public API didn't change and the implementation looks much cleaner now and is supposedly a lot faster, so for sure this will be a yes :)

[minad]

@sjaeckel Addressed your comments. Let's see if the tests come out green.