Unobfuscate chainstate data in CCoinsViewDB::GetStats #6777

Merged
merged 4 commits into from Oct 13, 2015

Conversation

Projects
None yet
4 participants
@jamesob
Member

jamesob commented Oct 8, 2015

Per the thread on the mailing list, we missed (at least) one use of CLevelDBWrapper when adding chainstate obfuscation.

Preferably, this PR (or a followup) will also add automated tests that prevent future bugs of this kind. Subsequently we should also introduce an abstraction that prevents this sort of leak when performing iteration with CLevelDBWrapper.

cc @domob1812 @dexX7

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Oct 8, 2015

Contributor

Tested, "gettxoutsetinfo" now works as expected.

Based on a quick scan for value() it looks like the only other place where raw iterator data is used, is related to the block index DB: src/txdb.cpp#L205 (which isn't obfuscated)

Contributor

dexX7 commented Oct 8, 2015

Tested, "gettxoutsetinfo" now works as expected.

Based on a quick scan for value() it looks like the only other place where raw iterator data is used, is related to the block index DB: src/txdb.cpp#L205 (which isn't obfuscated)

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 8, 2015

Member

@dexX7 thanks for testing. I'm going to go ahead and apply the same treatment to the block index DB as well so that we won't have an issue if we decide to obfuscate it in the future.

Member

jamesob commented Oct 8, 2015

@dexX7 thanks for testing. I'm going to go ahead and apply the same treatment to the block index DB as well so that we won't have an issue if we decide to obfuscate it in the future.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 8, 2015

Member
Member

sipa commented Oct 8, 2015

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 8, 2015

Member

Okay @sipa, I'm thinking I'll just subclass leveldb's Iterator for the purposes of CLevelDBWrapper, then return that type from NewIterator.

Member

jamesob commented Oct 8, 2015

Okay @sipa, I'm thinking I'll just subclass leveldb's Iterator for the purposes of CLevelDBWrapper, then return that type from NewIterator.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 8, 2015

Member
Member

sipa commented Oct 8, 2015

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 8, 2015

Member

@sipa ha! and like magic... wish writing code was always this easy ;). Will cherry-pick and layer on modifications if possible.

Member

jamesob commented Oct 8, 2015

@sipa ha! and like magic... wish writing code was always this easy ;). Will cherry-pick and layer on modifications if possible.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 8, 2015

Member

Good catch @domob1812 , thanks for fix @jamesob @sipa

I'm getting an issue starting bitcoind on my node w/ obfuscation, though:
assertion "hashPrevBlock == view.GetBestBlock()" failed: file "main.cpp", line 1694, function "ConnectBlock" Abort trap (core dumped)

Not sure whether it's related to this, will investigate further. From debug.log:

2015-10-08 14:05:08 Using obfuscation key for /home/orion/.bitcoin/chainstate: 0000000000000000
...
2015-10-08 14:05:19 LoadBlockIndexDB: transaction index disabled
2015-10-08 14:05:20 Initializing databases...

Looks like my obfuscation key was lost, and it's now trying to interpret the data in raw.
debug.log unfortunately doesn't go back far enough to check what the key was before.

Member

laanwj commented Oct 8, 2015

Good catch @domob1812 , thanks for fix @jamesob @sipa

I'm getting an issue starting bitcoind on my node w/ obfuscation, though:
assertion "hashPrevBlock == view.GetBestBlock()" failed: file "main.cpp", line 1694, function "ConnectBlock" Abort trap (core dumped)

Not sure whether it's related to this, will investigate further. From debug.log:

2015-10-08 14:05:08 Using obfuscation key for /home/orion/.bitcoin/chainstate: 0000000000000000
...
2015-10-08 14:05:19 LoadBlockIndexDB: transaction index disabled
2015-10-08 14:05:20 Initializing databases...

Looks like my obfuscation key was lost, and it's now trying to interpret the data in raw.
debug.log unfortunately doesn't go back far enough to check what the key was before.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 8, 2015

Member

Found the issue. Indeed has nothing to do with this pull. On my node I was using a previous version of your patch, using a different OBFUSCATE_KEY_KEY. So no, the key was not lost, just misplaced. This shouldn't affect anyone starting with master.

 // Prefixed with null character to avoid collisions with other keys
