[Security]: key generation algorithm reduces entropy of raw Curve25519 keys

[fmoessbauer]

Hardware

Not Applicable

Connection Type

HTTP

Local or Hosted

http://meshtastic.local

Firmware Version

2.5.3

Description

In d0bd029, the generation of the Curve25519 based keypair was reworked, setting multiple bits of the private key to fixed values. This change was done without any further explanation in the commit message or in code. While this is suspicious by itself, I also don't understand why these particular bits are pinned. A possible explanation is given in https://neilmadden.blog/2020/05/28/whats-the-curve25519-clamping-all-about/. Anyways, this code change must be commented.

Problematic code:

  const key = x25519.utils.randomPrivateKey();
  key[0] &= 248  # 01111100b
  key[31] &= 127 # 01111111b
  key[31] |= 64  # 01000000b

Diff: d0bd029#diff-01dc5badd89a6aa375c3b6218d6c7abb9e72f62f0d6fa9cbddada0cfd0837338L4

PS: Please add a security policy to this project so that users can report findings in a confidential way. Otherwise no responsible disclosure is possible.

Relevant console output

No response

[ianmcorvidae]

This is due to the differences between ed25519 and x25519, to the best of my understanding. I don't fully understand the difference myself, but this is in various implementations, e.g. https://github.com/paulmillr/noble-curves/blob/187654df5e6811a21d06e0819414e7e87293246a/src/ed25519.ts#L58 within the noble library being used here, and https://rweather.github.io/arduinolibs/Curve25519_8cpp_source.html#l00245 within the library used by firmware.

[ianmcorvidae]

Ah, found it in the RFC: https://www.rfc-editor.org/rfc/rfc7748#section-5

Scalars are assumed to be randomly generated bytes. For X25519, in
order to decode 32 random bytes as an integer scalar, set the three
least significant bits of the first byte and the most significant bit
of the last to zero, set the second most significant bit of the last
byte to 1 and, finally, decode as little-endian. This means that the
resulting integer is of the form 2^254 plus eight times a value
between 0 and 2^251 - 1 (inclusive). Likewise, for X448, set the two
least significant bits of the first byte to 0, and the most
significant bit of the last byte to 1. This means that the resulting
integer is of the form 2^447 plus four times a value between 0 and
2^445 - 1 (inclusive).

[fmoessbauer]

Ah, found it in the RFC: https://www.rfc-editor.org/rfc/rfc7748#section-5

Thanks! Can someone of the original authors (@jp-bennett / @KomelT) please add this reference as a comment next to the code? This is not obvious and these (seemingly random) changes will always ring the bells of security auditors. I don't want to make that comment, as I'm still not 100% sure what exactly is returned by the library function.

[jp-bennett]

Hey, thanks for tagging me. Yes, this is specified in the RFCs, to create valid keys. You're absolutely right that it should be documented. We have a security policy for the firmware itself. I'll see if we can expand it to the whole org.