Fix integer overflow in ecmult_multi_var when n is large

Author: jonasnick

src/ecmult_impl.h

@@ -972,12 +972,33 @@ static size_t secp256k1_pippenger_max_points(secp256k1_scratch *scratch) {
     return res;
 }
 
+/* Compute the number and size of batches given the maximum number of points in a batch and the
+ * total number of points */
+static int secp256k1_ecmult_multi_batch_size_helper(size_t *n_batches, size_t *n_batch_points, size_t max_points, size_t n) {
+    if (max_points == 0) {
+        return 0;
+    }
+    if (max_points > ECMULT_MAX_POINTS_PER_BATCH) {
+        max_points = ECMULT_MAX_POINTS_PER_BATCH;
+    }
+    /* Check overflow */
+    if (n > (size_t)-1 - (max_points-1)) {
+        return 0;
+    }
+    *n_batches = (n + (max_points-1)) / max_points;
+    /* Check overflow */
+    if (n_batches == 0 || n > (size_t)-1 - (*n_batches-1)) {
+        return 0;
+    }
+    *n_batch_points = (n + (*n_batches-1)) / *n_batches;
+    return 1;
+}
+
 typedef int (*secp256k1_ecmult_multi_func)(const secp256k1_ecmult_context*, secp256k1_scratch*, secp256k1_gej*, const secp256k1_scalar*, secp256k1_ecmult_multi_callback cb, void*, size_t);
 static int secp256k1_ecmult_multi_var(const secp256k1_ecmult_context *ctx, secp256k1_scratch *scratch, secp256k1_gej *r, const secp256k1_scalar *inp_g_sc, secp256k1_ecmult_multi_callback cb, void *cbdata, size_t n) {
     size_t i;
 
     int (*f)(const secp256k1_ecmult_context*, secp256k1_scratch*, secp256k1_gej*, const secp256k1_scalar*, secp256k1_ecmult_multi_callback cb, void*, size_t, size_t);
-    size_t max_points;
     size_t n_batches;
     size_t n_batch_points;
 
@@ -991,24 +1012,17 @@ static int secp256k1_ecmult_multi_var(const secp256k1_ecmult_context *ctx, secp2
         return 1;
     }
 
-    max_points = secp256k1_pippenger_max_points(scratch);
-    if (max_points == 0) {
+    /* Compute the batch sizes for pippenger given a scratch space. If it's greater than a threshold
+     * use pippenger. Otherwise use strauss */
+    if (!secp256k1_ecmult_multi_batch_size_helper(&n_batches, &n_batch_points, secp256k1_pippenger_max_points(scratch), n)) {
         return 0;
-    } else if (max_points > ECMULT_MAX_POINTS_PER_BATCH) {
-        max_points = ECMULT_MAX_POINTS_PER_BATCH;
     }
-    n_batches = (n+max_points-1)/max_points;
-    n_batch_points = (n+n_batches-1)/n_batches;
-
     if (n_batch_points >= ECMULT_PIPPENGER_THRESHOLD) {
         f = secp256k1_ecmult_pippenger_batch;
     } else {
-        max_points = secp256k1_strauss_max_points(scratch);
-        if (max_points == 0) {
+        if (!secp256k1_ecmult_multi_batch_size_helper(&n_batches, &n_batch_points, secp256k1_strauss_max_points(scratch), n)) {
             return 0;
         }
-        n_batches = (n+max_points-1)/max_points;
-        n_batch_points = (n+n_batches-1)/n_batches;
         f = secp256k1_ecmult_strauss_batch;
     }
     for(i = 0; i < n_batches; i++) {

src/tests.c

@@ -2903,6 +2903,27 @@ void test_ecmult_multi_batching(void) {
     free(pt);
 }
 
+void test_ecmult_multi_overflow_n(void) {
+    secp256k1_scratch *scratch;
+    secp256k1_gej r;
+    ecmult_multi_data data;
+
+    /* Set number of points n to (size_t)-1 and max_points to 2. n_batches is computed as (n+2-1)/2 =
+     * 0, so if the overflow wasn't caught ecmult_multi wouldn't do any multiplication. */
+    scratch = secp256k1_scratch_create(&ctx->error_callback, secp256k1_pippenger_scratch_size(2, secp256k1_pippenger_bucket_window(2)) + PIPPENGER_SCRATCH_OBJECTS*ALIGNMENT);
+    CHECK(secp256k1_pippenger_max_points(scratch) == 2);
+    CHECK(secp256k1_ecmult_multi_var(&ctx->ecmult_ctx, scratch, &r, NULL, ecmult_multi_callback, &data, (size_t)-1) == 0);
+    secp256k1_scratch_destroy(scratch);
+
+    /* Set number of points n to (size_t)-1/2+2 and max_points to 1. n_batches is computed as n and
+     * n_batch_points = (n + n-1)/n = 1/n = 0 which would result in an infinite loop if the overflow
+     * wasn't caught */
+    scratch = secp256k1_scratch_create(&ctx->error_callback, secp256k1_strauss_scratch_size(1) + STRAUSS_SCRATCH_OBJECTS*ALIGNMENT);
+    CHECK(secp256k1_strauss_max_points(scratch) == 1);
+    CHECK(secp256k1_ecmult_multi_var(&ctx->ecmult_ctx, scratch, &r, NULL, ecmult_multi_callback, &data, ((size_t)-1)/2+2) == 0);
+    secp256k1_scratch_destroy(scratch);
+}
+
 void run_ecmult_multi_tests(void) {
     secp256k1_scratch *scratch;
 
@@ -2920,6 +2941,7 @@ void run_ecmult_multi_tests(void) {
     secp256k1_scratch_destroy(scratch);
 
     test_ecmult_multi_batching();
+    test_ecmult_multi_overflow_n();
 }
 
 void test_wnaf(const secp256k1_scalar *number, int w) {