Skip to content
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

Fix ECDH-Curve25519 private key masking #40

Merged
merged 10 commits into from
Nov 20, 2019

Conversation

zugzwang
Copy link
Contributor

@zugzwang zugzwang commented Nov 7, 2019

Curve25519 private key issue

(See #39)

Per the original scheme and RFC7748, private keys are integers in the set

2^{254} + 8 * [0, 2^{251})

(In particular, they must be a multiple of 8, because of the small subgroup - see the cofactor). This translates to the following "masking" operation:

var k [32]byte
// Sample k at random
k[0] &= 248
k[31] &= 127
k[31] |= 64

Currently, private keys are stored and exported "unmasked", which is conceptually wrong since they do not encode integers in the mentioned set.

Correctness within the library works before the "secret key" is masked before concerned crypto operations, but for instance, when exporting, this causes gpg to treat them as if they were encrypted, and keeps asking for a password that doesn't exist.

Fix

Implement the following key-generation procedure:

1. Sample random k in [0, 256)
2. Secret key = Mask(k)
3. Public key = (Secret Key) * G

@zugzwang zugzwang changed the title [WIP] Fix ECDH-Curve25519 private key masking Fix ECDH-Curve25519 private key masking Nov 8, 2019
Comment on lines 19 to 24
func X25519GenerateParams(rand io.Reader) (priv [32]byte, x [32]byte, err error) {
var n, helper = new (big.Int), new (big.Int)
// Generates a private-public key-pair.
// 'priv' is a private key; a scalar belonging to the set
// 2^{254} + 8 * [0, 2^{251}), in order to avoid the small subgroup of the
// curve. 'pub' is simply 'priv' * G where G is the base point.
// See https://cr.yp.to/ecdh.html and RFC7748, sec 5.
func generateKeyPair(rand io.Reader) (priv [32]byte, pub [32]byte, err error) {
Copy link
Member

@twiss twiss Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this function is technically part of the public API, so we probably shouldn't remove it - even though it does seem unlikely that someone would use it.

Other than that, LGTM 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, decided to unexport this function and provide a better name.

@twiss twiss merged commit 53909a3 into ProtonMail:master Nov 20, 2019
@zugzwang zugzwang deleted the fix-gpg-ecdh-compatibility branch April 21, 2020 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants