Reject small order elements on key import and signature verification

[twiss]

Reject small order elements when importing keys, and reject small order elements during EdDSA signature verification.

Also, no longer require checking for all-zero derived keys in X25519 and X448, as this is redundant with checking for small-order elements.

Fixes #10.

Cc @panva and @littledivy, since you have experimental implementations, it would be great if you could review this. I believe many crypto libraries already check for small order elements, but this aims to specify when exactly we should reject them.

(There's also an open question on whether we should list the small order elements in the spec explicitly? Or perhaps that's unnecessary / too low level. It may be worth having them in the tests, though.)

---

Preview | Diff

[panva]

@twiss Happy to check but could you propose some vectors?

[twiss]

Yeah; here are lists of small order elements from libsodium and circl:

• x25519
• ed25519
• x448 (also valid for ed448, I think)

I'll try to find a more authoritative source but for now I think those could be used.

[panva]

There doesn't seem to be a definitive list to use in those links. The linked go impl list is a subset of one from sodium.

Not being an expert on the subject myself it would be great if someone could propose changes to #7 and #8 with all to be failing vectors.

[twiss]

I believe the libsodium lists are exhaustive, not sure about the circl one. @galadran would you be able to weigh in here, perhaps?

[twiss]

Hey 👋 Here are some test cases, partially gleaned from libsodium, partially gleaned from https://cr.yp.to/ecdh.html#validate.

Test cases

// Test X25519 low-order public keys
const curve25519p = 2n**255n - 19n;
const x25519LowOrderElements = [
    0n,
    1n,
    325606250916557431795983626356110631294008115727848805560023387167927233504n,
    39382357235489614581723060781553021112529911719440698176882885853963445705823n,
    curve25519p - 1n,
    curve25519p,
    curve25519p + 1n,
];
x25519LowOrderElements.forEach(async (lowOrderElement) => {
    const rawPublicKey = toLittleEndian(lowOrderElement, 32);
    try {
        await crypto.subtle.importKey('raw', rawPublicKey, 'X25519', false, []);
    } catch (e) {
        if (e.name !== 'DataError') throw new Error(`Expected DataError, received ${ e.name }`)
        return;
    }
    throw new Error(`X25519 low-order public key accepted: ${ lowOrderElement }`);
});

// Test X25519 public keys that will be low-order after being clamped
x25519LowOrderElements.forEach(async (lowOrderElement) => {
    const rawPublicKey = toLittleEndian(lowOrderElement, 32);
    rawPublicKey[31] |= 1 << 7; // Set the bit that will be clamped
    try {
        await crypto.subtle.importKey('raw', rawPublicKey, 'X25519', false, []);
    } catch (e) {
        if (e.name !== 'DataError') throw new Error(`Expected DataError, received ${ e.name }`)
        return;
    }
    throw new Error(`X25519 low-order public key accepted: ${ lowOrderElement } (with unused bit set to 1)`);
});

// Test X25519 low-order elements that *won't* be low-order public keys after being clamped
const x25519UnclampedLowOrderElements = [
    curve25519p + 325606250916557431795983626356110631294008115727848805560023387167927233504n,
    curve25519p + 39382357235489614581723060781553021112529911719440698176882885853963445705823n,
    2n * curve25519p - 1n,
    2n * curve25519p,
    2n * curve25519p + 1n,
];
x25519UnclampedLowOrderElements.forEach(async (lowOrderElement) => {
    const { privateKey } = await crypto.subtle.generateKey('X25519', false, ['deriveBits']);
    const rawPublicKey = toLittleEndian(lowOrderElement, 32);
    const publicKey = await crypto.subtle.importKey('raw', rawPublicKey, 'X25519', false, []);
    const derivedBits = new Uint8Array(await crypto.subtle.deriveBits({ name: 'X25519', public: publicKey }, privateKey, null));
    if (derivedBits.length !== 32) throw new Error('Unexpected derived bits length');
    if (derivedBits.every(bit => bit === 0)) throw new Error('Derived bits are all zero');
});

// Test Ed25519 low-order public keys
const ed25519LowOrderElements = [
    0n,
    1n,
    2707385501144840649318225287225658788936804267575313519463743609750303402022n,
    55188659117513257062467267217118295137698188065244968500265048394206261417927n,
    curve25519p - 1n,
    curve25519p,
    curve25519p + 1n,
];
ed25519LowOrderElements.forEach(async (lowOrderElement) => {
    const rawPublicKey = toLittleEndian(lowOrderElement, 32);
    try {
        await crypto.subtle.importKey('raw', rawPublicKey, 'Ed25519', false, ['verify']);
    } catch (e) {
        if (e.name !== 'DataError') throw new Error(`Expected DataError, received ${ e.name }`)
        return;
    }
    throw new Error(`Ed25519 low-order public key accepted: ${ lowOrderElement }`);
});

// Test Ed25519 low-order signature R
ed25519LowOrderElements.forEach(async (lowOrderElement) => {
    const { publicKey } = await crypto.subtle.generateKey('Ed25519', false, ['verify']);
    const signature = new Uint8Array(64);
    signature.set(toLittleEndian(lowOrderElement, 32));
    const data = new Uint8Array(8);
    try {
        await crypto.subtle.verify('Ed25519', publicKey, signature, data);
    } catch (e) {
        if (e.name !== 'DataError') throw new Error(`Expected DataError, received ${ e.name }`)
        return;
    }
    throw new Error(`Ed25519 low-order signature R accepted: ${ lowOrderElement }`);
});

// Test X448 low-order public keys
const curve448p = 2n**448n - 2n**224n - 1n;
const curve448LowOrderElements = [
    0n,
    1n,
    curve448p - 1n,
    curve448p,
    curve448p + 1n,
];
curve448LowOrderElements.forEach(async (lowOrderElement, i) => {
    const rawPublicKey = toLittleEndian(lowOrderElement, 56);
    try {
        await crypto.subtle.importKey('raw', rawPublicKey, 'X448', false, []);
    } catch (e) {
        if (e.name !== 'DataError') throw new Error(`Expected DataError, received ${ e.name }`)
        return;
    }
    throw new Error(`X448 low-order public key accepted: ${ lowOrderElement }`);
});

// Test Ed448 low-order public keys
curve448LowOrderElements.forEach(async (lowOrderElement, i) => {
    const rawPublicKey = toLittleEndian(lowOrderElement, 57);
    try {
        await crypto.subtle.importKey('raw', rawPublicKey, 'Ed448', false, ['verify']);
    } catch (e) {
        if (e.name !== 'DataError') throw new Error(`Expected DataError, received ${ e.name }`)
        return;
    }
    throw new Error(`Ed448 low-order public key accepted: ${ lowOrderElement }`);
});

// Test Ed448 low-order signature R
curve448LowOrderElements.forEach(async (lowOrderElement, i) => {
    const { publicKey } = await crypto.subtle.generateKey('Ed448', false, ['verify']);
    const signature = new Uint8Array(114);
    signature.set(toLittleEndian(lowOrderElement, 57));
    const data = new Uint8Array(8);
    try {
        await crypto.subtle.verify('Ed448', publicKey, signature, data);
    } catch (e) {
        if (e.name !== 'DataError') throw new Error(`Expected DataError, received ${ e.name }`)
        return;
    }
    throw new Error(`Ed448 low-order signature R accepted: ${ lowOrderElement }`);
});


function toLittleEndian(bigNumber, size) {
    const result = new Uint8Array(size);
    let i = 0;
    while (bigNumber > 0n) {
        if (i >= size) throw new Error('Number too big');
        result[i] = Number(bigNumber % 256n);
        bigNumber /= 256n;
        i++;
    }
    return result;
}

@panva On Node.js v19.0.0-nightly20220613156365ebfc, importing CFRG raw public keys seems to fail unconditionally, so these test cases don't really work there / test anything currently, but it would still be great if you could check them.

And then, if they seem reasonable, we can add them to wpt once this is merged.

(Also, it would be good to test the same using JWK and SPKI at some point.)

[panva]

importing CFRG raw public keys seems to fail unconditionally

I need to look into this first.

[panva]

The checks are certainly possible but in case of key imports add a non-trivial amount of input normalization. In case of spki and pkcs8 formats (given it may also contain the public key) also asn.1 parsing to pluck the public key out of the der structure.

[twiss]

Yeah. Ideally, this would be done by the crypto library.

