Fix BigInt::operator< when both integers are negative

Broken in 00b6842a54

GH #2638

5 files changed, 200 insertions, 4 deletions

diff --git a/src/fuzzer/bn_cmp.cpp b/src/fuzzer/bn_cmp.cpp
new file mode 100644
index 000000000..48c25f324
--- /dev/null
+++ b/src/fuzzer/bn_cmp.cpp
@@ -0,0 +1,74 @@
+/*
+* (C) 2021 Jack Lloyd
+*
+* Botan is released under the Simplified BSD License (see license.txt)
+*/
+
+#include "fuzzers.h"
+
+#include <botan/bigint.h>
+
+void fuzz(const uint8_t in[], size_t len)
+   {
+   const size_t max_bits = 512;
+
+   if(len < 3 || len > 1 + 2*(max_bits/8))
+      return;
+
+   const uint8_t signs = in[0];
+   const size_t x_len = (len - 1) / 2;
+
+   Botan::BigInt x = Botan::BigInt::decode(in + 1, x_len);
+   Botan::BigInt y = Botan::BigInt::decode(in + 1 + x_len, len - x_len - 1);
+
+   if(signs & 1)
+      x.flip_sign();
+   if(signs & 2)
+      y.flip_sign();
+
+   const Botan::BigInt d1 = x - y;
+   const Botan::BigInt d2 = y - x;
+
+   FUZZER_ASSERT_TRUE(d1.cmp(d2, false) == 0);
+
+   const bool is_eq = (x == y);
+   const bool is_lt = (x < y);
+   const bool is_gt = (x > y);
+   const bool is_lte = (x <= y);
+   const bool is_gte = (x >= y);
+
+   if(is_eq)
+      {
+      FUZZER_ASSERT_TRUE(d1.is_zero());
+      FUZZER_ASSERT_TRUE(d2.is_zero());
+      }
+
+   if(is_lte)
+      {
+      FUZZER_ASSERT_TRUE(is_lt || is_eq);
+      }
+
+   if(is_gte)
+      {
+      FUZZER_ASSERT_TRUE(is_gt || is_eq);
+      }
+
+   if(is_lt)
+      {
+      FUZZER_ASSERT_TRUE(!is_gt);
+      FUZZER_ASSERT_TRUE(d1.is_nonzero());
+      FUZZER_ASSERT_TRUE(d2.is_nonzero());
+      FUZZER_ASSERT_TRUE(d1.is_negative());
+      FUZZER_ASSERT_TRUE(d2.is_positive());
+      }
+
+   if(is_gt)
+      {
+      FUZZER_ASSERT_TRUE(!is_lt);
+      FUZZER_ASSERT_TRUE(d1.is_nonzero());
+      FUZZER_ASSERT_TRUE(d2.is_nonzero());
+      FUZZER_ASSERT_TRUE(d1.is_positive());
+      FUZZER_ASSERT_TRUE(d2.is_negative());
+      }
+   }
+
diff --git a/src/lib/math/bigint/bigint.cpp b/src/lib/math/bigint/bigint.cpp
index 5fc201cef..6a3923d55 100644
--- a/src/lib/math/bigint/bigint.cpp
+++ b/src/lib/math/bigint/bigint.cpp
@@ -165,8 +165,8 @@ bool BigInt::is_less_than(const BigInt& other) const
 
    if(other.is_negative() && this->is_negative())
       {
-      return !bigint_ct_is_lt(other.data(), other.sig_words(),
-                              this->data(), this->sig_words(), true).is_set();
+      return bigint_ct_is_lt(other.data(), other.sig_words(),
+                             this->data(), this->sig_words()).is_set();
       }
 
    return bigint_ct_is_lt(this->data(), this->sig_words(),
diff --git a/src/tests/data/bn/cmp.vec b/src/tests/data/bn/cmp.vec
new file mode 100644
index 000000000..935634fb0
--- /dev/null
+++ b/src/tests/data/bn/cmp.vec
@@ -0,0 +1,78 @@
+[EQ]
+X = 5
+Y = 5
+R = true
+
+X = -5
+Y = 5
+R = false
+
+X = 5
+Y = -5
+R = false
+
+X = -5
+Y = -5
+R = true
+
+X = 4
+Y = 5
+R = false
+
+X = 5
+Y = 4
+R = false
+
+[LT]
+X = 5
+Y = 5
+R = false
+
+X = -5
+Y = 5
+R = true
+
+X = 5
+Y = -5
+R = false
+
+X = -5
+Y = -5
+R = false
+
+X = -1
+Y = -3
+R = false
+
+X = -2
+Y = -1
+R = true
+
+[LTE]
+X = 5
+Y = 5
+R = true
+
+X = 5
+Y = 6
+R = true
+
+X = 6
+Y = 5
+R = false
+
+X = -5
+Y = 5
+R = true
+
+X = 5
+Y = -5
+R = false
+
+X = -1
+Y = -2
+R = false
+
+X = -2
+Y = -1
+R = true
diff --git a/src/tests/test_bigint.cpp b/src/tests/test_bigint.cpp
index 1552fad08..831a66d2c 100644
--- a/src/tests/test_bigint.cpp
+++ b/src/tests/test_bigint.cpp
@@ -218,6 +218,50 @@ class BigInt_Unit_Tests final : public Test
 
 BOTAN_REGISTER_TEST("math", "bigint_unit", BigInt_Unit_Tests);
 
+class BigInt_Cmp_Test final : public Text_Based_Test
+   {
+   public:
+      BigInt_Cmp_Test() : Text_Based_Test("bn/cmp.vec", "X,Y,R") {}
+
+      Test::Result run_one_test(const std::string& op, const VarMap& vars) override
+         {
+         Test::Result result("BigInt Comparison " + op);
+
+         const BigInt x = vars.get_req_bn("X");
+         const BigInt y = vars.get_req_bn("Y");
+         const bool expected = vars.get_req_bool("R");
+
+         if(op == "EQ")
+            {
+            result.confirm("Values equal", x == y, expected);
+            }
+         else if(op == "LT")
+            {
+            result.confirm("Values LT", x < y, expected);
+
+            if(expected)
+               result.confirm("If LT then reverse is GT", y >= x);
+            else
+               result.confirm("If not LT then GTE", x >= y);
+            }
+         else if(op == "LTE")
+            {
+            result.confirm("Values LTE", x <= y, expected);
+
+            if(expected)
+               result.confirm("If LTE then either LT or EQ", x < y || x == y);
+            else
+               result.confirm("If not LTE then GT", x > y);
+            }
+         else
+            throw Test_Error("Unknown BigInt comparison type " + op);
+
+         return result;
+         }
+   };
+
+BOTAN_REGISTER_TEST("math", "bn_cmp", BigInt_Cmp_Test);
+
 class BigInt_Add_Test final : public Text_Based_Test
    {
    public:
diff --git a/src/tests/tests.h b/src/tests/tests.h
index 159c61400..bcd91bd5c 100644
--- a/src/tests/tests.h
+++ b/src/tests/tests.h
@@ -234,9 +234,9 @@ class Test
                test_failure(what, buf.data(), buf.size());
                }
 
-            bool confirm(const std::string& what, bool expr)
+            bool confirm(const std::string& what, bool expr, bool expected = true)
                {
-               return test_eq(what, expr, true);
+               return test_eq(what, expr, expected);
                }
 
             template<typename T>