Fix integer overflow in ecmult_multi_var when n is large by jonasnick · Pull Request #568 · bitcoin-core/secp256k1

jonasnick · 2018-10-24T18:04:22Z

Without this PR ecmult_multi could return wrong results. If the number of points n is large enough then some or all multiplications could be skipped or the function could end up in an infinite loop. This PR adds two checks to prevent n from wrapping around.

real-or-random · 2018-10-24T19:09:17Z

src/ecmult_impl.h

+    if (n > (size_t)-1 - (max_points-1)) {
+        return 0;
+    }
+    *n_batches = (n + max_points-1) / max_points;


Suggested change

*n_batches = (n + max_points-1) / max_points;

*n_batches = (n + (max_points-1)) / max_points;

you could also CHECK/VERIFY that *n_batches != 0 here. As I understand it, this can only happen if n == 0 and then this function won't be called currently.

real-or-random · 2018-10-24T19:09:24Z

src/ecmult_impl.h

+    if (n > (size_t)-1 - (*n_batches-1)) {
+        return 0;
+    }
+    *n_batch_points = (n + *n_batches-1) / *n_batches;


Suggested change

*n_batch_points = (n + *n_batches-1) / *n_batches;

*n_batch_points = (n + (*n_batches-1)) / *n_batches;

apoelstra · 2018-10-24T19:34:13Z

ACK

jonasnick · 2018-10-24T21:23:58Z

@real-or-random I added a commit

dcousens · 2018-10-24T22:21:45Z

src/ecmult_impl.h

+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;
+    } else if (max_points > ECMULT_MAX_POINTS_PER_BATCH) {


unnecessary else?

indeed, thanks

real-or-random · 2018-10-25T08:40:36Z

Oh I didn't want to bother you with nits only. I assumed that the missing parentheses are real bugs but no, we deal with unsigned integers. :)
ACK

jonasnick · 2018-10-26T08:27:45Z

Removed unnecessary else (h/t @dcousens) and squashed

gmaxwell · 2018-10-27T00:12:23Z

src/ecmult_impl.h

+        max_points = ECMULT_MAX_POINTS_PER_BATCH;
+    }
+    /* Check overflow */
+    if (n > (size_t)-1 - (max_points-1)) {


Unless we have a reason, I think we'd prefer to use stdint SIZE_MAX here. (and in all the other cases where the largest SIZE_T is required)

SIZE_MAX requires C99 as far as I remember.

@real-or-random It is an stdint.h type, like uint32_t. The codebase uses stdint.h ( https://github.com/bitcoin-core/secp256k1/blob/master/src/util.h#L15) types everywhere. In practice every compiler someone would want to use has this header (even non C99 compilers), but if someone did have a compiler without it, they'd have to provide a compatibility header.

Okay, I see. It's equivalent but yes, SIZE_MAX is obviously much nicer to read. I was the one who suggested size_t(-1) to @jonasnick in the Schnorr signature PR: #558 (review) Maybe that's why @apoelstra used it in #557 too...

jonasnick · 2018-10-30T16:41:32Z

Replaced (size_t)-1 with MAX_SIZE

gmaxwell · 2018-11-17T17:14:24Z

ACK

jimpo · 2018-11-27T19:58:52Z

This seems fine, though you could also avoid the overflow checks entirely with

n_batches = 1 + (n-1)/max_points;
n_batch_points = 1 + (n-1)/n_batches;

This should work in all cases since n is guaranteed to be > 0.

jonasnick · 2019-01-15T19:14:52Z

I rewrote this PR with @jimpo's suggestion. I think this is better because the previous code was complex enough that we missed a potential bug [0] and the new code doesn't have unnecessary return 0's. Because n = SIZE_MAX is now actually possible I had to rewrite the tests to directly test the batch_size_helper.

[0] if (n_batches == 0 || n > SIZE_MAX - (*n_batches-1)) { (first asterisk missing)

real-or-random · 2019-01-16T09:20:36Z

ACK

jimpo · 2019-01-16T20:47:56Z

utACK

laanwj · 2019-02-21T15:49:04Z

utACK c37e53d, changes look correct to me

gmaxwell · 2019-02-24T19:29:45Z

The addition of the trivial ecmult_multi algorithm in pr580 unfortunately created a trivial merge conflict for this PR, sorry about that. I am ACK on the PR, but need the rebase.

gmaxwell · 2019-02-24T19:58:06Z

If you'd prefer I can make a fixup commit, whatever you'd be happiest with.

2277af5 Fix integer overflow in ecmult_multi_var when n is large (Jonas Nick) Pull request description: Without this PR ecmult_multi could return wrong results. If the number of points `n` is large enough then some or all multiplications could be skipped or the function could end up in an infinite loop. This PR adds two checks to prevent `n` from wrapping around. Tree-SHA512: 342944369b24776fa3ec0694eee159259ff67e94d2d8176c1d3159875f387d943d5bfdff7cde59f058e13f07fd09bde1cbc609426e63c8a5b8040e382dd865d8

gmaxwell · 2019-02-25T21:09:58Z

ACK

real-or-random reviewed Oct 24, 2018

View reviewed changes

dcousens reviewed Oct 24, 2018

View reviewed changes

jonasnick force-pushed the fix-ecmult-multi-overflow branch 2 times, most recently from 90ff155 to 24908be Compare October 26, 2018 08:27

gmaxwell reviewed Oct 27, 2018

View reviewed changes

jonasnick force-pushed the fix-ecmult-multi-overflow branch from 24908be to c17ef67 Compare October 30, 2018 16:40

jonasnick force-pushed the fix-ecmult-multi-overflow branch from c17ef67 to c37e53d Compare January 15, 2019 19:08

jonasnick force-pushed the fix-ecmult-multi-overflow branch from c37e53d to 2277af5 Compare February 25, 2019 16:13

Fix integer overflow in ecmult_multi_var when n is large

2277af5

gmaxwell merged commit 2277af5 into bitcoin-core:master Feb 25, 2019

real-or-random mentioned this pull request Apr 23, 2020

Improve C89 compliance #746

Closed

	*n_batches = (n + max_points-1) / max_points;
	*n_batches = (n + (max_points-1)) / max_points;

	n_batch_points = (n + n_batches-1) / *n_batches;
	n_batch_points = (n + (n_batches-1)) / *n_batches;

Comments

Conversation

jonasnick commented Oct 24, 2018

Uh oh!

real-or-random Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

real-or-random Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Oct 24, 2018

Uh oh!

jonasnick commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcousens Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

jonasnick Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

real-or-random commented Oct 25, 2018

Uh oh!

jonasnick commented Oct 26, 2018

Uh oh!

gmaxwell Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

real-or-random Oct 27, 2018

Choose a reason for hiding this comment

Uh oh!

gmaxwell Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

real-or-random Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

jonasnick commented Oct 30, 2018

Uh oh!

gmaxwell commented Nov 17, 2018

Uh oh!

jimpo commented Nov 27, 2018

Uh oh!

jonasnick commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

real-or-random commented Jan 16, 2019

Uh oh!

jimpo commented Jan 16, 2019

Uh oh!

laanwj commented Feb 21, 2019

Uh oh!

gmaxwell commented Feb 24, 2019

Uh oh!

gmaxwell commented Feb 24, 2019

Uh oh!

gmaxwell commented Feb 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jonasnick commented Oct 24, 2018 •

edited

Loading

gmaxwell Oct 27, 2018 •

edited

Loading

jonasnick commented Jan 15, 2019 •

edited

Loading