src|cmake: Fix some FromBytes decoding issues, add more test cases (#…

…253)

* test: Expand invalid G1/G2 element test cases

Test all prefix values, not only 0-10.

* util: Introduce `HasOnlyZeros` to check a `Bytes` object for zero bytes

* elements: Don't accept zero only buffer with valid prefix in `FromBytes`

* test: Drop cases which become invalid with relic commit 1620a03b388e50acd68ed9c88d7cd82ec5490ce4

* cmake: Pin relic to b7b2266a0e4ee6f628f61d3ab638f524a18b52f1

Which is currently the latest `main`

src/CMakeLists.txt

@@ -7,7 +7,7 @@ include(FetchContent)
 if (DEFINED ENV{RELIC_MAIN})
   set(RELIC_GIT_TAG "origin/main")
 else ()
-  set(RELIC_GIT_TAG "7ed8e702db74d5d5a83b0bfaf9ee8e33a70e36ed")
+  set(RELIC_GIT_TAG "b7b2266a0e4ee6f628f61d3ab638f524a18b52f1")
 endif ()
 
 message(STATUS "Relic will be built from: ${RELIC_GIT_TAG}")

src/elements.cpp

@@ -32,17 +32,11 @@ G1Element G1Element::FromBytes(const Bytes& bytes)
     buffer[0] = 0x00;
     buffer[1] &= 0x1f;  // erase 3 msbs from given input
 
+    bool fZerosOnly = Util::HasOnlyZeros(Bytes(buffer, G1Element::SIZE + 1));
     if ((bytes[0] & 0xc0) == 0xc0) {  // representing infinity
         // enforce that infinity must be 0xc0000..00
-        if (bytes[0] != 0xc0) {
-            throw std::invalid_argument(
-                "Given G1 infinity element must be canonical");
-        }
-        for (int i = 1; i < G1Element::SIZE; ++i) {
-            if (bytes[i] != 0x00) {
-                throw std::invalid_argument(
-                    "Given G1 infinity element must be canonical");
-            }
+        if (bytes[0] != 0xc0 || !fZerosOnly) {
+            throw std::invalid_argument("Given G1 infinity element must be canonical");
         }
         return ele;
     } else {
@@ -51,6 +45,10 @@ G1Element G1Element::FromBytes(const Bytes& bytes)
                 "Given G1 non-infinity element must start with 0b10");
         }
 
+        if (fZerosOnly) {
+            throw std::invalid_argument("G1 non-infinity element can't have only zeros");
+        }
+
         if (bytes[0] & 0x20) {  // sign bit
             buffer[0] = 0x03;
         } else {
@@ -218,24 +216,24 @@ G2Element G2Element::FromBytes(const Bytes& bytes)
         throw std::invalid_argument(
             "Given G2 element must always have 48th byte start with 0b000");
     }
+    bool fZerosOnly = Util::HasOnlyZeros(Bytes(buffer, G2Element::SIZE + 1));
     if (((bytes[0] & 0xc0) == 0xc0)) {  // infinity
         // enforce that infinity must be 0xc0000..00
-        if (bytes[0] != 0xc0) {
+        if (bytes[0] != 0xc0 || !fZerosOnly) {
             throw std::invalid_argument(
                 "Given G2 infinity element must be canonical");
         }
-        for (int i = 1; i < G2Element::SIZE; ++i) {
-            if (bytes[i] != 0x00) {
-                throw std::invalid_argument(
-                    "Given G2 infinity element must be canonical");
-            }
-        }
         return ele;
     } else {
         if (((bytes[0] & 0xc0) != 0x80)) {
             throw std::invalid_argument(
                 "G2 non-inf element must have 0th byte start with 0b10");
         }
+
+        if (fZerosOnly) {
+            throw std::invalid_argument("G2 non-infinity element can't have only zeros");
+        }
+
         if (bytes[0] & 0x20) {
             buffer[0] = 0x03;
         } else {

src/test.cpp

@@ -466,21 +466,35 @@ TEST_CASE("Error handling")
     SECTION("Should throw on a bad public key")
     {
         vector<uint8_t> buf(G1Element::SIZE, 0);
-
-        for (int i = 0; i < 10; i++) {
+        // This are all the "first byte values" which are tested to be valid G1 elements if the remaining bytes are zero.
+        std::vector<uint8_t> vecValid{0x85, 0x86, 0x87, 0x88, 0x89, 0x8C, 0x8F, 0x91, 0x93, 0x94, 0x96, 0x98, 0x99, 0x9A,
+                                      0xA5, 0xA6, 0xA7, 0xA8, 0xA9, 0xAC, 0xAF, 0xB1, 0xB3, 0xB4, 0xB6, 0xB8, 0xB9, 0xBA,
+                                      0xC0};
+        for (int i = 0; i < 0xFF; i++) {
             buf[0] = (uint8_t)i;
-            REQUIRE_THROWS(G1Element::FromByteVector(buf));
+            if (std::find(vecValid.begin(), vecValid.end(), i) != vecValid.end()) {
+                REQUIRE_NOTHROW(G1Element::FromByteVector(buf));
+            } else {
+                REQUIRE_THROWS(G1Element::FromByteVector(buf));
+            }
         }
     }
 
     SECTION("Should throw on a bad G2Element")
     {
         vector<uint8_t> buf(G2Element::SIZE, 0);
 
-        for (int i = 0; i < 10; i++) {
+        for (int i = 0; i < 0xFF; i++) {
             buf[0] = (uint8_t)i;
-            REQUIRE_THROWS(G2Element::FromByteVector(buf));
+            if (i == 0xc0) { // Infinity prefix shouldn't throw here as we have only zero values
+                REQUIRE_NOTHROW(G2Element::FromByteVector(buf));
+            } else {
+                REQUIRE_THROWS(G2Element::FromByteVector(buf));
+            }
         }
+        // Trigger "G2 element must always have 48th byte start with 0b000" error case
+        buf[48] = 0xFF;
+        REQUIRE_THROWS(G2Element::FromByteVector(buf));
     }
 
     SECTION("Error handling should be thread safe")

src/util.hpp

@@ -15,6 +15,7 @@
 #ifndef SRC_BLSUTIL_HPP_
 #define SRC_BLSUTIL_HPP_
 
+#include <algorithm>
 #include <iomanip>
 #include <sstream>
 #include <string>
@@ -142,6 +143,10 @@ class Util {
         return sum;
     }
 
+    static bool HasOnlyZeros(const Bytes& bytes) {
+        return std::all_of(bytes.begin(), bytes.end(), [](uint8_t byte){ return byte == 0x00; });
+    }
+
  private:
     friend class BLS;
     static SecureAllocCallback secureAllocCallback;