Avoid implementation-defined and undefined behavior when dealing with sizes #578

real-or-random · 2018-12-14T13:30:10Z

This is a result of auditing the code for overflow issues at random places. None of this is critical but I think all of it should be fixed.

I know this touches "red" code. I double-checked and triple-checked this but I can understand if some of the changes are not desirable because they change well-tested code.

Best reviewed in individual commits.

If we’re in the last loop iteration, then `lenleft == 1` and it could be the case that `ret == MAX_SIZE`, and so `ret + lenleft` will overflow to 0 and the sanity check will not catch it. Then we will return `(int) MAX_SIZE`, which should be avoided because this value is implementation-defined. (However, this is harmless because `(int) MAX_SIZE == -1` on all supported platforms.)

gmaxwell · 2018-12-15T07:58:11Z

Aside, haven't reviewed yet... but your first commit messages appear to be exactly the sort of thing I want to see on a PR like that-- it gives a trace of the execution it's concerned with. I want to merge it just so I can point people in the future at what this sort of change should look like.

laanwj · 2019-02-21T13:33:09Z

src/ecdsa_impl.h

-static int secp256k1_der_read_len(const unsigned char **sigp, const unsigned char *sigend) {
- int lenleft, b1;
- size_t ret = 0;
+static int secp256k1_der_read_len(size_t *len, const unsigned char **sigp, const unsigned char *sigend) {


Does this need a not-NULL check/assertion for len?

sounds like a good idea

laanwj · 2019-02-21T13:35:26Z

src/ecdsa_impl.h

- /* Result exceeds the length of the passed array. */
- return -1;
- }
+ *len = (*len << 8) | **sigp;


No initial value is set for *len?

luke-jr · 2019-02-21T19:18:53Z

contrib/lax_der_parsing.c

@@ -51,7 +51,7 @@ int ecdsa_signature_parse_der_lax(const secp256k1_context* ctx, secp256k1_ecdsa_
 lenbyte = input[pos++];
 if (lenbyte & 0x80) {
 lenbyte -= 0x80;
- if (pos + lenbyte > inputlen) {
+ if (lenbyte > inputlen - pos) {


pos could possibly be larger than inputlen here, since it was just post-incremented on line 51 without a check.

Right before line 51, we have pos < inputlen since we have pos != input due to the check in line 48.
So after the increment in line 51, we have pos <= inputlen

luke-jr · 2019-02-21T19:25:59Z

contrib/lax_der_parsing.c

@@ -89,7 +89,7 @@ int ecdsa_signature_parse_der_lax(const secp256k1_context* ctx, secp256k1_ecdsa_
 lenbyte = input[pos++];
 if (lenbyte & 0x80) {
 lenbyte -= 0x80;
- if (pos + lenbyte > inputlen) {
+ if (lenbyte > inputlen - pos) {


Same here, pos is incremented without check at line 89.

Same here too

luke-jr · 2019-02-21T19:38:12Z

src/ecdsa_impl.h

@@ -66,7 +66,7 @@ static int secp256k1_der_read_len(const unsigned char **sigp, const unsigned cha
 return -1;
 }
 /* X.690-207 8.1.3.5 long form length octets */
- lenleft = b1 & 0x7F;
+ lenleft = b1 & 0x7F; /* lenleft is at least 1 */


ACK, b1 comes from an unsigned char, and the function returns before this point if b1 < 0x80 (ruling out a value of 0) or b1 == 0x80 (the only value that would yield a 0 from this mask)

luke-jr · 2019-02-21T19:42:42Z

src/ecdsa_impl.h

@@ -82,13 +82,13 @@ static int secp256k1_der_read_len(const unsigned char **sigp, const unsigned cha
 }
 while (lenleft > 0) {
 ret = (ret << 8) | **sigp;
- if (ret + lenleft > (size_t)(sigend - *sigp)) {


This looks like it was wrong. If the remaining digits are all zero, the final size is just ret, not ret + ....

If my understanding is correct, is there a possibility fixing this bug could create a a consensus incompatibility?

[edit: Deleted what I wrote here because it was wrong. But did you see the commit message? I think that explains the change. If not, I don't understand your sentence about the remaining digits]

It should be noted that none of this is code is consensus-critical (for Bitcoin) because Bitcoin relies on lax DER parsing function (as implemented in the contrib directory of secp256k1). The change may affect other projects though. AFAIK Zcash uses the functions in the PR here. But none of my changes are intended to change the behavior (except removing UB and IDB).

This looks like it was wrong. If the remaining digits are all zero, the final size is just ret, not ret + ....

Ah I think now I know what you are saying. It took me also a while to understand that code. The purpose of ret + lenleft instead of ret is a (not really useful) early return: this function needs to read lenleft more bytes (or actually lenleft - 1 bytes but we didn't increment sigp* yet), and the caller needs to read at least ret more bytes. So if lenleft + ret is more bytes than available, it is indeed correct to return -1 early.

luke-jr · 2019-02-21T19:44:12Z

src/ecdsa_impl.h

+ * if there were two leading zero bytes, we would have failed and returned 0
+ * because of excessive 0x00 padding already. */
+ if (rlen > 0 && **sig == 0) {
+ /* Skip leading zero byte */


luke-jr · 2019-02-21T19:46:38Z

src/ecdsa_impl.h

- if (sig + rlen != sigend) {
- /* Garbage after tuple. */
+ if (rlen != (size_t)(sigend - sig)) {
+ /* Tuple exceeds bounds or garage after tuple. */


luke-jr · 2019-02-21T19:49:04Z

src/hash_impl.h

@@ -131,7 +131,8 @@ static void secp256k1_sha256_transform(uint32_t* s, const uint32_t* chunk) {
 static void secp256k1_sha256_write(secp256k1_sha256 *hash, const unsigned char *data, size_t len) {
 size_t bufsize = hash->bytes & 0x3F;
 hash->bytes += len;
- while (bufsize + len >= 64) {
+ VERIFY_CHECK(hash->bytes >= len);
+ while (len >= 64 - bufsize) {


luke-jr · 2019-02-21T19:54:09Z

src/ecdsa_impl.h

-static int secp256k1_der_read_len(const unsigned char **sigp, const unsigned char *sigend) {
- int lenleft, b1;
- size_t ret = 0;
+static int secp256k1_der_read_len(size_t *len, const unsigned char **sigp, const unsigned char *sigend) {


n.b. This returns via output pointer instead of return value, to prevent accidentally casting to int (or other wrong types) implicitly.

real-or-random · 2019-02-22T21:06:36Z

Addressed the comments

gmaxwell · 2019-05-22T22:17:07Z

Can you squash?

This avoids a possibly implementation-defined signed (int) to unsigned (size_t) conversion portably.

This changes pointer calculations in size comparions to a form that ensures that no out-of-bound pointers are computed, because even their computation yields undefined behavior. Also, this changes size comparions to a form that ensures that neither the left-hand side nor the right-hand side can overflow.

real-or-random · 2019-05-23T13:23:11Z

squashed

…dealing with sizes 14c7dbd Simplify control flow in DER parsing (Tim Ruffing) ec8f20b Avoid out-of-bound pointers and integer overflows in size comparisons (Tim Ruffing) 01ee1b3 Parse DER-enconded length into a size_t instead of an int (Tim Ruffing) 3cb057f Fix possible integer overflow in DER parsing (Tim Ruffing) Pull request description: This is a result of auditing the code for overflow issues at random places. None of this is critical but I think all of it should be fixed. I know this touches "red" code. I double-checked and triple-checked this but I can understand if some of the changes are not desirable because they change well-tested code. Best reviewed in individual commits. ACKs for commit 14c7db: Tree-SHA512: 312dd3f961739752e1a861e75bd755920f634f87ee9668793e102c224434e8d21367452e114de729322c71a89f4fa82126aa5d32742f2bbbc091777c99515e10

real-or-random · 2019-05-29T12:16:31Z

src/ecdsa_impl.h

- if (sig + rlen != sigend) {
- /* Garbage after tuple. */
+ if (rlen != (size_t)(sigend - sig)) {
+ /* Tuple exceeds bounds or garage after tuple. */


I admit that "garage after tuple" seems to be an unrealistic concern. 🔧🚗

e10439c scripted-diff: rename privkey with seckey in secp256k1 interface (Pieter Wuille) ca8bc42 Drop --disable-jni from libsecp256k1 configure options (Pieter Wuille) ddc2419 Update MSVC build config for libsecp256k1 (Pieter Wuille) 67f232b Squashed 'src/secp256k1/' changes from b19c000..2ed54da (Pieter Wuille) Pull request description: It's been abound a year since the subtree was updated. Here is a list of the included PRs: * bitcoin-core/secp256k1#755: Recovery signing: add to constant time test, and eliminate non ct operators * bitcoin-core/secp256k1#754: Fix uninit values passed into cmov * bitcoin-core/secp256k1#752: autoconf: Use ":" instead of "dnl" as a noop * bitcoin-core/secp256k1#750: Add macOS to the CI * bitcoin-core/secp256k1#701: Make ec_ arithmetic more consistent and add documentation * bitcoin-core/secp256k1#732: Retry if r is zero during signing * bitcoin-core/secp256k1#742: Fix typo in ecmult_const_impl.h * bitcoin-core/secp256k1#740: Make recovery/main_impl.h non-executable * bitcoin-core/secp256k1#735: build: fix OpenSSL EC detection on macOS * bitcoin-core/secp256k1#728: Suppress a harmless variable-time optimization by clang in memczero * bitcoin-core/secp256k1#722: Context isn't freed in the ECDH benchmark * bitcoin-core/secp256k1#700: Allow overriding default flags * bitcoin-core/secp256k1#708: Constant-time behaviour test using valgrind memtest. * bitcoin-core/secp256k1#710: Eliminate harmless non-constant time operations on secret data. * bitcoin-core/secp256k1#718: Clarify that a secp256k1_ecdh_hash_function must return 0 or 1 * bitcoin-core/secp256k1#714: doc: document the length requirements of output parameter. * bitcoin-core/secp256k1#682: Remove Java Native Interface * bitcoin-core/secp256k1#713: Docstrings * bitcoin-core/secp256k1#704: README: add a section for test coverage * bitcoin-core/secp256k1#709: Remove secret-dependant non-constant time operation in ecmult_const. * bitcoin-core/secp256k1#703: Overhaul README.md * bitcoin-core/secp256k1#689: Remove "except in benchmarks" exception for fp math * bitcoin-core/secp256k1#679: Add SECURITY.md * bitcoin-core/secp256k1#685: Fix issue where travis does not show the ./tests seed… * bitcoin-core/secp256k1#690: Add valgrind check to travis * bitcoin-core/secp256k1#678: Preventing compiler optimizations in benchmarks without a memory fence * bitcoin-core/secp256k1#688: Fix ASM setting in travis * bitcoin-core/secp256k1#684: Make no-float policy explicit * bitcoin-core/secp256k1#677: Remove note about heap allocation in secp256k1_ecmult_odd_multiples_table_storage_var * bitcoin-core/secp256k1#647: Increase robustness against UB in secp256k1_scalar_cadd_bit * bitcoin-core/secp256k1#664: Remove mention of ec_privkey_export because it doesn't exist * bitcoin-core/secp256k1#337: variable sized precomputed table for signing * bitcoin-core/secp256k1#661: Make ./configure string consistent * bitcoin-core/secp256k1#657: Fix a nit in the recovery tests * bitcoin-core/secp256k1#650: secp256k1/src/tests.c: Properly handle sscanf return value * bitcoin-core/secp256k1#654: Fix typo (∞) * bitcoin-core/secp256k1#583: JNI: fix use sig array * bitcoin-core/secp256k1#644: Avoid optimizing out a verify_check * bitcoin-core/secp256k1#652: README.md: update instruction to run tests * bitcoin-core/secp256k1#651: Fix typo in secp256k1_preallocated.h * bitcoin-core/secp256k1#640: scalar_impl.h: fix includes * bitcoin-core/secp256k1#655: jni: Use only Guava for hex encoding and decoding * bitcoin-core/secp256k1#634: Add a descriptive comment for secp256k1_ecmult_const. * bitcoin-core/secp256k1#631: typo in comment for secp256k1_ec_pubkey_tweak_mul () * bitcoin-core/secp256k1#629: Avoid calling _is_zero when _set_b32 fails. * bitcoin-core/secp256k1#630: Note intention of timing sidechannel freeness. * bitcoin-core/secp256k1#628: Fix ability to compile tests without -DVERIFY. * bitcoin-core/secp256k1#627: Guard memcmp in tests against mixed size inputs. * bitcoin-core/secp256k1#578: Avoid implementation-defined and undefined behavior when dealing with sizes * bitcoin-core/secp256k1#595: Allow to use external default callbacks * bitcoin-core/secp256k1#600: scratch space: use single allocation * bitcoin-core/secp256k1#592: Use trivial algorithm in ecmult_multi if scratch space is small * bitcoin-core/secp256k1#566: Enable context creation in preallocated memory * bitcoin-core/secp256k1#596: Make WINDOW_G configurable * bitcoin-core/secp256k1#561: Respect LDFLAGS and #undef STATIC_PRECOMPUTATION if using basic config * bitcoin-core/secp256k1#533: Make sure we're not using an uninitialized variable in secp256k1_wnaf_const(...) * bitcoin-core/secp256k1#617: Pass scalar by reference in secp256k1_wnaf_const() * bitcoin-core/secp256k1#619: Clear a copied secret key after negation * bitcoin-core/secp256k1#612: Allow field_10x26_arm.s to compile for ARMv7 architecture ACKs for top commit: real-or-random: ACK e10439c I verified the diff (subtree matches my local tree, manual inspection of other commits) but I didn't tested the resulting code fanquake: ACK e10439c Sjors: ACK e10439c jonasnick: reACK e10439c Tree-SHA512: eb6284a485da78e9d2ed3f771df85560d47c770ebf480a0d4121ab356ad26be101a2b973efe412f26e6c142bc1dbd2efbb5cc08774233e41918c59fe3dff3387

laanwj reviewed Feb 21, 2019

View reviewed changes

luke-jr reviewed Feb 21, 2019

View reviewed changes

real-or-random added 3 commits May 23, 2019 15:22

Parse DER-enconded length into a size_t instead of an int

01ee1b3

This avoids a possibly implementation-defined signed (int) to unsigned (size_t) conversion portably.

Simplify control flow in DER parsing

14c7dbd

real-or-random force-pushed the der-len branch from 70fd11d to 14c7dbd Compare May 23, 2019 13:22

gmaxwell merged commit 14c7dbd into bitcoin-core:master May 29, 2019

real-or-random commented May 29, 2019

View reviewed changes

sipa mentioned this pull request Jun 9, 2020

Update libsecp256k1 subtree bitcoin/bitcoin#19228

Merged

real-or-random mentioned this pull request Jul 29, 2020

Remove lax DER parsing functions from contrib #781

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid implementation-defined and undefined behavior when dealing with sizes #578

Avoid implementation-defined and undefined behavior when dealing with sizes #578

real-or-random commented Dec 14, 2018

gmaxwell commented Dec 15, 2018

laanwj Feb 21, 2019

real-or-random Feb 21, 2019

laanwj Feb 21, 2019

real-or-random Feb 21, 2019

luke-jr Feb 21, 2019

real-or-random Feb 22, 2019

luke-jr Feb 21, 2019

real-or-random Feb 22, 2019

luke-jr Feb 21, 2019

luke-jr Feb 21, 2019

real-or-random Feb 21, 2019 •

edited

Loading

real-or-random Feb 22, 2019 •

edited

Loading

luke-jr Feb 21, 2019

luke-jr Feb 21, 2019

luke-jr Feb 21, 2019

luke-jr Feb 21, 2019

real-or-random commented Feb 22, 2019

gmaxwell commented May 22, 2019

real-or-random commented May 23, 2019

real-or-random May 29, 2019

Avoid implementation-defined and undefined behavior when dealing with sizes #578

Avoid implementation-defined and undefined behavior when dealing with sizes #578

Conversation

real-or-random commented Dec 14, 2018

gmaxwell commented Dec 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

real-or-random Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

real-or-random Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

real-or-random commented Feb 22, 2019

gmaxwell commented May 22, 2019

real-or-random commented May 23, 2019

Choose a reason for hiding this comment

real-or-random Feb 21, 2019 •

edited

Loading

real-or-random Feb 22, 2019 •

edited

Loading