Add internal EC parsing functions that take lists of allowed groups
They're internal for now; I'm thinking we'll just have the public
versions of these at the EVP layer for now and try to move past this
mess where there's two versions of every API.
The API is mildly annoying in that both this caller and the one added
next will want to distinguish UNKNOWN_GROUP from other errors, but the
recent helper function makes checking the error queue not tooooo bad.
Bug: 42290364
Change-Id: Ibb5e3e643a9180459cc09b4559ee40696cea2688
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81650
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/ec/ec_asn1.cc b/crypto/ec/ec_asn1.cc
index 58f379c..3d54e11 100644
--- a/crypto/ec/ec_asn1.cc
+++ b/crypto/ec/ec_asn1.cc
@@ -17,6 +17,9 @@
#include <limits.h>
#include <string.h>
+#include <algorithm>
+#include <array>
+
#include <openssl/bn.h>
#include <openssl/bytestring.h>
#include <openssl/ec_key.h>
@@ -27,6 +30,7 @@
#include "../bytestring/internal.h"
#include "../fipsmodule/ec/internal.h"
#include "../internal.h"
+#include "internal.h"
static const CBS_ASN1_TAG kParametersTag =
@@ -34,17 +38,23 @@
static const CBS_ASN1_TAG kPublicKeyTag =
CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 1;
-// TODO(https://crbug.com/boringssl/497): Allow parsers to specify a list of
-// acceptable groups, so parsers don't have to pull in all four.
-typedef const EC_GROUP *(*ec_group_func)(void);
-static const ec_group_func kAllGroups[] = {
- &EC_group_p224,
- &EC_group_p256,
- &EC_group_p384,
- &EC_group_p521,
-};
+static auto get_all_groups() {
+ return std::array{
+ EC_group_p224(),
+ EC_group_p256(),
+ EC_group_p384(),
+ EC_group_p521(),
+ };
+}
-EC_KEY *EC_KEY_parse_private_key(CBS *cbs, const EC_GROUP *group) {
+EC_KEY *ec_key_parse_private_key(
+ CBS *cbs, const EC_GROUP *group,
+ bssl::Span<const EC_GROUP *const> allowed_groups) {
+ // If a group was supplied externally, no other groups can be parsed.
+ if (group != nullptr) {
+ allowed_groups = bssl::Span(&group, 1);
+ }
+
CBS ec_private_key, private_key;
uint64_t version;
if (!CBS_get_asn1(cbs, &ec_private_key, CBS_ASN1_SEQUENCE) ||
@@ -66,23 +76,31 @@
OPENSSL_PUT_ERROR(EC, EC_R_DECODE_ERROR);
return nullptr;
}
- const EC_GROUP *inner_group = EC_KEY_parse_parameters(&child);
+ const EC_GROUP *inner_group =
+ ec_key_parse_parameters(&child, allowed_groups);
if (inner_group == nullptr) {
+ // If the caller already supplied a group, any explicit group is required
+ // to match. On mismatch, |ec_key_parse_parameters| will fail to recognize
+ // any other groups, so remap the error.
+ if (group != nullptr &&
+ ERR_equals(ERR_peek_last_error(), ERR_LIB_EC, EC_R_UNKNOWN_GROUP)) {
+ ERR_clear_error();
+ OPENSSL_PUT_ERROR(EC, EC_R_GROUP_MISMATCH);
+ }
return nullptr;
}
- if (group == nullptr) {
- group = inner_group;
- } else if (EC_GROUP_cmp(group, inner_group, nullptr) != 0) {
- // If a group was supplied externally, it must match.
- OPENSSL_PUT_ERROR(EC, EC_R_GROUP_MISMATCH);
- return nullptr;
- }
+ // Overriding |allowed_groups| above ensures the only returned group will be
+ // the matching one.
+ assert(group == nullptr || inner_group == group);
+ group = inner_group;
if (CBS_len(&child) != 0) {
OPENSSL_PUT_ERROR(EC, EC_R_DECODE_ERROR);
return nullptr;
}
}
+ // The group must have been specified either externally, or explicitly in the
+ // structure.
if (group == nullptr) {
OPENSSL_PUT_ERROR(EC, EC_R_MISSING_PARAMETERS);
return nullptr;
@@ -151,6 +169,10 @@
return ret.release();
}
+EC_KEY *EC_KEY_parse_private_key(CBS *cbs, const EC_GROUP *group) {
+ return ec_key_parse_private_key(cbs, group, get_all_groups());
+}
+
int EC_KEY_marshal_private_key(CBB *cbb, const EC_KEY *key,
unsigned enc_flags) {
if (key == NULL || key->group == NULL || key->priv_key == NULL) {
@@ -296,23 +318,29 @@
return CBS_mem_equal(©, buf, CBS_len(©));
}
-EC_GROUP *EC_KEY_parse_curve_name(CBS *cbs) {
+const EC_GROUP *ec_key_parse_curve_name(
+ CBS *cbs, bssl::Span<const EC_GROUP *const> allowed_groups) {
CBS named_curve;
if (!CBS_get_asn1(cbs, &named_curve, CBS_ASN1_OBJECT)) {
OPENSSL_PUT_ERROR(EC, EC_R_DECODE_ERROR);
- return NULL;
+ return nullptr;
}
// Look for a matching curve.
- for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kAllGroups); i++) {
- const EC_GROUP *group = kAllGroups[i]();
- if (CBS_mem_equal(&named_curve, group->oid, group->oid_len)) {
- return (EC_GROUP *)group;
+ for (const EC_GROUP *group : allowed_groups) {
+ if (named_curve == bssl::Span(group->oid, group->oid_len)) {
+ return group;
}
}
OPENSSL_PUT_ERROR(EC, EC_R_UNKNOWN_GROUP);
- return NULL;
+ return nullptr;
+}
+
+EC_GROUP *EC_KEY_parse_curve_name(CBS *cbs) {
+ // This function only ever returns a static |EC_GROUP|, but currently returns
+ // a non-const pointer for historical reasons.
+ return const_cast<EC_GROUP *>(ec_key_parse_curve_name(cbs, get_all_groups()));
}
int EC_KEY_marshal_curve_name(CBB *cbb, const EC_GROUP *group) {
@@ -324,9 +352,10 @@
return CBB_add_asn1_element(cbb, CBS_ASN1_OBJECT, group->oid, group->oid_len);
}
-EC_GROUP *EC_KEY_parse_parameters(CBS *cbs) {
+const EC_GROUP *ec_key_parse_parameters(
+ CBS *cbs, bssl::Span<const EC_GROUP *const> allowed_groups) {
if (!CBS_peek_asn1_tag(cbs, CBS_ASN1_SEQUENCE)) {
- return EC_KEY_parse_curve_name(cbs);
+ return ec_key_parse_curve_name(cbs, allowed_groups);
}
// OpenSSL sometimes produces ECPrivateKeys with explicitly-encoded versions
@@ -348,8 +377,7 @@
return nullptr;
}
- for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kAllGroups); i++) {
- const EC_GROUP *group = kAllGroups[i]();
+ for (const EC_GROUP *group : allowed_groups) {
if (!integers_equal(&curve.order, EC_GROUP_get0_order(group))) {
continue;
}
@@ -372,13 +400,19 @@
!integers_equal(&curve.base_y, y.get())) {
break;
}
- return const_cast<EC_GROUP *>(group);
+ return group;
}
OPENSSL_PUT_ERROR(EC, EC_R_UNKNOWN_GROUP);
return nullptr;
}
+EC_GROUP *EC_KEY_parse_parameters(CBS *cbs) {
+ // This function only ever returns a static |EC_GROUP|, but currently returns
+ // a non-const pointer for historical reasons.
+ return const_cast<EC_GROUP *>(ec_key_parse_parameters(cbs, get_all_groups()));
+}
+
int EC_POINT_point2cbb(CBB *out, const EC_GROUP *group, const EC_POINT *point,
point_conversion_form_t form, BN_CTX *ctx) {
size_t len = EC_POINT_point2oct(group, point, form, NULL, 0, ctx);
@@ -543,13 +577,12 @@
size_t EC_get_builtin_curves(EC_builtin_curve *out_curves,
size_t max_num_curves) {
- if (max_num_curves > OPENSSL_ARRAY_SIZE(kAllGroups)) {
- max_num_curves = OPENSSL_ARRAY_SIZE(kAllGroups);
- }
+ auto all = get_all_groups();
+ max_num_curves = std::min(all.size(), max_num_curves);
for (size_t i = 0; i < max_num_curves; i++) {
- const EC_GROUP *group = kAllGroups[i]();
+ const EC_GROUP *group = all[i];
out_curves[i].nid = group->curve_name;
out_curves[i].comment = group->comment;
}
- return OPENSSL_ARRAY_SIZE(kAllGroups);
+ return all.size();
}
diff --git a/crypto/ec/internal.h b/crypto/ec/internal.h
index 8dbe4a7..0e4a5ae 100644
--- a/crypto/ec/internal.h
+++ b/crypto/ec/internal.h
@@ -17,6 +17,8 @@
#include <openssl/ec.h>
+#include <openssl/span.h>
+
#include "../fipsmodule/ec/internal.h"
#if defined(__cplusplus)
@@ -24,6 +26,31 @@
#endif
+// Parsing functions.
+
+// ec_key_parse_curve_name behaves like |EC_KEY_parse_curve_name| but only
+// supports the groups in |allowed_groups|. If no syntax errors were found but
+// the group is unknown, it will fail with an error of |EC_R_UNKNOWN_GROUP|.
+const EC_GROUP *ec_key_parse_curve_name(
+ CBS *cbs, bssl::Span<const EC_GROUP *const> allowed_groups);
+
+// ec_key_parse_parameters behaves like |EC_KEY_parse_parameters| but only
+// supports the groups in |allowed_groups|. If no syntax errors were found but
+// the group is unknown, it will fail with an error of |EC_R_UNKNOWN_GROUP|.
+const EC_GROUP *ec_key_parse_parameters(
+ CBS *cbs, bssl::Span<const EC_GROUP *const> allowed_groups);
+
+// ec_key_parse_private_key behaves like |EC_KEY_parse_private_key| but only
+// supports the groups in |allowed_groups|. If |group| is non-NULL,
+// |allowed_groups| is ignored and instead only |group| is supported.
+//
+// TODO(crbug.com/boringssl/414361735): This should return a bssl::UniquePtr,
+// but cannot until it is made C++ linkage.
+EC_KEY *ec_key_parse_private_key(
+ CBS *cbs, const EC_GROUP *group,
+ bssl::Span<const EC_GROUP *const> allowed_groups);
+
+
// Hash-to-curve.
//
// Internal |EC_JACOBIAN| versions of the corresponding public APIs.
