WIP: Fix biased BLE pass key generation

[Zorvalt]

The pass key generation in the NimbleController is biased to due the use of a modulo.

TLDR: This can be simply fix by rejecting random values that are too big.

This patch simply re-generates the random number when it is not suitable. This happens less than 0.1 % of the time so the loop does not cost that much and fixes the bias.

I am not sure this is an issue now but leaving this kind of cryptographic holes can come bite us later.

Not tested yet

I am not able to test this change before a few days. Hence the WIP state.

Details

The pass key is a 6 digits code (1'000'000 possibilities).
It is important every possible value has an equal probability of getting generated.
Simply applying a modulo creates a bias since 2^32 is not a multiple of 1'000'000 and high values (such as 999'999)
do not have the same probability of being generated.

To prevent that, we can reject values greater than 999'999.
Rejecting values would happen a lot since 2^32-1 is way greater than 1'000'000.
An optimization is to use the greatest multiple of 1'000'000 as the upper bound for acceptable values.

Great explanation at:
https://research.kudelskisecurity.com/2020/07/28/the-definitive-guide-to-modulo-bias-and-how-to-avoid-it/

[Avamander]

How unlucky would one have to be to make the while loop take three or more seconds?

[Zorvalt]

Extremely unlucky !

I do not know how long it takes to generate the number but I guess it's reasonable. The probability of having to run it a second time is already low :
The random number must fall between 4293999999 and 4294967295 (967296 values). The chance of that to happen is 967296 / 2^32 ~= 0.023 %

Two runs : 0.023 % * 0.023 % ~= 0.00053 %
Three runs : 0.023 % *0.023 % *0.023 % ~= 1,2167e-05 %
And so on...

Let's say it's practically impossible :-)

[evergreen22]

Yeah, EXTREMEMLY unlucky, so why worry about it?

[trman]

Yeah, EXTREMEMLY unlucky, so why worry about it?

because , it's like @Zorvalt said , it's a hole in a secure connection and it's better closing a hole than suffering later @evergreen22 .

moreover 0.02 % over 20000 people mean than more than 4 people might have this bug...
the more the people , the more it might be triggered

[evergreen22]

You obviously do not understand the role that the passkey plays in secure pairing. It isn't and secure "hole" and it is not worth fixing.

[Zorvalt]

I agree it is not an exploitable security issue on its own, now. That is why I said it could bite us in the future.
What if another bug allows an attacker to force re-pairing the phone with the watch ? Then, the probability increases. This is still not enough but one step closer to allowing an attacker to sniff or tamper with InfiniTime connections.

If the fix was complex or creating a performance issue, I would understand but here, it's a 4 lines fix that has almost no impact. So let me ask the opposite question : Why not patch it?

[Riksu9000]

Aren't the encryption keys generated separately from the passkey? The article linked says that with enough biased signatures you can break some cryptographic schemes, but the issue here is different. I'm assuming that the phone just sends the inputted passkey to the watch and the watch internally checks if it matches, and the watch never leaks the passkey or anything related to it via BLE does it?

Reason not to patch it would be if it isn't an issue in the first place.

[yehoshuapw]

Have a look at
part 3 and part 4

The whole passkey thing happens after key exchanges, and is just meant to verify that there is no MitM attack - so at least on one side the human inputs the number,
and then only those two endspoints know it - and it can act well as a one time passcode.

If there is a MitM device, it will not know the right number.
if there is enough options, even if some or more likely then others - a single mistake by the attacker will cause the verification to fail - in which case, the devices realize something is wrong.

So having this guessable is bad, but having a non-uniform random is ok. (unless there is some specific value, which happens often enough that guessing only for it, will work enough of the times to be meaningful)

[evergreen22]

Aren't the encryption keys generated separately from the passkey?

Correct. The passkey has NOTHING to do with the keys of the ble connection.

Reason not to patch it would be if it isn't an issue in the first place.

Yes, which is why it isn't worth patching.

The OP is literally asking us to fix something that is NOT broken.

This is the case of reading something on the internet and not following it all the way through to see if there is an actual impact. Originally I generated the passkey as 6 separate digits in a loop to avoid potential bias, but after investigating and sampling the rng on the watch I determined there was zero impact on the predictability of the passkey by switching to a single statement with mod. So I switched out 4 lines of not easily understandable code for a single line of crystal clear code.

I applaud the OP for reading the code, asking questions, and searching for answers. But they should have also asked "how is the rng setup on the watch?" and then found out. They should have written a test program (mine is 6 lines of C++) to sample the rng with mod to see if there is any impact (bias) of using mod on the rng in the watch. These two steps would have clearly confirmed there is not an issue and the rng output with mod satisfies the required statistical properties for use as a pairing passkey.

[Zorvalt]

@Riksu9000 @yehoshuapw Thank you for your detailed explanations and the links. I was subject to a misconception in the Bluetooth protocol and the role of the passkey: I saw a biased randomness for a secret generation and jumped on it. I am no cryptography or Bluetooth expert - and I believed it shows - I was concerned about leaving a possible flaw of any kind to an attacker.

@evergreen22 Thank you for your applause and your work on the encryption. Sorry for the misunderstanding. Even though I know about the existence of testing of statistical properties of a random generator, I mainly focused on fixing the existing bias than quantifying it as I thought it was a critical issue and the fix was simple.

I close this PR. Thanks again to all of you.

[Avamander]

This improvement costs what, a handful of bytes? I see no reason why not merge it.

Fair, this does not pose any significant risk right now, but that is the mother of all future f*ckups. Secondly, it's then a thought-trough segment of code, that's great, we need more of that.

[JF002]

I read these comments and... I'm confused...
If I understand correctly, there is no security issue at all regarding the encryption of the BLE communication. But there might be a small bias regarding the generation of the 6-digit PIN code? Which is not an issue per se, but the changes in this PR seems to apply "state of the art" rules regarding the handling of random numbers.

Am I right?

[Zorvalt]

@JF002 Yes, except I would not use "state of the art" in this context. Just correct math to have unbiased randomness.

I was thinking of maybe suggest an other PR just implementing some rand_max(int value) that would produce an unbiased random number between 0 and value. This could be used in other contexts such as the "dice app", for example. And maybe then it could be used here.

But, still I was wrong when I said there was an issue about security. There is none.

[JF002]

Just correct math to have unbiased randomness.

I think it deserved to be merged for this reason alone ;-) What do you think?

[evergreen22]

> Just correct math to have unbiased randomness. I think it deserved to be merged for this reason alone ;-) What do you think?

I do not think it should be merged for the following reasons: 1) It is less efficient in terms of time. 2) It is less efficient in terms of space. 3) It solves a non-existent problem. The underlying tinycrypt PRNG provides enough randomness, even with the bias, to make the resulting passkey essentially impossible to guess, so it is sufficient for our authentication needs. Thank you for asking for my opinion.