Prevent integer overflow in ReadVarInt. #9693

gmaxwell · 2017-02-06T02:55:04Z

We don't normally use ReadVarInt from untrusted inputs, but we might
see this in the case of corruption.

This is exposed in test_bitcoin_fuzzy.

We don't normally use ReadVarInt from untrusted inputs, but we might see this in the case of corruption. This is exposed in test_bitcoin_fuzzy.

gmaxwell · 2017-02-06T02:55:23Z

@sipa might have a suggestion for a cleaner way to do this?

dcousens · 2017-02-06T03:13:32Z

src/serialize.h

@@ -336,11 +336,18 @@ I ReadVarInt(Stream& is)
    I n = 0;
    while(true) {
        unsigned char chData = ser_readdata8(is);
+        if (n > (std::numeric_limits<I>::max() >> 7)) {
+           throw std::ios_base::failure("ReadVarInt(): size too large");
+        }


Could this not be done before chData is read?
It reads currently as though chData being read impacts this in some way

It won't hurt as this branch will never be taken in non-exceptional situation, and at best it may improve ILP.

I meant for readability

sipa · 2017-02-06T03:57:45Z

Alternative (as this is not used for data read from the network): restrict CVarInt to unsigned types only (the only reason to suggest this is because I'm not sure if there may be a performance impact of this PR as is).

laanwj · 2017-02-06T08:25:03Z

Concept ACK

pstratem · 2017-02-08T06:20:32Z

A micro benchmark of this change on my laptop did not show any significant change in runtime.

(This is a pretty bad benchmark though).

sipa · 2017-02-09T00:05:25Z

tACK 45f0961

I ran a benchmark: comparing the wall-clock time of the gettxoutsetinfo command's inner loop after commenting out the chainstate hash computation, on a node with 0 connections synced to block 250065. The chainstate was on a tmpfs. Done on i7-6820HQ CPU with frequency pegged to 2.60GHz, with 32 GiB of RAM, and dbcache=8000. Measurements using GetTimeMicros.

Master: 3747773 3747375 3734935 3743421 3723418 3734187 3717763 3732135 3693859 3729604 3731461 3752013 3714361 3703590 3748206 3743741 3733456 3743399 3743514 3750425 3730095 3719208 3735383 3714827 3728027 3712683 3718786

This PR: 3690632 3692104 3686250 3681913 3667378 3661354 3680667 3700359 3687146 3684476 3686028 3679919 3669181 3731960 3671747 3679751 3664658 3691547 3683182 3675638 3682229 3672767 3667536 3669105 3670341 3663954 3664253

If anything, it seems this patch makes UTXO deserialization (likely the most impactful usage of VARINT deserialization) slightly, but measurably, faster by 1-2% at least.

TheBlueMatt · 2017-02-09T15:58:40Z

utACK 45f0961

ryanofsky · 2017-02-09T16:52:00Z

It doesn't seem like the varint code supports negative numbers. Should there be a
static_assert(std::is_unsigned<I>::value, "no support for signed varints"); somewhere in here?

gmaxwell · 2017-02-10T08:21:20Z

@ryanofsky would have been good when it was written, unfortunately it's wrong right now-- ccoins uses it on nVersion which is signed, and the result is corrupted data (this is a known issue) fortunately we don't use the nversion for anything. (We should make sure that fact doesn't mean these exceptions can trigger.)

practicalswift · 2017-02-11T12:45:51Z

@gmaxwell Nice find! Do you have a test_bitcoin_fuzzy test case to share that triggers this issue?

I'm doing some bitcoin fuzzing myself and I'm curious to roughly how many CPU months that went into finding this issue and more specifically how many total paths that had been reached before finding this (the number printed at overall results/total paths assuming afl-fuzz)?

From my experience the code paths that can be reached using test_bitcoin_fuzzy seem to be very very robust :-)

BTW, are there any known plans to submit a test_bitcoin_fuzzy based fuzzer to https://github.com/google/oss-fuzz?

I'm really interested in fuzzing bitcoind and I'd love to share corpora. I've done some fuzzing of the Apple Swift compiler using my own fuzzer - see results and @practicalswift for some context.

jtimon · 2017-04-17T11:37:30Z

Concept ACK

45f0961 Prevent integer overflow in ReadVarInt. (Gregory Maxwell) Tree-SHA512: 385ea0efb6b59d44c45a49227e5f6fff236b4775544cbeb236312a3fd87fd75c226ac56f7aa1bca66b853639da75a579610074f7582f92cf2ebd4a74bc40f6f0

499d95e Add static_assert to prevent VARINT(<signed value>) (Russell Yanofsky) Pull request description: Using VARINT with signed types is dangerous because negative values will appear to serialize correctly, but then deserialize as positive values mod 128. This commit changes the VARINT macro to trigger a compile error by default if called with an signed value, and it updates existing broken uses of VARINT to pass a special flag that lets them keep working with no changes in behavior. There is some discussion about this issue here: #9693 (comment). I think another good change along these lines would be to make `GetSizeOfVarInt` and `WriteVarInt` throw exceptions if they are passed numbers less than 0 to serialize. But unlike this change, that would be a change in runtime behavior, and need more consideration. Tree-SHA512: 082c65598cfac6dc1da042bdb47dbc9d5d789fc849fe52921cc238578588f4e5ff976c8b4b2ce42cb75290eb14f3b42ea76e26202c223c5b2aa63ef45c2ea3cc

Summary: 45f0961 Prevent integer overflow in ReadVarInt. (Gregory Maxwell) Tree-SHA512: 385ea0efb6b59d44c45a49227e5f6fff236b4775544cbeb236312a3fd87fd75c226ac56f7aa1bca66b853639da75a579610074f7582f92cf2ebd4a74bc40f6f0 Backport of Core PR 9693 bitcoin/bitcoin#9693 Test Plan: make check Reviewers: deadalnix, jasonbcox, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D2950

45f0961 Prevent integer overflow in ReadVarInt. (Gregory Maxwell) Tree-SHA512: 385ea0efb6b59d44c45a49227e5f6fff236b4775544cbeb236312a3fd87fd75c226ac56f7aa1bca66b853639da75a579610074f7582f92cf2ebd4a74bc40f6f0

Summary: 45f0961 Prevent integer overflow in ReadVarInt. (Gregory Maxwell) Tree-SHA512: 385ea0efb6b59d44c45a49227e5f6fff236b4775544cbeb236312a3fd87fd75c226ac56f7aa1bca66b853639da75a579610074f7582f92cf2ebd4a74bc40f6f0 Backport of Core PR 9693 bitcoin/bitcoin#9693 Test Plan: make check Reviewers: deadalnix, jasonbcox, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D2950

45f0961 Prevent integer overflow in ReadVarInt. (Gregory Maxwell) Tree-SHA512: 385ea0efb6b59d44c45a49227e5f6fff236b4775544cbeb236312a3fd87fd75c226ac56f7aa1bca66b853639da75a579610074f7582f92cf2ebd4a74bc40f6f0

499d95e Add static_assert to prevent VARINT(<signed value>) (Russell Yanofsky) Pull request description: Using VARINT with signed types is dangerous because negative values will appear to serialize correctly, but then deserialize as positive values mod 128. This commit changes the VARINT macro to trigger a compile error by default if called with an signed value, and it updates existing broken uses of VARINT to pass a special flag that lets them keep working with no changes in behavior. There is some discussion about this issue here: bitcoin#9693 (comment). I think another good change along these lines would be to make `GetSizeOfVarInt` and `WriteVarInt` throw exceptions if they are passed numbers less than 0 to serialize. But unlike this change, that would be a change in runtime behavior, and need more consideration. Tree-SHA512: 082c65598cfac6dc1da042bdb47dbc9d5d789fc849fe52921cc238578588f4e5ff976c8b4b2ce42cb75290eb14f3b42ea76e26202c223c5b2aa63ef45c2ea3cc

172fe15 Add missing locks and locking annotations for CAddrMan (practicalswift) 9271ace Support serialization as another type without casting (Pieter Wuille) dc37dd9 Remove unnecessary NONNEGATIVE_SIGNED (Russell Yanofsky) ddd2ab1 Add static_assert to prevent VARINT(<signed value>) (Russell Yanofsky) 539db35 Support deserializing into temporaries (Pieter Wuille) 36db7fd Merge READWRITEMANY into READWRITE (Pieter Wuille) 08ebd5b Remove old pre C++11 functions begin_ptr/end_ptr. (furszy) 9c84665 Prevent integer overflow in ReadVarInt. (Gregory Maxwell) Pull request description: Back ported some pretty concise PRs from upstream over the data serialization area. * bitcoin#9305. --> removal of pre c++11 compatibility functions. * bitcoin#9693. --> prevent integer overflow in `ReadVarInt`. * bitcoin#9753. --> compile error if VARINT is called with a signed value. * bitcoin#12683. --> fixing constness violations * bitcoin#12731. --> support for `READWRITEAS` macro. ACKs for top commit: random-zebra: ACK 172fe15 Fuzzbawls: ACK 172fe15 Tree-SHA512: 1e1e697761b885dcc1aed8a2132bed693b1c76f1f2ed22ae5c074dfb4c353b81d307f71a4c12ed71fc39fd2207c1403881bd699e32b85a167bee57b4f0946130

Prevent integer overflow in ReadVarInt.

45f0961

We don't normally use ReadVarInt from untrusted inputs, but we might see this in the case of corruption. This is exposed in test_bitcoin_fuzzy.

dcousens reviewed Feb 6, 2017

View reviewed changes

ryanofsky mentioned this pull request Feb 13, 2017

Add static_assert to prevent VARINT(<signed value>) #9753

Merged

jtimon approved these changes Feb 27, 2017

View reviewed changes

sipa merged commit 45f0961 into bitcoin:master Apr 17, 2017

ptschip mentioned this pull request Oct 3, 2017

dbcache phase 3 enhancements BitcoinUnlimited/BitcoinUnlimited#785

Merged

furszy mentioned this pull request Mar 21, 2021

[Backport] Serialization updates PIVX-Project/PIVX#2256

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Prevent integer overflow in ReadVarInt. #9693

Prevent integer overflow in ReadVarInt. #9693

Uh oh!

gmaxwell commented Feb 6, 2017

Uh oh!

gmaxwell commented Feb 6, 2017

Uh oh!

dcousens Feb 6, 2017

Uh oh!

sipa Feb 8, 2017

Uh oh!

dcousens Feb 8, 2017

Uh oh!

sipa commented Feb 6, 2017 •

edited

Loading

Uh oh!

laanwj commented Feb 6, 2017

Uh oh!

pstratem commented Feb 8, 2017

Uh oh!

sipa commented Feb 9, 2017 •

edited

Loading

Uh oh!

TheBlueMatt commented Feb 9, 2017

Uh oh!

ryanofsky commented Feb 9, 2017

Uh oh!

gmaxwell commented Feb 10, 2017 •

edited

Loading

Uh oh!

practicalswift commented Feb 11, 2017 •

edited

Loading

Uh oh!

jtimon commented Apr 17, 2017

Uh oh!

Uh oh!

Prevent integer overflow in ReadVarInt. #9693

Prevent integer overflow in ReadVarInt. #9693

Uh oh!

Conversation

gmaxwell commented Feb 6, 2017

Uh oh!

gmaxwell commented Feb 6, 2017

Uh oh!

dcousens Feb 6, 2017

Choose a reason for hiding this comment

Uh oh!

sipa Feb 8, 2017

Choose a reason for hiding this comment

Uh oh!

dcousens Feb 8, 2017

Choose a reason for hiding this comment

Uh oh!

sipa commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Feb 6, 2017

Uh oh!

pstratem commented Feb 8, 2017

Uh oh!

sipa commented Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Feb 9, 2017

Uh oh!

ryanofsky commented Feb 9, 2017

Uh oh!

gmaxwell commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Feb 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Apr 17, 2017

Uh oh!

Uh oh!

sipa commented Feb 6, 2017 •

edited

Loading

sipa commented Feb 9, 2017 •

edited

Loading

gmaxwell commented Feb 10, 2017 •

edited

Loading

practicalswift commented Feb 11, 2017 •

edited

Loading