-const std::string CLevelDBWrapper::OBFUSCATE_KEY_KEY = "\000obfuscate_key";
+//
+// We must use a string constructor which specifies length so that we copy
+// past the null-terminator.
+const std::string CLevelDBWrapper::OBFUSCATE_KEY_KEY("\000obfuscate_key", 14);
Member

laanwj commented Oct 8, 2015

Found the issue. Indeed has nothing to do with this pull. On my node I was using a previous version of your patch, using a different OBFUSCATE_KEY_KEY. So no, the key was not lost, just misplaced. This shouldn't affect anyone starting with master.

 // Prefixed with null character to avoid collisions with other keys
-const std::string CLevelDBWrapper::OBFUSCATE_KEY_KEY = "\000obfuscate_key";
+//
+// We must use a string constructor which specifies length so that we copy
+// past the null-terminator.
+const std::string CLevelDBWrapper::OBFUSCATE_KEY_KEY("\000obfuscate_key", 14);
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 8, 2015

Member

Tested ACK. gettxoutsetinfo works again after this.

Member

laanwj commented Oct 8, 2015

Tested ACK. gettxoutsetinfo works again after this.

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 8, 2015

Member

@laanwj good to hear. Thanks for testing.

Member

jamesob commented Oct 8, 2015

@laanwj good to hear. Thanks for testing.

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 8, 2015

Member

I've added some test coverage on the CLevelDB utilities. Provided travis is happy, I'm done adding changes.

Member

jamesob commented Oct 8, 2015

I've added some test coverage on the CLevelDB utilities. Provided travis is happy, I'm done adding changes.

sipa and others added some commits Oct 8, 2015

@dexX7

View changes

qa/rpc-tests/blockchain.py
+ u'total_amount': decimal.Decimal('8725.00000000'),
+ u'transactions': 200,
+ u'height': 200,
+ u'bestblock': u'6189e9cc58bedca8cb8e917cefe831839d296c2d12ae2e460066aa38d4a06f3e',

This comment has been minimized.

@dexX7

dexX7 Oct 9, 2015

Contributor

Is this always deterministic?

@dexX7

dexX7 Oct 9, 2015

Contributor

Is this always deterministic?

This comment has been minimized.

@dexX7

dexX7 Oct 9, 2015

Contributor

Just to clarify: unfortunally it's not (due to the potentially different time to create the blocks + random address generation).

Even if it were, I'm not sure, if such a strong coupling is desired. You might setup the chain or a few blocks manually and use util.set_node_times + pre-generated key-pairs, but this seems pretty laborious.

A middle ground could be to check, whether the JSON keys are available (as in your previous version), and then do some sanity checks, such as res['height'] >= 0, len(res['bestblock']) == 64, [...].

@dexX7

dexX7 Oct 9, 2015

Contributor

Just to clarify: unfortunally it's not (due to the potentially different time to create the blocks + random address generation).

Even if it were, I'm not sure, if such a strong coupling is desired. You might setup the chain or a few blocks manually and use util.set_node_times + pre-generated key-pairs, but this seems pretty laborious.

A middle ground could be to check, whether the JSON keys are available (as in your previous version), and then do some sanity checks, such as res['height'] >= 0, len(res['bestblock']) == 64, [...].

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 9, 2015

Member

@sipa any comments here?

Member

jamesob commented Oct 9, 2015

@sipa any comments here?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 9, 2015

No need to copy the obfuscation key, you can just store a pointer, as it's immutable data from the database object itself, so it won't disappear before the iterator is gone.

No need to copy the obfuscation key, you can just store a pointer, as it's immutable data from the database object itself, so it won't disappear before the iterator is gone.

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 10, 2015

Owner

Done, thanks.

Owner

jamesob replied Oct 10, 2015

Done, thanks.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 9, 2015

Member

Code ACK, I haven't reviewed the tests.

Member

sipa commented Oct 9, 2015

Code ACK, I haven't reviewed the tests.

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 10, 2015

Member

Ping on this. I think it's ready for merge.

Member

jamesob commented Oct 10, 2015

Ping on this. I think it's ready for merge.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 13, 2015

Member

