Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from
May 29, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3cb057f
Fix possible integer overflow in DER parsing
real-or-random Nov 1, 2018
01ee1b3
Parse DER-enconded length into a size_t instead of an int
real-or-random Nov 7, 2018
ec8f20b
Avoid out-of-bound pointers and integer overflows in size comparisons
real-or-random Nov 7, 2018
14c7dbd
Simplify control flow in DER parsing
real-or-random Nov 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions contrib/lax_der_parsing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,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) {
return 0;
}
pos += lenbyte;
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return 0;
}
while (lenbyte > 0 && input[pos] == 0) {
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here too

return 0;
}
while (lenbyte > 0 && input[pos] == 0) {
Expand Down
70 changes: 38 additions & 32 deletions src/ecdsa_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,68 +46,73 @@ static const secp256k1_fe secp256k1_ecdsa_const_p_minus_order = SECP256K1_FE_CON
0, 0, 0, 1, 0x45512319UL, 0x50B75FC4UL, 0x402DA172UL, 0x2FC9BAEEUL
);

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like a good idea

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

size_t lenleft;
unsigned char b1;
VERIFY_CHECK(len != NULL);
*len = 0;
if (*sigp >= sigend) {
return -1;
return 0;
}
b1 = *((*sigp)++);
if (b1 == 0xFF) {
/* X.690-0207 8.1.3.5.c the value 0xFF shall not be used. */
return -1;
return 0;
}
if ((b1 & 0x80) == 0) {
/* X.690-0207 8.1.3.4 short form length octets */
return b1;
*len = b1;
return 1;
}
if (b1 == 0x80) {
/* Indefinite length is not allowed in DER. */
return -1;
return 0;
}
/* X.690-207 8.1.3.5 long form length octets */
lenleft = b1 & 0x7F;
if (lenleft > sigend - *sigp) {
return -1;
lenleft = b1 & 0x7F; /* lenleft is at least 1 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if (lenleft > (size_t)(sigend - *sigp)) {
return 0;
}
if (**sigp == 0) {
/* Not the shortest possible length encoding. */
return -1;
return 0;
}
if ((size_t)lenleft > sizeof(size_t)) {
if (lenleft > sizeof(size_t)) {
/* The resulting length would exceed the range of a size_t, so
* certainly longer than the passed array size.
*/
return -1;
return 0;
}
while (lenleft > 0) {
ret = (ret << 8) | **sigp;
if (ret + lenleft > (size_t)(sigend - *sigp)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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).

Copy link
Contributor Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No initial value is set for *len?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

(*sigp)++;
lenleft--;
}
if (ret < 128) {
if (*len > (size_t)(sigend - *sigp)) {
/* Result exceeds the length of the passed array. */
return 0;
}
if (*len < 128) {
/* Not the shortest possible length encoding. */
return -1;
return 0;
}
return ret;
return 1;
}

static int secp256k1_der_parse_integer(secp256k1_scalar *r, const unsigned char **sig, const unsigned char *sigend) {
int overflow = 0;
unsigned char ra[32] = {0};
int rlen;
size_t rlen;

if (*sig == sigend || **sig != 0x02) {
/* Not a primitive integer (X.690-0207 8.3.1). */
return 0;
}
(*sig)++;
rlen = secp256k1_der_read_len(sig, sigend);
if (rlen <= 0 || (*sig) + rlen > sigend) {
if (secp256k1_der_read_len(&rlen, sig, sigend) == 0) {
return 0;
}
if (rlen == 0 || *sig + rlen > sigend) {
/* Exceeds bounds or not at least length 1 (X.690-0207 8.3.1). */
return 0;
}
Expand All @@ -123,8 +128,11 @@ static int secp256k1_der_parse_integer(secp256k1_scalar *r, const unsigned char
/* Negative. */
overflow = 1;
}
while (rlen > 0 && **sig == 0) {
/* Skip leading zero bytes */
/* There is at most one leading zero byte:
* 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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

rlen--;
(*sig)++;
}
Expand All @@ -144,18 +152,16 @@ static int secp256k1_der_parse_integer(secp256k1_scalar *r, const unsigned char

static int secp256k1_ecdsa_sig_parse(secp256k1_scalar *rr, secp256k1_scalar *rs, const unsigned char *sig, size_t size) {
const unsigned char *sigend = sig + size;
int rlen;
size_t rlen;
if (sig == sigend || *(sig++) != 0x30) {
/* The encoding doesn't start with a constructed sequence (X.690-0207 8.9.1). */
return 0;
}
rlen = secp256k1_der_read_len(&sig, sigend);
if (rlen < 0 || sig + rlen > sigend) {
/* Tuple exceeds bounds */
if (secp256k1_der_read_len(&rlen, &sig, sigend) == 0) {
return 0;
}
if (sig + rlen != sigend) {
/* Garbage after tuple. */
if (rlen != (size_t)(sigend - sig)) {
/* Tuple exceeds bounds or garage after tuple. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return 0;
}

Expand Down
3 changes: 2 additions & 1 deletion src/hash_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

/* Fill the buffer, and process it. */
size_t chunk_len = 64 - bufsize;
memcpy(((unsigned char*)hash->buf) + bufsize, data, chunk_len);
Expand Down