Add support for NIST P-256 curve #185

AlfioEmanueleFresta · 2024-09-28T22:00:31Z

Changes

Adds support for NIST P-256, aka secp256r1, using the p256 crate from RustCrypto. This is a pure-Rust implementation.
As per feedback in previous abandoned PR try to add secp256k1_support #38 (comment), the code is feature gated, as this is a non-standard curve.
Includes PR Update Cargo.toml #181 which updates curve25519-dalek, which unbreaks the build for me.

Motivation

This curve is used for the Noise handshake in CTAP2 Hybrid Transport. This is the protocol used for browsers to access FIDO2 credentials (aka Passkeys) on roaming authenticators connected over the internet, such as phones running Android and iOS.

I'm using snow in my implementation of CTAP2, aimed at bringing Passkeys to the Linux desktop, see xdg-credentials-portal.

Tests

All commands below succeed:

$ cargo build --all-targets
$ cargo build --all-targets --features p256
$ cargo test --all-targets
$ cargo test --all-targets --features p256

…nt) (#51) An initial rough implementation of hybrid transport - formerly known as Cloud Assisted Bluetooth Low Energy, or caBLE. ## Includes * Generates QR code for QR-initiated transactions * Adds WebAuthn example which prints QR code to terminal (`examples/webauthn_cable.rs`) * Adds submodule fork of `snow` (noise protocol framework) with NIST P-256 curve implementation, as required by CTAP spec (see mcginty/snow#185) ## Limitations and follow ups * Unable to discover Android devices (created issue #52) * Does not yet support state-assisted transactions (part of #31) * Clean up (part of #31)

AlfioEmanueleFresta · 2025-02-12T22:18:31Z

Friendy ping on this @mcginty - I would need this change to be upstreamed so we can begin publishing linux-credentials/libwebauthn to crates.io. TIA!

mcginty · 2025-02-13T16:33:28Z

@AlfioEmanueleFresta Wow, I'm so sorry I dropped the ball on this! Reviewing now.

mcginty

Thanks for the PR! This is looking pretty good, I just have some small comments/questions.

mcginty · 2025-02-13T16:34:43Z

src/constants.rs

@@ -4,7 +4,7 @@ pub const TAGLEN: usize = 16;

 pub const MAXHASHLEN: usize = 64;
 pub const MAXBLOCKLEN: usize = 128;
-pub const MAXDHLEN: usize = 56;
+pub const MAXDHLEN: usize = 65;


I think we should feature-gate this as well so we don't change any constants in the default state (although really our MAXDHLEN should be 32 by default here, since we don't support Curve448 as of yet, but that's another fix outside of this PR :)

Suggested change

pub const MAXDHLEN: usize = 65;

// P-256 uncompressed SEC-1 encodings are 65 bytes long, larger than the `MAXDHLEN` in the official Noise spec.

#[cfg(feature = "p256")]

pub const MAXDHLEN: usize = 65;

// Curve448 keys are the largest in the official Noise spec.

#[cfg(not(feature = "p256"))]

pub const MAXDHLEN: usize = 56;

mcginty · 2025-02-13T16:50:45Z

src/resolvers/default.rs

+#[cfg(feature = "p256")]
+#[derive(Default)]
+struct P256 {
+    privkey: [u8; 32],


Any reason not to store privkey as a p256::SecretKey here?

I gave this a try again, as I couldn't remember why - it seems it's not trivial:

error[E0515]: cannot return reference to temporary value --> src/resolvers/default.rs:241:9 | 241 | &self.pubkey.to_sec1_bytes() | ^--------------------------- | || | |temporary value created here | returns a reference to data owned by the current function

The issue is that the Dh trait requires exporting slices:

pub trait Dh: Send + Sync { fn pubkey(&self) -> &[u8]; fn privkey(&self) -> &[u8];

However, p256::SecretKey and p256::PublicKey only return owned types (eg. FieldBytes<...>, or GenericArray<u8, ...>).

Pre-serializing the keys to their byte representation allows keeping the trait unchanged.

I also took a quick look and agree this isn't a simple drop-in; the key types available only provide copied-array access to the inner bytes because they are stored as a "generic" field representation.

FWIW I have run into similar issues before with the use of slices for everything in the Dh implementations. It was when attempting to add support for ECDH directly in the ring provider because those also don't (at all) provide access to a slice representation of private keys.

Got it -- that makes sense to keep it as-is then!

I can address this more holistically in a large rework of this trait later. It doesn't make sense to keep these slices if they're holding us back in the long run.

mcginty · 2025-02-13T16:52:17Z

src/resolvers/default.rs

+        let scalar =
+            Vec::<u8>::from_hex("58c77a30bb0fa177286346d18f59678ac1b8d3637ee65f1bd88a8f52e49ef189")
+                .unwrap();
+        keypair.set(&scalar);
+        let public = Vec::<u8>::from_hex(
+            "042009fddefeed3b342696b11683b423db8ede2ef5cd66af9b7db2772f7deaf3d\
+                                 f1a69a4648d990ae2a4e5928f156f32e15fa08ba4465df8cb17838dc2afb719d2",
+        )
+        .unwrap();
+        let mut output = [0u8; 32];
+        keypair.dh(&public, &mut output).unwrap();
+        assert_eq!(
+            hex::encode(output),
+            "07505b308650d07e6ead11dd36bbf6f24bf99fc6479649fadd3939faa33ddeb3"
+        );


Where did you get these test constants from? It would be good to have a citation to a known-good example to derive trust for this implementation.

I honestly do not recall, so I've replaced these with the test vectors from RFC 5903, and added some comments to this effect.

mcginty

Thanks for the PR! This is looking pretty good, I just have some small comments/questions.

complexspaces · 2025-02-13T17:05:32Z

As a drive-by comment, which might be helpful context: I asked Trevor at RWC one year what his thoughts were on CTAP Hybrid being specified with a non-standard Noise curve, as the designer of Noise because I was curious, and the answer was that it wasn't a big deal these days and that maybe future versions of Noise would be more curve agnostic anyway.

Tl;dr I think this change is worthwhile for sure :)

mcginty · 2025-02-13T18:35:11Z

@complexspaces thanks for the context! Yeah, generally I'm open to snow supporting "unofficial" general-use Noise modes, as long as the default snow only exposes the officially supported modes and unofficial modifications are feature-gated, just as this PR does.

AlfioEmanueleFresta · 2025-02-14T23:48:32Z

Thanks for the review @mcginty! Addressed your comments.

@complexspaces that's interesting! FWIW, here are some reasons P-256 might have been chosen for the FIDO specs: fido-alliance/discussions#2 (comment)

complexspaces · 2025-02-15T01:26:29Z

@AlfioEmanueleFresta Yeah it makes a lot of sense why they picked P-256, including the reasons listed in your link. I don't think it was done harmfully to be clear, just out of necessity for what algorithms existing key APIs supported across OSes.

mcginty

Looks great, thank you very much for making the change and patience in needing to ping me to review it :).

* Adds support for NIST P-256, aka secp256r1, using the p256 crate from RustCrypto. This is a pure-Rust implementation. * As per feedback in previous abandoned PR try to add secp256k1_support #38 (comment), the code is feature gated, as this is a non-standard curve. * Includes PR Update Cargo.toml #181 which updates curve25519-dalek, which unbreaks the build for me.

PR mcginty/snow#185 was merged into snow v0.10.0-alpha.1, adding support for P-256. This is required by the CTAP Hybrid Transport spec. This PR deprecates the snow fork, in favour of the new upstream version containing my changes.

AlfioEmanueleFresta added 4 commits September 28, 2024 22:25

P-256: Update curve25519-dalek to unbreak build (see mcginty#181)

dddc80b

P-256: Add support via p256 crate

0b0f2b3

P-256: Move under 'p256' feature

60be571

P-256: Add DH test

8fb05c4

AlfioEmanueleFresta changed the title ~~Add support for p256~~ Add support for NIST P-256 curve Sep 28, 2024

P-256: DH: Use correct length for pub key vs DH

30c5166

AlfioEmanueleFresta force-pushed the main branch from d6d7176 to 30c5166 Compare September 29, 2024 14:46

AlfioEmanueleFresta mentioned this pull request Jan 5, 2025

Ref #31: Initial support for Hybrid transport (iOS only, non persistent) linux-credentials/libwebauthn#51

Merged

AlfioEmanueleFresta mentioned this pull request Feb 12, 2025

Publish crate to crates.io linux-credentials/libwebauthn#71

Open

7 tasks

mcginty requested changes Feb 13, 2025

View reviewed changes

Address PR review feedback

1e81a44

AlfioEmanueleFresta requested a review from mcginty February 14, 2025 23:48

AlfioEmanueleFresta marked this pull request as draft February 17, 2025 20:54

AlfioEmanueleFresta marked this pull request as ready for review February 17, 2025 20:55

mcginty approved these changes Feb 17, 2025

View reviewed changes

mcginty merged commit 656a56f into mcginty:main Feb 17, 2025
3 checks passed

AlfioEmanueleFresta mentioned this pull request Feb 18, 2025

Ref #71: Use upstreamed snow with P-256 support linux-credentials/libwebauthn#76

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for NIST P-256 curve #185

Add support for NIST P-256 curve #185

AlfioEmanueleFresta commented Sep 28, 2024 •

edited

Loading

AlfioEmanueleFresta commented Feb 12, 2025

mcginty commented Feb 13, 2025

mcginty left a comment

mcginty Feb 13, 2025 •

edited

Loading

mcginty Feb 13, 2025

AlfioEmanueleFresta Feb 14, 2025

complexspaces Feb 15, 2025

mcginty Feb 17, 2025 •

edited

Loading

mcginty Feb 13, 2025

AlfioEmanueleFresta Feb 14, 2025

mcginty left a comment

complexspaces commented Feb 13, 2025 •

edited

Loading

mcginty commented Feb 13, 2025

AlfioEmanueleFresta commented Feb 14, 2025

complexspaces commented Feb 15, 2025

mcginty left a comment

Add support for NIST P-256 curve #185

Add support for NIST P-256 curve #185

Conversation

AlfioEmanueleFresta commented Sep 28, 2024 • edited Loading

Changes

Motivation

Tests

AlfioEmanueleFresta commented Feb 12, 2025

mcginty commented Feb 13, 2025

mcginty left a comment

Choose a reason for hiding this comment

mcginty Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

mcginty Feb 13, 2025

Choose a reason for hiding this comment

AlfioEmanueleFresta Feb 14, 2025

Choose a reason for hiding this comment

complexspaces Feb 15, 2025

Choose a reason for hiding this comment

mcginty Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

mcginty Feb 13, 2025

Choose a reason for hiding this comment

AlfioEmanueleFresta Feb 14, 2025

Choose a reason for hiding this comment

mcginty left a comment

Choose a reason for hiding this comment

complexspaces commented Feb 13, 2025 • edited Loading

mcginty commented Feb 13, 2025

AlfioEmanueleFresta commented Feb 14, 2025

complexspaces commented Feb 15, 2025

mcginty left a comment

Choose a reason for hiding this comment

AlfioEmanueleFresta commented Sep 28, 2024 •

edited

Loading

mcginty Feb 13, 2025 •

edited

Loading

mcginty Feb 17, 2025 •

edited

Loading

complexspaces commented Feb 13, 2025 •

edited

Loading