@jamesob Yes going to merge this now, I've been sidetracked by the upnp vulnerabilty the last few days.

Member

laanwj commented Oct 13, 2015

@jamesob Yes going to merge this now, I've been sidetracked by the upnp vulnerabilty the last few days.

@laanwj laanwj merged commit dcd8e27 into bitcoin:master Oct 13, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 13, 2015

Merge pull request #6777
dcd8e27 Refer to obfuscate_key via pointer in peripheral CLevelDB classes (James O'Beirne)
1488506 Add tests for gettxoutsetinfo, CLevelDBBatch, CLevelDBIterator (James O'Beirne)
0fdf8c8 Handle obfuscation in CLevelDBIterator (James O'Beirne)
3499ce1 Encapsulate CLevelDB iterators cleanly (Pieter Wuille)

laanwj added a commit to laanwj/bitcoin that referenced this pull request Oct 31, 2015

Encapsulate CLevelDB iterators cleanly
Conflicts:
	src/leveldb.cpp
	src/leveldb.h
	src/txdb.cpp

Github-Pull: #6777
Rebased-From: 3499ce1

laanwj added a commit to laanwj/bitcoin that referenced this pull request Oct 31, 2015

Handle obfuscation in CLevelDBIterator
Github-Pull: #6777
Rebased-From: 0fdf8c8

laanwj added a commit to laanwj/bitcoin that referenced this pull request Oct 31, 2015

Add tests for gettxoutsetinfo, CLevelDBBatch, CLevelDBIterator
Thanks @dexX7.

Conflicts:
	qa/pull-tester/rpc-tests.py

Github-Pull: #6777
Rebased-From: 1488506

laanwj added a commit to laanwj/bitcoin that referenced this pull request Oct 31, 2015

@str4d str4d referenced this pull request in zcash/zcash Aug 28, 2017

Merged

Bitcoin 0.12+ dbwrapper improvements #2598

@dagurval dagurval referenced this pull request in bitcoinxt/bitcoinxt Oct 27, 2017

Merged

Encapsulate CLevelDB iterators #273

@dagurval dagurval referenced this pull request in bitcoinxt/bitcoinxt Dec 18, 2017

Merged

Add unittests for dbwrapper + blockchain.py rpc test #284

zkbot added a commit to zcash/zcash that referenced this pull request Jan 15, 2018

Auto merge of #2598 - str4d:2074-dbwrapper, r=<try>
Bitcoin 0.12+ dbwrapper improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6650
  - Only refactor - excludes obfuscation
- bitcoin/bitcoin#6777
  - Excluding obfuscation-related changes
- bitcoin/bitcoin#6865
- bitcoin/bitcoin#6823
- bitcoin/bitcoin#6873
- bitcoin/bitcoin#7927
  - Excluding first commit (already included) and second commit (obfuscation-related)
- bitcoin/bitcoin#8467

Part of #2074.

litecoinz-project added a commit to litecoinz-project/litecoinz that referenced this pull request Mar 15, 2018

zkbot added a commit to zcash/zcash that referenced this pull request Apr 3, 2018

Auto merge of #2598 - str4d:2074-dbwrapper, r=str4d
Bitcoin 0.12+ dbwrapper improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6650
  - Only refactor - excludes obfuscation
- bitcoin/bitcoin#6777
  - Excluding obfuscation-related changes
- bitcoin/bitcoin#6865
- bitcoin/bitcoin#6823
- bitcoin/bitcoin#6873
- bitcoin/bitcoin#7927
  - Excluding first commit (already included) and second commit (obfuscation-related)
- bitcoin/bitcoin#8467

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request Apr 3, 2018

Auto merge of #2598 - str4d:2074-dbwrapper, r=str4d
Bitcoin 0.12+ dbwrapper improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6650
  - Only refactor - excludes obfuscation
- bitcoin/bitcoin#6777
  - Excluding obfuscation-related changes
- bitcoin/bitcoin#6865
- bitcoin/bitcoin#6823
- bitcoin/bitcoin#6873
- bitcoin/bitcoin#7927
  - Excluding first commit (already included) and second commit (obfuscation-related)
- bitcoin/bitcoin#8467

Part of #2074.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment