Unfriendly exceptions are thrown when a malformed p256dh key is provided

[GurGaller]

Let's consider the following example:

public static async Task Main(string[] args)
{
    var vapidDetails = VapidHelper.GenerateVapidKeys();
    vapidDetails.Subject = "http://example.com";

    var publicKey = Guid.NewGuid().ToString();
    var auth = "_DAk3a6DeEFIcxqV2_Ozow";
    var subscription = new PushSubscription("http://example.com", publicKey, auth);

    await new WebPushClient().SendNotificationAsync(subscription, "payload", vapidDetails);
}

This throws the following exception:

Unhandled exception. System.FormatException: Invalid point encoding 119
   at Org.BouncyCastle.Math.EC.ECCurve.DecodePoint(Byte[] encoded) in C:\projects\bc-csharp\crypto\src\math\ec\ECCurve.cs:line 463
   at Org.BouncyCastle.Asn1.X9.X9ECPoint.get_Point() in C:\projects\bc-csharp\crypto\src\asn1\x9\X9ECPoint.cs:line 51
   at Org.BouncyCastle.Security.PublicKeyFactory.CreateKey(SubjectPublicKeyInfo keyInfo) in C:\projects\bc-csharp\crypto\src\security\PublicKeyFactory.cs:line 141
   at Org.BouncyCastle.Security.PublicKeyFactory.CreateKey(Byte[] keyInfoData) in C:\projects\bc-csharp\crypto\src\security\PublicKeyFactory.cs:line 30
   at Org.BouncyCastle.OpenSsl.PemReader.ReadPublicKey(PemObject pemObject) in C:\projects\bc-csharp\crypto\src\openssl\PEMReader.cs:line 139
   at Org.BouncyCastle.OpenSsl.PemReader.ReadObject() in C:\projects\bc-csharp\crypto\src\openssl\PEMReader.cs:line 102
   at WebPush.Util.ECKeyHelper.GetPublicKey(Byte[] publicKey)
   at WebPush.Util.Encryptor.Encrypt(Byte[] userKey, Byte[] userSecret, Byte[] payload)
   at WebPush.Util.Encryptor.Encrypt(String userKey, String userSecret, String payload)
   at WebPush.WebPushClient.GenerateRequestDetails(PushSubscription subscription, String payload, Dictionary`2 options)
   at WebPush.WebPushClient.SendNotificationAsync(PushSubscription subscription, String payload, Dictionary`2 options)
   at WebPush.WebPushClient.SendNotificationAsync(PushSubscription subscription, String payload, VapidDetails vapidDetails)
   at ConsoleApp8.Program.Main(String[] args) in C:\Users\1\source\repos\ConsoleApp8\ConsoleApp8\Program.cs:line 18
   at ConsoleApp8.Program.<Main>(String[] args)

And if we use a different malformed key, we'll get a different exception:

public static async Task Main(string[] args)
{
    var vapidDetails = VapidHelper.GenerateVapidKeys();
    vapidDetails.Subject = "http://example.com";

    var publicKey = "BIbObZY0YfPEvdiHd_fn73ULFo8jlyx94xT_KaJcl_wGigVjyoipl7pUykh3p1xibPVIwPGLcHZMeA1ekynpEa";
    var auth = "_DAk3a6DeEFIcxqV2_Ozow";
    var subscription = new PushSubscription("http://example.com", publicKey, auth);

    await new WebPushClient().SendNotificationAsync(subscription, "payload", vapidDetails);
}

Unhandled exception. System.ArgumentException: Incorrect length for uncompressed encoding (Parameter 'encoded')
   at Org.BouncyCastle.Math.EC.ECCurve.DecodePoint(Byte[] encoded) in C:\projects\bc-csharp\crypto\src\math\ec\ECCurve.cs:line 437
   at Org.BouncyCastle.Asn1.X9.X9ECPoint.get_Point() in C:\projects\bc-csharp\crypto\src\asn1\x9\X9ECPoint.cs:line 51
   at Org.BouncyCastle.Security.PublicKeyFactory.CreateKey(SubjectPublicKeyInfo keyInfo) in C:\projects\bc-csharp\crypto\src\security\PublicKeyFactory.cs:line 141
   at Org.BouncyCastle.Security.PublicKeyFactory.CreateKey(Byte[] keyInfoData) in C:\projects\bc-csharp\crypto\src\security\PublicKeyFactory.cs:line 30
   at Org.BouncyCastle.OpenSsl.PemReader.ReadPublicKey(PemObject pemObject) in C:\projects\bc-csharp\crypto\src\openssl\PEMReader.cs:line 139
   at Org.BouncyCastle.OpenSsl.PemReader.ReadObject() in C:\projects\bc-csharp\crypto\src\openssl\PEMReader.cs:line 102
   at WebPush.Util.ECKeyHelper.GetPublicKey(Byte[] publicKey)
   at WebPush.Util.Encryptor.Encrypt(Byte[] userKey, Byte[] userSecret, Byte[] payload)
   at WebPush.Util.Encryptor.Encrypt(String userKey, String userSecret, String payload)
   at WebPush.WebPushClient.GenerateRequestDetails(PushSubscription subscription, String payload, Dictionary`2 options)
   at WebPush.WebPushClient.SendNotificationAsync(PushSubscription subscription, String payload, Dictionary`2 options)
   at WebPush.WebPushClient.SendNotificationAsync(PushSubscription subscription, String payload, VapidDetails vapidDetails)
   at ConsoleApp8.Program.Main(String[] args) in C:\Users\1\source\repos\ConsoleApp8\ConsoleApp8\Program.cs:line 17
   at ConsoleApp8.Program.<Main>(String[] args)

I think this is worth fixing as the key comes from the client, so we can't trust it to be well-formed. Ideally, there will also be a way to validate the key easily when the client provides it, but at the very least I think the SendNotification method should throw a friendly exception that we can catch and deal with instead of crushing the thread.

May I open a pull request with a fix?

[coryjthompson]

That's an excellent idea. I would definitely be interested in a pull request.