Avoid validation of known EC groups when decoding an explicit curve block#5268
Avoid validation of known EC groups when decoding an explicit curve block#5268
Conversation
e995fc8 to
dd3337d
Compare
There was a problem hiding this comment.
Pull request overview
Improves performance of decoding explicitly-encoded EC domain parameters by first attempting to recognize them as a known/registered group (skipping expensive primality checks in the common case), and adds regression tests for explicit-curve decoding behavior.
Changes:
- Add a parameter-based lookup path (
lookup_from_params) to map explicit curve parameters to known EC groups before doing heavyweight validation. - Add an EC group parameter matcher that accepts the encoded base point.
- Add new text-based tests and vectors for explicit EC curve decoding/validation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/test_ecc_explicit_params.cpp | Adds a text-based test that loads explicit-curve public keys and checks acceptance/rejection behavior. |
| src/tests/data/pubkey/ecc_explicit_curve.vec | Adds test vectors for valid and invalid explicit curve encodings across multiple groups. |
| src/lib/pubkey/ec_group/ec_inner_data.h | Declares a new params_match overload that matches the encoded base point. |
| src/lib/pubkey/ec_group/ec_inner_data.cpp | Implements the new encoded-base-point matcher and refactors existing matcher. |
| src/lib/pubkey/ec_group/ec_group.cpp | Adds lookup_from_params and uses it early in explicit group decoding to avoid expensive validation for known groups. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
reneme
left a comment
There was a problem hiding this comment.
Great idea! Changes look good to me. We could reorder the parameter lookup a little to avoid unnecessary point serializations in unlikely failure cases. But given that the benefit of that would be reaped only in fail cases, I don't see that as more than a nit.
Some SEC1 encoder/decoder helper classes would probably be nice at this point, though.
| if(base_pt.size() == 1 + field_len && (base_pt[0] == 0x02 || base_pt[0] == 0x03)) { | ||
| // compressed |
There was a problem hiding this comment.
Given the number of times we jotted down the encoding/decoding of this SEC1 encoding across the code base now, I think we should introduce a small helper along those lines:
class SEC1_Decoder {
public:
enum Label {
Identity = 0x00;
Compressed_Even = 0x02;
Compressed_Odd = 0x03;
Full = 0x04;
// Hybrid?
};
public:
static SEC1_Decoder from_bytes(const EC_Group& group, std::span<const uint8_t> bytes);
static SEC1_Decoder from_bytes(size_t field_len, std::span<const uint8_t> bytes);
Label label() const { /* ... */ }
std::span<const uint8_t> x_bytes() const { /* ... */ }
std::optional<std::span<const uint8_t>> y_bytes() cosnt { /* ... */ }
EC_AffinePoint affine_point() const;
private:
std::span<const uint8_t> m_bytes;
};... certainly a follow-up, though.
|
Oh, and the CI failure is relevant. I guess it tries to instantiate a standard curve in a build where there's no backend for them. Perhaps try deserializing in a |
dd3337d to
53ec20d
Compare
…lock Instead first check if the group is a known group, either a group known at compile time or something that the application registered. Only if that fails, perform the usual validation. This skips various expensive primality checks for the common case when the explicit curve is just an explicit encoding of a standard group. It also sets up for easily removing support for non-standard explicit groups; if lookup_from_params fails, instead of trying to validate the group just throw an exception.
53ec20d to
b325234
Compare
Instead first check if the group is a known group, either a group known at compile time or something that the application registered. Only if that fails, perform the usual validation.
This skips various expensive primality checks for the common case when the explicit curve is just an explicit encoding of a standard group. It also sets up for easily removing support for non-standard explicit groups; if lookup_from_params fails, instead of trying to validate the group just throw an exception.