Point validation

[frederikbosch]

Question Checklist

• I searched for existing GitHub issues

Question Subject

In the swift-paseto package we were discussing the initialization of P384.Signing.Publickey. While this is SwiftCrypto and not CryptoKit, I was hoping you could answer our question anyhow. I have no clue where to direct our question otherwise, and so I hope you do not mind me opening this question here.

couldn't find (and am still having trouble finding) documentation about whether or not Apple's CryptoKit does point validation when constructing the PublicKey type.

Question Description

So the question comes down to: do you know if the following line does any point validation? I think this is a valid question for this library and for CryptoKit.

let publicKey = P384.Signing.Publickey.init(pemRepresentation: publicKeyPem)

[remko]

I think I asked a related question yesterday specifically on CryptoKit, and filed a documentation request in response. I'm interested in the answer for Crypto too.

[Lukasa]

Thanks for filing this! @remko has kindly already received an answer on the developer forums, which is a better venue for this question, so I'll defer to that answer. To close the loop, swift-crypto's BoringSSL backend does point validation as well.

[aidantwoods]

To be clear, when does the point validation occur, and what does it include?

Mostly what I'm interested in is validating that points are actually on the curve when constructing a public key. I wrote a test to check that my library can catch this, because it seems that public key objects with points not on the curve will be created in CryptoKit.

To test if the point is on the curve I am exporting the raw representation of the key and the compressed representation of the key. I then use the compressed representation to construct a new key (which forces a point on the curve to be found). If the exported raw representation of this new key is not the same as the original, then a different point was recovered from compression (and so the original point was therefore not on the curve).

[Lukasa]

I can't speak to the behaviour of CryptoKit in this context I'm afraid, that's a topic for the developer forums. In the BoringSSL-backed implementation the call stack that does this validation is:

swift-crypto/Sources/Crypto/Key Agreement/ECDH.swift

Lines 314 to 316 in a1b8005

	public init<Bytes: ContiguousBytes>(x963Representation: Bytes) throws {
	impl = try NISTCurvePublicKeyImpl(x963Representation: x963Representation)
	}

using

swift-crypto/Sources/Crypto/Key Agreement/ECDH.swift

Line 21 in a1b8005

typealias NISTCurvePublicKeyImpl = OpenSSLNISTCurvePublicKeyImpl

calls

swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/NISTCurvesKeys_boring.swift

Lines 94 to 96 in a1b8005

	init<Bytes: ContiguousBytes>(x963Representation: Bytes) throws {
	self.key = try BoringSSLECPublicKeyWrapper(x963Representation: x963Representation)
	}

calls

swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/NISTCurvesKeys_boring.swift

Lines 360 to 376 in a1b8005

	init<Bytes: ContiguousBytes>(x963Representation bytes: Bytes) throws {
	// Before we do anything, we validate that the x963 representation has the right number of bytes.
	// This is because BoringSSL will quietly accept shorter byte counts, though it will reject longer ones.
	// This brings our behaviour into line with CryptoKit
	let group = Curve.group
	let length = bytes.withUnsafeBytes { $0.count }
	switch length {
	case (group.coordinateByteCount * 2) + 1:
	var (x, y) = try bytes.readx963PublicNumbers()
	self.key = try group.makeUnsafeOwnedECKey()
	try self.setPublicKey(x: &x, y: &y)
	default:
	throw CryptoKitError.incorrectParameterSize
	}
	}

calls

swift-crypto/Sources/Crypto/Keys/EC/BoringSSL/NISTCurvesKeys_boring.swift

Lines 493 to 503 in a1b8005

	func setPublicKey(x: inout ArbitraryPrecisionInteger, y: inout ArbitraryPrecisionInteger) throws {
	try x.withUnsafeMutableBignumPointer { xPointer in
	try y.withUnsafeMutableBignumPointer { yPointer in
	// This function is missing some const declarations here, which is why we need the bignums inout.
	// If that gets fixed, we can clean this function up.
	guard CCryptoBoringSSL_EC_KEY_set_public_key_affine_coordinates(self.key, xPointer, yPointer) != 0 else {
	throw CryptoKitError.internalBoringSSLError()
	}
	}
	}
	}

calls

swift-crypto/Sources/CCryptoBoringSSL/crypto/fipsmodule/ec/ec_key.c

Lines 372 to 395 in a1b8005

	int EC_KEY_set_public_key_affine_coordinates(EC_KEY *key, const BIGNUM *x,
	const BIGNUM *y) {
	EC_POINT *point = NULL;
	int ok = 0;
	if (!key || !key->group || !x || !y) {
	OPENSSL_PUT_ERROR(EC, ERR_R_PASSED_NULL_PARAMETER);
	return 0;
	}
	point = EC_POINT_new(key->group);
	if (point == NULL ||
	!EC_POINT_set_affine_coordinates_GFp(key->group, point, x, y, NULL) ||
	!EC_KEY_set_public_key(key, point) ||
	!EC_KEY_check_key(key)) {
	goto err;
	}
	ok = 1;
	err:
	EC_POINT_free(point);
	return ok;
	}

calls

swift-crypto/Sources/CCryptoBoringSSL/crypto/fipsmodule/ec/ec_key.c

Lines 294 to 330 in a1b8005

	int EC_KEY_check_key(const EC_KEY *eckey) {
	if (!eckey || !eckey->group || !eckey->pub_key) {
	OPENSSL_PUT_ERROR(EC, ERR_R_PASSED_NULL_PARAMETER);
	return 0;
	}
	if (EC_POINT_is_at_infinity(eckey->group, eckey->pub_key)) {
	OPENSSL_PUT_ERROR(EC, EC_R_POINT_AT_INFINITY);
	return 0;
	}
	// Test whether the public key is on the elliptic curve.
	if (!EC_POINT_is_on_curve(eckey->group, eckey->pub_key, NULL)) {
	OPENSSL_PUT_ERROR(EC, EC_R_POINT_IS_NOT_ON_CURVE);
	return 0;
	}
	// Check the public and private keys match.
	//
	// NOTE: this is a FIPS pair-wise consistency check for the ECDH case. See SP
	// 800-56Ar3, page 36.
	if (eckey->priv_key != NULL) {
	EC_RAW_POINT point;
	if (!ec_point_mul_scalar_base(eckey->group, &point,
	&eckey->priv_key->scalar)) {
	OPENSSL_PUT_ERROR(EC, ERR_R_EC_LIB);
	return 0;
	}
	if (!ec_GFp_simple_points_equal(eckey->group, &point,
	&eckey->pub_key->raw)) {
	OPENSSL_PUT_ERROR(EC, EC_R_INVALID_PRIVATE_KEY);
	return 0;
	}
	}
	return 1;
	}

which performs the point validation in the various functions it calls, specifically confirming that the point is not the point at infinity and that it is the point on the curve.

[aidantwoods]

Thanks!

It would seem that CryptoKit is behaving differently to that, which is a tad concerning 😬 Guess we'll need to chase that on the dev forums.