Validate implementation of ECIES

Question

I've created a basic implementation of ECIES (Elliptic Curve Integrated Encryption Scheme) based on http://www.secg.org/sec1-v2.pdf section 5.1.

/// <summary>
/// Simple implementation of ECIES (Elliptic Curve Integrated Encryption Scheme) based on http://www.secg.org/sec1-v2.pdf, section 5.1
/// The KDF, cipher and HMAC are fixed as ANSI-X9.63-KDF, AES-256-CBC and HMAC–SHA-256–256 respectively
/// Note this implementation does not use the optional SharedInfo1 & SharedInfo2 parameters
/// </summary>
public static class Ecies
{
 /// <summary>
 /// Based on http://www.secg.org/sec1-v2.pdf, section 5.1.3
 /// Encrypt data using ECIES (Elliptic Curve Integrated Encryption Scheme)
 /// </summary>
 /// <param name="recipientPubKey">Public key of the recipient</param>
 /// <param name="m">M, the message to be encrypted</param>
 /// <returns>(R̄, EM, D̄), the elliptic curve parameters, encrypted message and HMAC</returns>
 public static (byte[] rBar, byte[] em, byte[] d) Encrypt(ECDiffieHellmanPublicKey recipientPubKey, byte[] m)
 {
 var curve = recipientPubKey.ExportParameters().Curve;
 // Generate an ephemeral keypair on the correct curve
 using (var ephemeral = ECDiffieHellman.Create(curve))
 {
 // R̄ (rBar) contains the parameters to be used for encryption/decryption operations
 var ephemPublicParams = ephemeral.ExportParameters(false);
 var pointLen = ephemPublicParams.Q.X.Length;
 byte[] rBar = new byte[pointLen * 2 + 1];
 rBar[0] = 0x04;
 Buffer.BlockCopy(ephemPublicParams.Q.X, 0, rBar, 1, pointLen);
 Buffer.BlockCopy(ephemPublicParams.Q.Y, 0, rBar, 1 + pointLen, pointLen);
 // Use ANSI-X9.63-KDF to derive the encryption key, EK
 var ek = ephemeral.DeriveKeyFromHash(recipientPubKey, HashAlgorithmName.SHA256, null, new byte[] {0, 0, 0, 1});
 // Use ANSI-X9.63-KDF to derive the HMAC key, MK
 var mk = ephemeral.DeriveKeyFromHash(recipientPubKey, HashAlgorithmName.SHA256, null, new byte[] {0, 0, 0, 2});
 // The ciphertext, EM
 byte[] em;
 // Use AES-256-CBC to encrypt the message
 // Note we use an empty IV - this is OK, as the key is never reused
 using (var aes = Aes.Create())
 using (var encryptor = aes.CreateEncryptor(ek, new byte[16]))
 {
 if (!encryptor.CanTransformMultipleBlocks)
 throw new InvalidOperationException();
 em = encryptor.TransformFinalBlock(m, 0, m.Length);
 }
 // Use HMAC–SHA-256–256 to compute D, HMAC of the ciphertext
 byte[] d;
 using (HMAC hmac = new HMACSHA256(mk))
 {
 d = hmac.ComputeHash(em);
 }
 return (rBar, em, d);
 }
 }
 /// <summary>
 /// Based on http://www.secg.org/sec1-v2.pdf, section 5.1.4
 /// Encrypt data using ECIES (Elliptic Curve Integrated Encryption Scheme)
 /// </summary>
 /// <param name="recipient">Recipient of the message</param>
 /// <param name="rBar">R̄, elliptic curve parameters to be used for decryption</param>
 /// <param name="em">EM, the ciphertext to be decrypted</param>
 /// <param name="d">D, HMAC of the ciphertext</param>
 /// <returns>M, the decrypted message</returns>
 public static byte[] Decrypt(ECDiffieHellman recipient, byte[] rBar, byte[] em, byte[] d)
 {
 // Convert R̄ to an elliptic curve point R=(xR, yR)
 var r = new ECParameters
 {
 Curve = recipient.ExportParameters(false).Curve,
 Q =
 {
 X = rBar.Skip(1).Take(32).ToArray(),
 Y = rBar.Skip(33).Take(32).ToArray(),
 }
 };
 r.Validate();
 // M, the plaintext
 byte[] m;
 using (var senderEcdh = ECDiffieHellman.Create(r))
 {
 // Use ANSI-X9.63-KDF to derive the encryption key, EK
 var ek = recipient.DeriveKeyFromHash(senderEcdh.PublicKey, HashAlgorithmName.SHA256, null, new byte[] {0, 0, 0, 1});
 // Use ANSI-X9.63-KDF to derive the HMAC key, MK
 var mk = recipient.DeriveKeyFromHash(senderEcdh.PublicKey, HashAlgorithmName.SHA256, null, new byte[] {0, 0, 0, 2});
 // Use HMAC–SHA-256–256 to verify that the HMAC matches D
 using (HMAC verify = new HMACSHA256(mk))
 {
 if (!verify.ComputeHash(em).SequenceEqual(d))
 throw new CryptographicException("Invalid HMAC");
 }
 // Use AES-256-CBC to decrypt the message
 using (var aes = Aes.Create())
 using (var encryptor = aes.CreateDecryptor(ek, new byte[16]))
 {
 if (!encryptor.CanTransformMultipleBlocks)
 throw new InvalidOperationException();
 m = encryptor.TransformFinalBlock(em, 0, em.Length);
 }
 }
 return m;
 }
}