In case of spki and pkcs8 formats (given it may also contain the public key) also asn.1 parsing to pluck the public key out of the der structure.

During public key import, you need to parse the spki anyway. During private key import, the way the spec is currently written, the private key actually can't contain the public key, because it says to parse a PrivateKeyInfo [RFC 5208] rather than a OneAsymmetricKey [RFC 8410], same as the other algorithms in Web Crypto. I considered changing that, but seemingly nobody implements the latter anyway, and it's not entirely clear what to do with it (should we check that the public key corresponds to the private key? That would be nice, but probably even more annoying to implement.) So for now checking the public key during private key import isn't necessary.

The way I'd imagine implementing this is that after the "normal" key import steps, you'd call a function to check for low-order public keys. For example, libsodium has a crypto_core_ed25519_is_valid_point function (but no crypto_core_x25519_is_valid_point, for some reason :/ As an alternative, one could check this by doing a trial X25519 invocation with any private key, and checking whether the result is all zeros - of course this is much more expensive than necessary, but it's simple to implement).

[littledivy]

@twiss IMO the small order check for both signature and public key should happen only in verify. This is how it is done in some crypto libraries so the behaviour makes most sense to me.

libsodium: https://github.com/jedisct1/libsodium/blob/d4ee08ab8a1c674203796161af6d013283b33d69/src/libsodium/crypto_sign/ed25519/ref10/open.c#L37-L42

Rust ed25519-dalek: https://github.com/dalek-cryptography/ed25519-dalek/blob/925eb9ea56192053c9eb93b9d30d1b9419eee128/src/public.rs#L303

Plus, it does not enfore the extra normalization steps on implementations in importKey. An implementation can be smart
about it and do this check AOT in importKey - if thats cheap enough... OR it can do the check in any operation where it gets to pluck out asn.1 parts and store the result to later throw in verify.

[panva]

@tniessen @jasnell is there a way to check key data for small order elements with our KeyObject implementation once established in OpenSSL?

[twiss]

@twiss IMO the small order check for both signature and public key should happen only in verify. This is how it is done in some crypto libraries so the behaviour makes most sense to me.

Thanks for the feedback. I'm aware most crypto libraries do it during verify (if at all), but I think that's usually because of historical or backwards compatibility reasons, as RFC8032 doesn't mandate this check. Returning false in one extra situation is less "disruptive", so to speak, than throwing an error in one more case. But since we're making a new spec here, I think it makes more sense to throw an error as soon as we can detect it.

libsodium: https://github.com/jedisct1/libsodium/blob/d4ee08ab8a1c674203796161af6d013283b33d69/src/libsodium/crypto_sign/ed25519/ref10/open.c#L37-L42

libsodium takes a const unsigned char* as public key, rather than any kind of imported key object, so it can't do it anywhere else.

It does offer a crypto_core_ed25519_is_valid_point function, though, which can be called ahead of time.

Rust ed25519-dalek: https://github.com/dalek-cryptography/ed25519-dalek/blob/925eb9ea56192053c9eb93b9d30d1b9419eee128/src/public.rs#L303

This points to the verify_strict function, while they also have a verify function, which doesn't do this check (and predates verify_strict). So, they can't do the check on import as it would break backwards compatibility for them.

Note that PublicKey::from_bytes does do the check for whether the point is on the curve, even though RFC8032 lists it as part of the "Verify" algorithm (since it doesn't have an "Import Key" step).

Plus, it does not enfore the extra normalization steps on implementations in importKey. An implementation can be smart about it and do this check AOT in importKey - if thats cheap enough... OR it can do the check in any operation where it gets to pluck out asn.1 parts and store the result to later throw in verify.

Right. The thing I'm worried about, though, is that some implementations will do the check in importKey (and throw immediately rather than store the error), and some will do it in verify, and some won't do it at all (as currently it's not specified at all). For Web Crypto, interoperability is obviously very important, and so I think it's important to define these error conditions clearly, and checking the public key in importKey makes the most sense to me.

This check also seems roughly analogous to the check in ECDSA key import which says:

If the key value is not a valid point on the Elliptic Curve identified by the namedCurve member of normalizedAlgorithm throw a DataError.

So - for consistency with the rest of Web Crypto I think it's better if we do it in importKey.

[twiss]

I realized that we should then probably also do the check for the point being on the curve in importKey, for consistency. (I'll propose that in a separate PR, though.)

Then, an implementation using libsodium can just call crypto_core_ed25519_is_valid_point during key import, for example. (And if other implementations don't have such a function, perhaps they would be amenable to adding one.)

[davidben]

From the Chrome side, WebCrypto should not invent its own cryptographic primitives or checks. Whether the check for X25519 happens on import or on use, the net effect is the same. RFC 7748 chose to do it on use, so the answer is to do it on use. This PR is thus a regression of this spec's suitability for implementation.

Ed25519 is a bit more complex because there are quite a lot of variations. That will probably require a bit more teasing apart, so I suggest we treat this issues as separate.

[panva]

I also side with rejection on use (X...) or verify returning false (Ed...). We're using OpenSSL in Node.js and that is what we have available. Adding more is non-trivial, expensive to execute and does not net obvious benefits.

[twiss]

Whether the check for X25519 happens on import or on use, the net effect is the same.

There is a difference between doing it before use and after use, though. RFC 7748 does it after use, which may leave open the possibility of side-channel attacks when using low-order points. That being said, I'll concede that Web Crypto is not the best place to address this problem, and it would be ideal if crypto libraries had better support for these checks. I guess we could always allow, once/if they add them, to do the checks before use but still inside the derive bits operation, without a change in observable behavior (outside of side channels). That way, we also explicitly accommodate Mozilla's current behavior, since NSS's implementation of ECDH over X25519 (for TLS) already does the check proposed here.

Also, not to be overly pedantic, but in RFC 7748, the check for the all-zero output of X25519 is suggested as an optional step of ECDH, outside of X25519 itself. So, we'll anyway be adding a nonstandard check to X25519, as many implementations (e.g. OpenSSL's) already do. So I think we shouldn't be too worried about adding a check for small-order elements before X25519, as it's similar to the check for an all-zero output afterwards. But OK, if existing crypto libraries don't have support for this, we could make it optional, instead.

I'll make a PR for that to replace this one, and consider Ed25519 separately, indeed ☺️

[davidben]

That paper seems to describe an attack on an implementation that is already not constant-time. When implementing X25519, or indeed any kind of modular arithmetic with secrets, the number of limbs should be based on the public modulus, not the secret value. E.g. this paper describes another thing that can go wrong if you do this. I'm pretty dubious that such a leak wouldn't admit another attacks even with the low-order check performed first.

But, yeah, as you say, it produces the same observable results to check all-zeros immediately after the function or do look for a list of low-order points immediately before. shrug

Also, not to be overly pedantic, but in RFC 7748, the check for the all-zero output of X25519 is suggested as an optional step of ECDH, outside of X25519 itself. So, we'll anyway be adding a nonstandard check to X25519, [...]

RFC 7748 describes an optional all-zero check as part of the overall suite. Yes, it is technically outside the X25519 function, but it's an easy thing the WebCrypto spec can reference ("do the all-zero check as described in RFC 7748, section 6.1").

RFC 8446 does exactly that:

Implementations MUST check whether the computed Diffie-Hellman shared
secret is the all-zero value and abort if so, as described in
Section 6 of [RFC7748].

There's no other defined mechanism for a low-order check. WebCrypto would need to invent its own (e.g. including a list of all of them, or prescribing an extra call to the X25519 function).

[twiss]

There's no other defined mechanism for a low-order check. WebCrypto would need to invent its own (e.g. including a list of all of them, or prescribing an extra call to the X25519 function).

Sure, fair enough. I think adding a list in an appendix or so is fairly doable, though. (Prescribing an extra call to X25519 seems like a lot of unnecessary overhead, instead.) At the risk of pulling Ed25519 back into the discussion, we'll most likely have to do the same thing there as well, anyway.

[javifernandez]

Hi, what's the status of this PR ? I'm preparing the intent-to-ship request for Chrome and it'd be good to know if there are still plans to do this change in the spec.

[twiss]

Hey 👋 Apologies for the delay. The plan is to replace this PR with a check for low-order elements (the key and the signature's point R) during signature verification (and key derivation, but that's already there) instead of key import. I'll try to do that next week, as I'm off this week ☺️

[twiss]

Closing in favor of #21.