From 073956966adee1973d3818b64ea117bad21b681b Mon Sep 17 00:00:00 2001
From: Alexander Block <ablock84@gmail.com>
Date: Tue, 30 Oct 2018 16:44:45 +0100
Subject: [PATCH] Better error handling for invalid public keys and signatures
 (#41)

* Throw error when ep_upk/ep2_upk fail

* Reset error code before throwing C++ exception

Otherwise it will keep throwing on the next CheckRelicErrors call.

* Call CheckRelicErrors after trying to read public keys or signatures

Before this, invalid public keys and signatures were silently ignored.

* Add test cases for error handling

Also move "Should throw on a bad private key" into new test case
---
 contrib/relic/src/ep/relic_ep_util.c   |  4 +-
 contrib/relic/src/epx/relic_ep2_util.c |  4 +-
 src/bls.cpp                            |  1 +
 src/publickey.cpp                      |  1 +
 src/signature.cpp                      |  1 +
 src/test.cpp                           | 58 ++++++++++++++++++++------
 6 files changed, 54 insertions(+), 15 deletions(-)

diff --git a/contrib/relic/src/ep/relic_ep_util.c b/contrib/relic/src/ep/relic_ep_util.c
index ce205ab3cd8455..75d6d8a8a9eae5 100755
--- a/contrib/relic/src/ep/relic_ep_util.c
+++ b/contrib/relic/src/ep/relic_ep_util.c
@@ -294,7 +294,9 @@ void ep_read_bin(ep_t a, const uint8_t *bin, int len) {
 				THROW(ERR_NO_VALID);
 				break;
 		}
-		ep_upk(a, a);
+		if (!ep_upk(a, a)) {
+			THROW(ERR_NO_VALID);
+		}
 	}
 
 	if (len == 2 * FP_BYTES + 1) {
diff --git a/contrib/relic/src/epx/relic_ep2_util.c b/contrib/relic/src/epx/relic_ep2_util.c
index 7c23aa27788ab0..0bdb689a0fdcf6 100755
--- a/contrib/relic/src/epx/relic_ep2_util.c
+++ b/contrib/relic/src/epx/relic_ep2_util.c
@@ -278,7 +278,9 @@ void ep2_read_bin(ep2_t a, uint8_t *bin, int len) {
 				THROW(ERR_NO_VALID);
 				break;
 		}
-		ep2_upk(a, a);
+		if (!ep2_upk(a, a)) {
+			THROW(ERR_NO_VALID);
+		}
 	}
 
 	if (len == 4 * FP_BYTES + 1) {
diff --git a/src/bls.cpp b/src/bls.cpp
index 7a0cb42dbcd2e0..6e481e2ee146dc 100644
--- a/src/bls.cpp
+++ b/src/bls.cpp
@@ -105,6 +105,7 @@ void BLS::CheckRelicErrors() {
         throw std::string("Library not initialized properly. Call BLS::Init()");
     }
     if (core_get()->code != STS_OK) {
+        core_get()->code = STS_OK;
         throw std::string("Relic library error");
     }
 }
diff --git a/src/publickey.cpp b/src/publickey.cpp
index d5cee9e4051ebe..bb466aa2190476 100644
--- a/src/publickey.cpp
+++ b/src/publickey.cpp
@@ -33,6 +33,7 @@ PublicKey PublicKey::FromBytes(const uint8_t * key) {
         uncompressed[0] = 0x02;   // Insert extra byte for Y=0
     }
     g1_read_bin(pk.q, uncompressed, PUBLIC_KEY_SIZE + 1);
+    BLS::CheckRelicErrors();
     return pk;
 }
 
diff --git a/src/signature.cpp b/src/signature.cpp
index f1ffe863f3b16b..d66ff6395aace2 100644
--- a/src/signature.cpp
+++ b/src/signature.cpp
@@ -34,6 +34,7 @@ InsecureSignature InsecureSignature::FromBytes(const uint8_t *data) {
         uncompressed[0] = 0x02;   // Insert extra byte for Y=0
     }
     g2_read_bin(sigObj.sig, uncompressed, SIGNATURE_SIZE + 1);
+    BLS::CheckRelicErrors();
     return sigObj;
 }
 
diff --git a/src/test.cpp b/src/test.cpp
index 056a9405fa87a4..d7dc6fb7b58452 100644
--- a/src/test.cpp
+++ b/src/test.cpp
@@ -193,6 +193,51 @@ TEST_CASE("Key generation") {
     }
 }
 
+TEST_CASE("Error handling") {
+    SECTION("Should throw on a bad private key") {
+        uint8_t seed[32];
+        getRandomSeed(seed);
+        PrivateKey sk1 = PrivateKey::FromSeed(seed, 32);
+        uint8_t* skData = Util::SecAlloc<uint8_t>(
+                Signature::SIGNATURE_SIZE);
+        sk1.Serialize(skData);
+        skData[0] = 255;
+        REQUIRE_THROWS(PrivateKey::FromBytes(skData));
+
+        Util::SecFree(skData);
+    }
+
+    SECTION("Should throw on a bad public key") {
+        uint8_t buf[PublicKey::PUBLIC_KEY_SIZE] = {0};
+        std::set<int> invalid = {1, 2, 3, 4};
+
+        for (int i = 0; i < 10; i++) {
+            buf[0] = (uint8_t)i;
+            try {
+                PublicKey::FromBytes(buf);
+                REQUIRE(invalid.count(i) == 0);
+            } catch (std::string& s) {
+                REQUIRE(invalid.count(i) != 0);
+            }
+        }
+    }
+
+    SECTION("Should throw on a bad signature") {
+        uint8_t buf[Signature::SIGNATURE_SIZE] = {0};
+        std::set<int> invalid = {0, 1, 2, 3, 5, 6, 7, 8};
+
+        for (int i = 0; i < 10; i++) {
+            buf[0] = (uint8_t)i;
+            try {
+                Signature::FromBytes(buf);
+                REQUIRE(invalid.count(i) == 0);
+            } catch (std::string& s) {
+                REQUIRE(invalid.count(i) != 0);
+            }
+        }
+    }
+}
+
 TEST_CASE("Util tests") {
     SECTION("Should convert an int to four bytes") {
         uint32_t x = 1024;
@@ -335,19 +380,6 @@ TEST_CASE("Signatures") {
         REQUIRE(Signature::FromInsecureSig(sig3) == sig2);
     }
 
-    SECTION("Should throw on a bad private key") {
-        uint8_t seed[32];
-        getRandomSeed(seed);
-        PrivateKey sk1 = PrivateKey::FromSeed(seed, 32);
-        uint8_t* skData = Util::SecAlloc<uint8_t>(
-                Signature::SIGNATURE_SIZE);
-        sk1.Serialize(skData);
-        skData[0] = 255;
-        REQUIRE_THROWS(PrivateKey::FromBytes(skData));
-
-        Util::SecFree(skData);
-    }
-
     SECTION("Should not validate a bad sig") {
         uint8_t message1[7] = {100, 2, 254, 88, 90, 45, 22};
         uint8_t seed[32];