I tested it using:

var alice = ECDiffieHellman.Create(ECCurve.NamedCurves.nistP256);
var bob = ECDiffieHellman.Create(ECCurve.NamedCurves.nistP256);
var encrypted = Ecies.Encrypt(bob.PublicKey, Encoding.UTF8.GetBytes(message));
var decrypted = Ecies.Decrypt(bob, encrypted.rBar, encrypted.em, encrypted.d);
var result = Encoding.UTF8.GetString(decrypted);

I'm able to encrypt/decrypt messages as expected. I also tried using ECC certificates, getting the ECDH object as so, and it worked as expected:

using (var ecdsa = cert.GetECDsaPrivateKey())
{
 return ECDiffieHellman.Create(ecdsa.ExportParameters(true));
}

So, seemingly all good! But crypto implementations are fraught with danger, and the spec I linked to was hard reading, so more eyes on it would be very welcome - does the implementation look correct?

Also, how might the API change if different KDF functions, cipher functions and HMAC functions were to be supported?

EDIT Created an updated version as a gist, based on feedback.

Answer 1

• Naming guidelines are in: For returning named tuples use PascalCased identifiers: https://github.com/dotnet/corefx/issues/33553#issuecomment-531420515
• Usage guidelines are in: Unless you're named something like GetOffsetAndLength, don't use tuples (Tuple<T1, T2, ...>, ValueTuple<T1, T2, ...> or (T1 T1, T2 T2, ...) as a return type. (Same link)

 public static byte[] Decrypt(ECDiffieHellman recipient, byte[] rBar, byte[] em, byte[] d)
 {
 // Convert R̄ to an elliptic curve point R=(xR, yR)
 var r = new ECParameters
 {
 Curve = recipient.ExportParameters(false).Curve,
 Q =
 {
 X = rBar.Skip(1).Take(32).ToArray(),
 Y = rBar.Skip(33).Take(32).ToArray(),
 }
 };

• You should really validate the inputs. Too short of rBar throws a weird exception unrelated to the parameter names. Too long of rBar has the trailing bytes ignored. You also didn't check it was type 0x04 (uncompressed point).

if (recipient == null)
 throw new ArgumentNullException(nameof(recipient));
if (rBar == null)
 throw new ArgumentNullException(nameof(rBar));
if (rBar.Length != 65)
 throw new ArgumentOutOfRangeException(nameof(rBar), ...);
if (em == null)
 throw new ArgumentNullException(nameof(em));
if (d == null)
 throw new ArgumentNullException(nameof(d));

• Also, rather than naming them from the forumlae, you should give the parameters more standard identifier names.

public static byte[] Decrypt(
 ECDiffieHellman recipient,
 byte[] encodedPoint,
 byte[] ciphertext,
 byte[] mac)

In decrypt you read the senderEcdh.PublicKey parameter twice, you should save it once as a local. Since a) looking at the implementation shows that it returns a new object every time (and thus shouldn't have been a property, but oh, well) and b) you've created the parent object; you should also have it in a using (Dispose) statement.

using (var senderEcdh = ECDiffieHellman.Create(r))
using (var senderPublicKey = senderEcdh.PublicKey))
{
 ...
}

(Note: I continued using var here, since you did, but the only var that would be permitted in the BCL is r, since it's the only variable declaration with an enforced expression type)

• The { 0x00, 0x00, 0x00, 0x01 } and { 0x00, 0x00, 0x00, 0x02 } arrays are being created every time in encrypt and decrypt. They could both be static readonly.

Also, how might the API change if different KDF functions

Two answers appear off the top of my head:

1) make a class structure for ECIES KDFs. Using a Span-writing one (so destination and length are the same parameter) it'd be something like protected abstract void DeriveKey(ECDiffieHellman privateEcdh, ECDiffieHellmanPublicKey publicEcdh, Span<byte> destination). This lets the SharedInfo stuff be ctor parameters / state.

2) Add a bunch of parameters.

... and HMAC functions

