SecurityDriven.Inferno icon indicating copy to clipboard operation
SecurityDriven.Inferno copied to clipboard

ECDiffieHellman code in NETCOREAPP2_1 is inefficient & complex - can it be improved?

Open sdrapkin opened this issue 7 years ago • 4 comments

ECDiffieHelman :: GetSharedSecret

@bartonjs The NETCOREAPP2_1 code to use ECDiffieHellman to derive a shared secret from a private CngKey and a public CngKey is very messy/complicated, compared to NET462 code. Can it be improved?

Ex. the NETCOREAPP2_1 code above creates 2 instances of ECDiffieHellman, instead of 1. There seems to be no other way to create ECDiffieHellmanPublicKey instance out of public-key blob/byte[], other than by creating an extra dummy ECDiffieHellman instance to get its .PublicKey, which is a lot of extra code.

sdrapkin avatar May 10 '18 19:05 sdrapkin

The ECDiffieHellmanCng type is there, with the same surface area. If you're using CngKey that means you're already bound to Windows and can just keep using it.

bartonjs avatar May 10 '18 19:05 bartonjs

@bartonjs Thx for reviewing. Is it by design that NETCOREAPP2_1 has no API which converts a byte array into ECDiffieHellmanPublicKey? Especially given than the reverse API that converts ECDiffieHellmanPublicKey into a byte array does exist (.PublicKey.ToByteArray)?

sdrapkin avatar May 10 '18 19:05 sdrapkin

Is it by design that NETCOREAPP2_1 has no API which converts a byte array into ECDiffieHellmanPublicKey?

Yes

Especially given than the reverse API that converts ECDiffieHellmanPublicKey into a byte array does exist (.PublicKey.ToByteArray)?

ECDiffieHellmanPublicKeyCng uses the CNG blob, which didn't seem like a sensible thing to move to cross-platform.

It's also a bit complicated by ECDiffieHellmanCng having expectations that it will get an ECDiffieHellmanPublicKeyCng (throwing otherwise), so really the API expects that the PublicKey object came from the parent class factory property.

I won't say that there's not room for improvement, but netcoreapp21 made ECDH be possible, if not nice :).

bartonjs avatar May 10 '18 19:05 bartonjs

I won't say that there's not room for improvement, but netcoreapp21 made ECDH be possible, if not nice :).

Indeed - no complaints here. In terms of future improvement though, we all want simple things:

  • "Standard" serialization/wire-format for ECDsa and ECDH keys, that works across all targets (ex. serialize in NETFULL and load back in NETCORE, or vice-versa).
  • Ability to serialize/deserialize ECDsa and ECDH public and private keys via convenient APIs. Preferably in a way that works on all NETCORE target platforms (ie. not CngKey-style).

CNGBLOB was not perfect, but it was something resembling a "standard MS approach":

internal enum KeyBlobMagicNumber : int
{
	BCRYPT_ECDH_PUBLIC_P256_MAGIC = 0x314B4345, // ECK1
	BCRYPT_ECDH_PRIVATE_P256_MAGIC = 0x324B4345, // ECK2
	BCRYPT_ECDH_PUBLIC_P384_MAGIC = 0x334B4345, // ECK3
	BCRYPT_ECDH_PRIVATE_P384_MAGIC = 0x344B4345, // ECK4
	BCRYPT_ECDH_PUBLIC_P521_MAGIC = 0x354B4345, // ECK5
	BCRYPT_ECDH_PRIVATE_P521_MAGIC = 0x364B4345, // ECK6
	BCRYPT_ECDH_PUBLIC_GENERIC_MAGIC = 0x504B4345, // ECKP
	BCRYPT_ECDH_PRIVATE_GENERIC_MAGIC = 0x564B4345, //ECKV

	BCRYPT_ECDSA_PUBLIC_P256_MAGIC = 0x31534345, // ECS1
	BCRYPT_ECDSA_PRIVATE_P256_MAGIC = 0x32534345, // ECS2
	BCRYPT_ECDSA_PUBLIC_P384_MAGIC = 0x33534345, // ECS3
	BCRYPT_ECDSA_PRIVATE_P384_MAGIC = 0x34534345, // ECS4
	BCRYPT_ECDSA_PUBLIC_P521_MAGIC = 0x35534345, // ECS5
	BCRYPT_ECDSA_PRIVATE_P521_MAGIC = 0x36534345,// ECS6
	BCRYPT_ECDSA_PUBLIC_GENERIC_MAGIC = 0x50444345, // ECDP
	BCRYPT_ECDSA_PRIVATE_GENERIC_MAGIC = 0x56444345, // ECDV
}

ECDiffieHellman and ECDsa in NETCORE import/export their keys as ECParameters, and that structure has no "default" serialization/deserialization APIs.

sdrapkin avatar May 10 '18 20:05 sdrapkin