Unobfuscate chainstate data in CCoinsViewDB::GetStats #6777

pull jamesob wants to merge 4 commits into bitcoin:master from jamesob:obfuscate_fix changing 6 files +245 −61

jamesob commented at 12:16 AM on October 8, 2015: member

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 commented at 12:28 AM on October 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)
jamesob commented at 12:31 AM on October 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.
sipa commented at 12:34 AM on October 8, 2015: member

jamesob: I think the obfuscation/deobfuscation should be done in Leveldbwrapper, not in the individual clients.
jamesob commented at 12:45 AM on October 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.
sipa commented at 12:54 AM on October 8, 2015: member

jamesob: see the code in #2802 for that :)
jamesob commented at 1:02 AM on October 8, 2015: member

@sipa ha! and like magic... wish writing code was always this easy ;). Will cherry-pick and layer on modifications if possible.
laanwj added the label Bug on Oct 8, 2015
laanwj added the label Priority High on Oct 8, 2015
laanwj commented at 2:15 PM on October 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.

laanwj commented at 2:42 PM on October 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);

laanwj commented at 2:48 PM on October 8, 2015: member

Tested ACK. gettxoutsetinfo works again after this.
jamesob commented at 3:34 PM on October 8, 2015: member

@laanwj good to hear. Thanks for testing.
jamesob commented at 4:16 PM on October 8, 2015: member

I've added some test coverage on the CLevelDB utilities. Provided travis is happy, I'm done adding changes.
jamesob force-pushed on Oct 8, 2015
jamesob force-pushed on Oct 8, 2015

Encapsulate CLevelDB iterators cleanly

Conflicts:
	src/leveldb.cpp
	src/leveldb.h
	src/txdb.cpp

3499ce1e1a

Handle obfuscation in CLevelDBIterator 0fdf8c80ee
jamesob force-pushed on Oct 8, 2015
in qa/rpc-tests/blockchain.py:None in 139df09438 outdated
```
  41 | +
  42 | +        assert_equal(res, {
  43 | +            u'total_amount': decimal.Decimal('8725.00000000'),
  44 | +            u'transactions': 200,
  45 | +            u'height': 200,
  46 | +            u'bestblock': u'6189e9cc58bedca8cb8e917cefe831839d296c2d12ae2e460066aa38d4a06f3e',
```
dexX7 commented at 11:12 AM on October 9, 2015:

Is this always deterministic?

dexX7 commented at 11:26 AM on October 9, 2015:

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 commented at 5:33 PM on October 9, 2015: member

@sipa any comments here?
sipa commented at 5:42 PM on October 9, 2015: member

Code ACK, I haven't reviewed the tests.
Add tests for gettxoutsetinfo, CLevelDBBatch, CLevelDBIterator
Thanks @dexX7.
1488506872
Refer to obfuscate_key via pointer in peripheral CLevelDB classes
cc @sipa
dcd8e27c65
jamesob force-pushed on Oct 9, 2015
jamesob commented at 9:38 PM on October 10, 2015: member

Ping on this. I think it's ready for merge.
laanwj commented at 10:22 AM on October 13, 2015: member

@jamesob Yes going to merge this now, I've been sidetracked by the upnp vulnerabilty the last few days.
laanwj merged this on Oct 13, 2015
laanwj closed this on Oct 13, 2015
laanwj referenced this in commit 9caaf6ed22 on Oct 13, 2015
laanwj referenced this in commit a8d781f863 on Oct 31, 2015
laanwj referenced this in commit ba03d14234 on Oct 31, 2015
laanwj referenced this in commit d6bf6ca2cc on Oct 31, 2015
laanwj referenced this in commit 2a92540fde on Oct 31, 2015
laanwj referenced this in commit 775d01d4e1 on Oct 31, 2015
laanwj referenced this in commit 8b7354bf2c on Oct 31, 2015
zkbot referenced this in commit 068e82e00a on Jan 15, 2018
litecoinz-project referenced this in commit fcc270706e on Mar 15, 2018
zkbot referenced this in commit 564119eb31 on Apr 3, 2018
zkbot referenced this in commit 77669b952b on Apr 3, 2018
random-zebra referenced this in commit 73d26f20e9 on May 27, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6804 [tests] Add basic coverage reporting for RPC tests #6919 Backport chainstate obfuscation to 0.11