Presumably accepting a HashAlgorithmName value for the MAC algorithm, then using IncrementalHash.CreateForHMAC to later build the HMAC calculator from the identifier.

, cipher functions [and key sizes]

There's not a strong precedent in .NET for this. PbeEncryptionAlgorithm exists for password-based encryption (used for ExportEncryptedPkcs8PrivateKey). The answer is probably a custom enum... or passing in a SymmetricAlgorithm instance whose key will get updated, but KeySize, padding, and mode are respected. Taking a SymmetricAlgorithm instance to modify isn't really common, either, though.

• Oh, and the class name is something that would get debated for a long time in API Review. In general, initialisms, abbreviations, and acronyms are frowned upon.

---

public readonly struct EciesResults
{
 public byte[] EncodedEphemeralPoint { get; }
 public byte[] Tag { get; }
 public byte[] Ciphertext { get; }
 public EciesResult(byte[] encodedEphemeralPoint, byte[] tag, byte[] ciphertext)
 {
 if (encodedEphemeralPoint == null)
 throw new ArgumentNullException(nameof(encodedEphemeralPoint));
 if (tag == null)
 throw new ArgumentNullException(nameof(tag));
 if (ciphertext == null)
 throw new ArgumentNullException(nameof(ciphertext));
 EncodedEphemeralPoint = encodedEphemeralPoint;
 Tag = tag;
 Ciphertext = ciphertext;
 }
}
public static class Ecies
{
 public static EciesResult Encrypt(
 ECDiffieHellmanPublicKey recipient,
 byte[] plaintext)
 {
 if (recipient == null)
 throw new ArgumentNullException(nameof(recipient));
 if (plaintext == null)
 throw new ArgumentNullException(nameof(plaintext));
 ...
 return new EciesResult(rBar, d, em);
 }
 public static byte[] Decrypt(ECDiffieHellman recipient, EciesResult encryptionResult)
 {
 if (recipient == null)
 throw new ArgumentNullException(nameof(recipient));
 if (encryptionResult.Tag == null)
 throw new ArgumentException("EciesResult must have values", nameof(encryptionResult));
 ECParameters ecParameters = recipient.ExportParameters(false);
 int curveSize = ecParameters.G.X.Length;
 if (encryptionResult.EncodedEphemeralPoint.Length != curveSize * 2 + 1)
 {
 throw new ArgumentException(
 "The EciesResult encoded point length is not appropriate for the recipient curve.",
 nameof(encryptionResult));
 }
 if (encryptionResult.EncodedEphemeralPoint[0] != 0x04)
 {
 throw new ArgumentException(
 "The EciesResult encoded point is not in the correct format.",
 nameof(encryptionResult));
 }
 var r = new ECParameters
 {
 Curve = recipient.ExportParameters(false).Curve,
 Q =
 {
 X = rBar.Skip(1).Take(curveSize).ToArray(),
 Y = rBar.Skip(1 + curveSize).ToArray(),
 }
 };
 r.Validate();
 using (ECDiffieHellman senderEcdh = ECDiffieHellman.Create(r))
 using (ECDiffieHellmanPublickey sender = senderEcdh.PublicKey)
 {
 ...
 return decryptor.TransformFinalBlock(em, 0, em.Length);
 }
 }
}
```

• Cocowalla: Thanks, there's a lot to take in here! Regarding naming, I fully agree; I usually do use PascalCase for tuple item names, and I usually use more "helpful" variable names - I like to stick to the names given in the spec when first implementing any crypto tho, as it helps as a reference when I'm constantly going back and forth from spec to code.
• Cocowalla: The class name I used had zero thought behind it yet, I just wanted a static class to encapsulate everything for now while I played with it :) Do you think it should handle the optional "shared info" arguments from the spec too? How do you feel about APIs that take X509Certificate2 instances? (my feeling is that will be a common use case, in which case it makes sense for usage to be as simple as possible; not sure if there is any precedent for that tho?)
• bartonjs: @Cocowalla The questions get different answers if they're "in .NET" (part of netstandard.dll / the .NET Core shared runtime) or "on .NET" (a personal library or a NuGet package). For in .NET: Yes, it's absolutely required to somehow deal with SharedInfo (probably the ECIES-KDF class approach). No, it can't take certs, because it belongs in S.S.C.Algorithms, a layer below certificates. For on .NET: Your choice :smile:.
• Cocowalla: Hah, fair point :) I'll give you your feedback some more thought tomorrow, and aim to open a proposal issue in github by end of week - thanks again for your help!
• Maarten Bodewes: Using the variable names from the standard I think seems a valid option to me, as long as you clearly reference that standard as the base. Otherwise you'd have to create a mapping of some kind in the comments. Making a simple statement in the comments that the variable names are taken from the standard is of course recommended - if you go that route.