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.)
45@@ -46,68 +46,71 @@ static const secp256k1_fe secp256k1_ecdsa_const_p_minus_order = SECP256K1_FE_CON
46 0, 0, 0, 1, 0x45512319UL, 0x50B75FC4UL, 0x402DA172UL, 0x2FC9BAEEUL
47 );
48
49-static int secp256k1_der_read_len(const unsigned char **sigp, const unsigned char *sigend) {
50- int lenleft, b1;
51- size_t ret = 0;
52+static int secp256k1_der_read_len(size_t *len, const unsigned char **sigp, const unsigned char *sigend) {
len?
98- ret = (ret << 8) | **sigp;
99- if (ret + lenleft > (size_t)(sigend - *sigp)) {
100- /* Result exceeds the length of the passed array. */
101- return -1;
102- }
103+ *len = (*len << 8) | **sigp;
*len?
50@@ -51,7 +51,7 @@ int ecdsa_signature_parse_der_lax(const secp256k1_context* ctx, secp256k1_ecdsa_
51 lenbyte = input[pos++];
52 if (lenbyte & 0x80) {
53 lenbyte -= 0x80;
54- if (pos + lenbyte > inputlen) {
55+ if (lenbyte > inputlen - pos) {
pos could possibly be larger than inputlen here, since it was just post-incremented on line 51 without a check.
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
88@@ -89,7 +89,7 @@ int ecdsa_signature_parse_der_lax(const secp256k1_context* ctx, secp256k1_ecdsa_
89 lenbyte = input[pos++];
90 if (lenbyte & 0x80) {
91 lenbyte -= 0x80;
92- if (pos + lenbyte > inputlen) {
93+ if (lenbyte > inputlen - pos) {
pos is incremented without check at line 89.
65@@ -66,7 +66,7 @@ static int secp256k1_der_read_len(const unsigned char **sigp, const unsigned cha
66 return -1;
67 }
68 /* X.690-207 8.1.3.5 long form length octets */
69- lenleft = b1 & 0x7F;
70+ lenleft = b1 & 0x7F; /* lenleft is at least 1 */
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)
81@@ -82,13 +82,13 @@ static int secp256k1_der_read_len(const unsigned char **sigp, const unsigned cha
82 }
83 while (lenleft > 0) {
84 ret = (ret << 8) | **sigp;
85- 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, notret + ....
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.
130- /* Skip leading zero bytes */
131+ /* There is at most one leading zero byte:
132+ * if there were two leading zero bytes, we would have failed and returned 0
133+ * because of excessive 0x00 padding already. */
134+ if (rlen > 0 && **sig == 0) {
135+ /* Skip leading zero byte */
160- return 0;
161- }
162- if (sig + rlen != sigend) {
163- /* Garbage after tuple. */
164+ if (rlen != (size_t)(sigend - sig)) {
165+ /* Tuple exceeds bounds or garage after tuple. */
130@@ -131,7 +131,8 @@ static void secp256k1_sha256_transform(uint32_t* s, const uint32_t* chunk) {
131 static void secp256k1_sha256_write(secp256k1_sha256 *hash, const unsigned char *data, size_t len) {
132 size_t bufsize = hash->bytes & 0x3F;
133 hash->bytes += len;
134- while (bufsize + len >= 64) {
135+ VERIFY_CHECK(hash->bytes >= len);
136+ while (len >= 64 - bufsize) {
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.
165 return 0;
166 }
167- if (sig + rlen != sigend) {
168- /* Garbage after tuple. */
169+ if (rlen != (size_t)(sigend - sig)) {
170+ /* Tuple exceeds bounds or garage after tuple. */