Obfuscate chainstate #6650

pull jamesob wants to merge 1 commits into bitcoin:master from jamesob:obfuscate_chainstate changing 7 files +338 −22
  1. jamesob commented at 11:41 pm on September 7, 2015: member

    Fixes #6613 (discussion here: #4069)

    Obfuscate the leveldb-stored chainstate with a simple XOR to avoid spurious detection by anti-virus software.

    Chainstate obfuscation will happen atomically, i.e. either all or none of the database will be obfuscated. A runtime error will be thrown if a user attempts to invoke bitcoind -obfuscatechaindata with existing, unobfuscated chainstate data.

    On reindexing, all new chainstates will be obfuscated. until then, we specify a degenerate obfuscation key (\000\000...) that has no effect when XOR’d with existing data.

    TODO after Concept ACK

    • write tests covering CDataStream.Xor
    • write tests covering obfuscation ops with CLevelDBWrapper
  2. in src/txdb.cpp: in 7eb45a3e5a outdated 
    39@@ -40,7 +40,7 @@ void static BatchWriteHashBestChain(CLevelDBBatch &batch, const uint256 &hash) {
40     batch.Write(DB_BEST_BLOCK, hash);
41 }
42 
43-CCoinsViewDB::CCoinsViewDB(size_t nCacheSize, bool fMemory, bool fWipe) : db(GetDataDir() / "chainstate", nCacheSize, fMemory, fWipe) {
44+CCoinsViewDB::CCoinsViewDB(size_t nCacheSize, bool fMemory, bool fWipe) : db(GetDataDir() / "chainstate", nCacheSize, fMemory, fWipe, GetArg("-obfuscatechainstate", 0)) {
    sipa commented at 0:02 am on September 8, 2015:
    Use GetBoolArg, so it enables the -no prefix etc.
  3. sipa commented at 0:10 am on September 8, 2015: member
    Concept ACK, this looks pretty good already.
  4. gmaxwell commented at 0:17 am on September 8, 2015: contributor

    Concept and approach ACK. Obviously this will need a fair amount of testing. (In particular we should get an AV afflicted system and confirm this fixes it).

    I think it should be on by default. The people who need this won’t know they need it, anyone who doesn’t need it will quickly figure it out. (Do other people have opinions?)

    I’m not even sure if we should support turning it off? It’s easy enough to support (as your code shows) that maybe we just should; but I’d like to hear an argument for it.

    Thank you for working on this.

  5. sipa commented at 0:20 am on September 8, 2015: member
    @gmaxwell The only argument against supporting unobfuscated is 1) it would need upgrading code (but I think that’s pretty simple, though supporting stop/restart in the middle is not trivial) 2) it would break backward compatibility.
  6. jamesob commented at 0:22 am on September 8, 2015: member
    @gmaxwell I agree that having the obfuscation enabled is a sane (and safe) default, but is it acceptable for that runtime_error to be thrown on upgrade for everyone with an existing chainstate? Seems harsh. Is it desirable to find a more gentle way of doing the migration?
  7. luke-jr commented at 0:28 am on September 8, 2015: member
    This is why supporting mixed obfuscated/unobfuscated is nice: no slow upgrade process. :)
  8. jamesob commented at 1:23 am on September 8, 2015: member
    @luke-jr I do think the mixed migration is a better option if we’re going to enforce this (or even make it the default behavior). Will implement if @gmaxwell, @laanwj agree.
  9. jgarzik commented at 2:30 am on September 8, 2015: contributor

    As noted in #6613 (comment) a mix of cleartext and obfu-text does not actually fix the problem seem in the field for active users.

    Users reporting this problem will still see the problem after upgrading to a release with this feature.

  10. in src/leveldbwrapper.cpp: in 8e3b71d303 outdated 
    111+ * obfuscating XOR key. 
112+ */
113+std::string CLevelDBWrapper::CreateObfuscateKey() const {
114+    unsigned char buff[8];
115+    GetRandBytes(buff, 8);
116+    return std::string((const char *)buff);
    pstratem commented at 4:02 am on September 8, 2015:
    This should be std::string(&buff[0], &buff[8]);
  11. jamesob commented at 4:27 am on September 8, 2015: member
    I’ve integrated @gmaxwell’s suggestion and I think it’s looking much better.
  12. in src/leveldbwrapper.h: in 3bdc38b7ed outdated 
    194+    /**
195+     * Return true if the database managed by this class contains no entries.
196+     */
197+    bool IsEmpty()
198+    {
199+        leveldb::Iterator* it = NewIterator();
    dcousens commented at 5:00 am on September 8, 2015:
    Does this have to be allocated?
    jamesob commented at 5:19 am on September 8, 2015:
    Is there an alternative? apologies, I’m a C++ beginner :)
    jamesob commented at 4:11 am on September 9, 2015:
    Yeah, looks like this only comes allocated.
    dcousens commented at 6:23 am on September 9, 2015:
    Thanks for clearing that up @jamesob
    laanwj commented at 1:26 pm on September 29, 2015:
    Please use a boost::scoped_ptr<leveldb::Iterator> here, like in txdb.cpp, to automatically free the iterator when it goes out of scope.
  13. in src/leveldbwrapper.h: in 3bdc38b7ed outdated 
    43@@ -44,6 +44,8 @@ class CLevelDBBatch
44         CDataStream ssValue(SER_DISK, CLIENT_VERSION);
45         ssValue.reserve(ssValue.GetSerializeSize(value));
46         ssValue << value;
47+        if (!obfuscate_key.empty())
    pstratem commented at 5:25 am on September 8, 2015:
    You can drop the conditional.
  14. in src/leveldbwrapper.h: in 3bdc38b7ed outdated 
    33@@ -34,7 +34,7 @@ class CLevelDBBatch
34 
35 public:
36     template <typename K, typename V>
37-    void Write(const K& key, const V& value)
38+    void Write(const K& key, const V& value, const std::string& obfuscate_key = std::string())
    pstratem commented at 5:26 am on September 8, 2015:
    And the default.
    jamesob commented at 5:45 am on September 8, 2015:

    I initially did that, but this is called from contexts where obfuscate_key isn’t in scope and hasn’t been defined, namely CBlockTreeDB::WriteTxIndex. Now that I’m looking at it, there’re some missing parameterizations of obfuscate_key (e.g. here).

    It isn’t immediately obvious to me how to convey db.obfuscate_key to that context… might need some refactoring.

  15. jamesob commented at 7:06 am on September 8, 2015: member
    CLevelDBBatch needs a good refactoring at some point… unless there’s something I’m not seeing, it should probably be absorbed into CLevelDBWrapper.That may be out of scope for this PR, though.
  16. jamesob commented at 7:08 am on September 8, 2015: member
    Now that I think about it, the Batch refactoring may be more contained than I thought. Anyone have qualms with me doing it in this PR?
  17. laanwj added the label UTXO Db and Indexes on Sep 8, 2015
  18. in src/leveldbwrapper.cpp: in 957feedc25 outdated 
    82+            // this won't disrupt existing data.
83+            obfuscate_key = std::string(OBFUSCATE_KEY_NUM_BYTES, '\000');
84+        }
85+
86+        Write(OBFUSCATE_KEY_KEY, obfuscate_key);
87+        LogPrintf("Wrote new obfuscate key '%s'\n", obfuscate_key);
    dexX7 commented at 8:22 pm on September 8, 2015:
    Hmm.. obfuscate_key is not necessarily printable. Maybe it would be better to log a hexified version of the key?
  19. in src/leveldbwrapper.h: in 957feedc25 outdated 
    149-        batch.Write(key, value);
150-        return WriteBatch(batch, fSync);
151+        CLevelDBBatch* batch = GetBatch();
152+        batch->Write(key, value);
153+        bool written = WriteBatch(batch, fSync);
154+        delete batch;
    dexX7 commented at 8:37 pm on September 8, 2015:

    As far as I can see it’s not guaranteed that batch is deleted, namely if WriteBatch -> HandleError -> throw.

    How about using something like boost::scoped_ptr to avoid the manual cleanup altogether?

    jamesob commented at 8:40 pm on September 8, 2015:
    Sounds good to me, though honestly I’m hoping to obviate the heap allocations entirely by absorbing CLevelDBBatch into CLevelDBWrapper… this is pending an okay from @sipa @gmaxwell.
  20. jamesob commented at 4:16 am on September 9, 2015: member

    Okay, I think this is in a good place. Obfuscation now happens automatically whenever the chainstate is initialized (either via -reindex or otherwise). The degenerate key that we use ("\000\000...") isn’t persisted, but it’s specified during class construction in the case that we can’t do real obfuscate due to existing data.

    IMO this is ready to go, modulo the addition of a few unittests.

  21. jamesob closed this on Sep 9, 2015
  22. jamesob reopened this on Sep 9, 2015
  23. in src/leveldbwrapper.h: in 1f6cb12a08 outdated 
    120+     * @param[in] unobfuscate   This should always be true unless we're reading
121+     *                          OBFUSCATE_KEY_KEY.
122+     */
123     template <typename K, typename V>
124-    bool Read(const K& key, V& value) const throw(leveldb_error)
125+    bool Read(const K& key, V& value, bool unobfuscate = true) const throw(leveldb_error)
    dexX7 commented at 10:24 pm on September 9, 2015:
    Is there any other case where unobfuscate == false, except in the constructor? If not, you might get away without it (and the condition in L140), because obfuscate_key is still blank during the read of the actual key, if I’m not mistaken.
    jamesob commented at 11:12 pm on September 9, 2015:
    Ah yep, you’re totally right. Initially I rationalized this by telling myself this param would be convenient for tests, but looks like there’s no real need for it now. Will push a fix.
  24. jamesob commented at 8:02 am on September 10, 2015: member

    OK, unittests here are done. If anyone would like to see additional testcases, let me know, but I think I’ve covered the two important cases:

    1. No existing chainstate/ data (obfuscation)
    2. Existing data (no-op obfuscation)

    It’d be great if we could get some testers who are affected by anti-virus software (@KyrosKrane ?) trying out this changeset.

  25. KyrosKrane commented at 8:27 am on September 12, 2015: none
    I’d be more than happy to test, but I’m new to testing with Bitcoin off Github. Do I need to download and compile from source?
  26. fanquake commented at 8:55 am on September 12, 2015: member

    @KyrosKrane if your comfortable with, and are able to compile from source then the github-merge script in /contrib/devtools is an easy way to test pull requests. Once you’ve cloned the repo, cd in to the directory and run

    0contrib/devtools/github-merge.sh 6650

    That will pull in the code changes from this pull request, and you can then compile as normal.

    Otherwise, @jonasschnelli has gitian buillt binaries for certain pull requests available from https://builds.jonasschnelli.ch/pulls/. He can probably spin up a build for this pull once he gets a chance.

  27. jonasschnelli commented at 10:31 am on September 12, 2015: contributor
    https://builds.jonasschnelli.ch/pulls/6650/
  28. KyrosKrane commented at 12:49 pm on September 12, 2015: none

    Thanks for the build; that will make this much faster for me. :)

    Here’s what I’ve done so far.

    1. Backed up my existing bitcoin config directory.
    2. Backed up my existing bitcoin exe files.
    3. Downloaded the test build from @jonasschnelli ’s site
    4. Deleted everything in my config dir other than the basic config file (just has my connection info and settings)
    5. Replaced the exes in my Program Files directory with the exes from the test build.
    6. Removed the antivirus exclusion from the bitcoin config directory.
    7. Started the test exe.

    I’m now waiting for it to download the blockchain (I have a second node on my local network, so it shouldn’t be too slow) and rebuild the chainstate and index files. Once that completes, I’ll do a manual scan of the config dir and see if Norton yells at me.

  29. jamesob commented at 6:48 pm on September 12, 2015: member
    @KyrosKrane that sounds great, thanks for your help.
  30. MarcoFalke commented at 6:59 pm on September 12, 2015: member
    @KyrosKrane If you have the time, could you also test with the “infected” data directory, verify that by just running the current bitcoin core version does not fix the AV alert? Afterward, you could check if a -reindex fixes the AV problem on the “infected” data directory.
  31. KyrosKrane commented at 7:32 pm on September 12, 2015: none
    You read my mind. :) That’s exactly what I’m going to do when this current run ends. It will probably be tomorrow, though; the re-download is taking longer than I thought, even though my other node is locally connected.
  32. KyrosKrane commented at 5:08 pm on September 13, 2015: none

    OK, update for today. The re-download completed overnight. No antivirus alerts. I manually scanned the bitcoin data dir. No alerts. This is as expected.

    Next, I stopped bitcoin-qt.exe and renamed the data directory. I copied my original (unobfuscated) blocks and chainstate folders, and the bitcoin.conf file from my backup into the now-empty data directory. I tested it by scanning that freshly-copied directory - no antivirus alert. This isn’t good. It should be alerting at this point.

    I tried renaming the files from *.ldb to *.sst, but that didn’t help. I even tried renaming them to *.exe, which got Norton to actually scan the files in depth, but still no alerts.

    I’m going to continue testing the debug build to see if it behaves as expected.

  33. MarcoFalke commented at 5:12 pm on September 13, 2015: member
    Do you know if your AV “fixed” the “infected” data directory back when it originally happened?
  34. KyrosKrane commented at 5:19 pm on September 13, 2015: none
    Well, it certainly broke bitcoin-qt and caused it to crash. It wouldn’t restart afterwards until I did a -reindex, which promptly broke a second time. Then I wised up and added the exclusion to my antivirus, and a second -reindex worked. I’ll try doing a reindex in a bit; I want to start bitcoin-qt normally first and see if it will read the (unobfuscated) files correctly.
  35. KyrosKrane commented at 5:45 pm on September 13, 2015: none

    bitcoin-qt happily opened with the unobfuscated files. I tested with an old wallet file I had, and it quickly rescanned the blockchain and updated the balance. (I now have three satoshis more than this morning, yay me!) Next, I exited bitcoin-qt, opened a command prompt, and ran bitcoin-qt.exe -reindex. This errored out. I repeated, in case this was a one time thing, and it gave me the same error. The message is “Error opening block database”. I attached a screenshot.

    block database error

  36. jamesob commented at 6:09 pm on September 13, 2015: member

    @KyrosKrane can you check your log file for anything that might indicate what went wrong, e.g. a stacktrace?

    On Sunday, September 13, 2015, KyrosKrane notifications@github.com wrote:

    bitcoin-qt happily opened with the unobfuscated files. I tested with an old wallet file I had, and it quickly rescanned the blockchain and updated the balance. (I now have three satoshis more than this morning, yay me!) Next, I exited bitcoin-qt, opened a command prompt, and ran bitcoin-qt.exe -reindex. This errored out. I repeated, in case this was a one time thing, and it gave me the same error. The message is “Error opening block database”. I attached a screenshot.

    [image: block database error] https://cloud.githubusercontent.com/assets/458727/9837936/b6329116-5a60-11e5-9e59-4e0369f36dc2.png

    — Reply to this email directly or view it on GitHub #6650 (comment).

  37. KyrosKrane commented at 6:22 pm on September 13, 2015: none

    I’m slowing down in my old age … I should have thought of that. Debug.log file below.

      02015-09-13 17:36:27 Bitcoin version v0.11.99.0-f1803c1 (2015-09-12 12:01:31 +0200)
  12015-09-13 17:36:27 AppInit2: parameter interaction: -connect set -> setting -dnsseed=0
  22015-09-13 17:36:27 GUI: "registerShutdownBlockReason: Successfully registered: Bitcoin Core didn't yet exit safely..."
  32015-09-13 17:36:27 Using OpenSSL version OpenSSL 1.0.1k 8 Jan 2015
  42015-09-13 17:36:27 Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
  52015-09-13 17:36:27 Default data directory D:\Users\USERNAME\AppData\Roaming\Bitcoin
  62015-09-13 17:36:27 Using data directory D:\Users\USERNAME\AppData\Roaming\Bitcoin
  72015-09-13 17:36:27 Using config file D:\Users\USERNAME\AppData\Roaming\Bitcoin\bitcoin.conf
  82015-09-13 17:36:27 Using at most 125 connections (2048 file descriptors available)
  92015-09-13 17:36:27 Using 4 threads for script verification
 102015-09-13 17:36:27 Using wallet wallet.dat
 112015-09-13 17:36:27 scheduler thread start
 122015-09-13 17:36:27 init message: Verifying wallet...
 132015-09-13 17:36:27 CDBEnv::Open: LogDir=D:\Users\USERNAME\AppData\Roaming\Bitcoin\database ErrorFile=D:\Users\USERNAME\AppData\Roaming\Bitcoin\db.log
 142015-09-13 17:36:27 Bound to [::]:8333
 152015-09-13 17:36:27 Bound to 0.0.0.0:8333
 162015-09-13 17:36:27 Cache configuration:
 172015-09-13 17:36:27 * Using 2.0MiB for block index database
 182015-09-13 17:36:27 * Using 32.5MiB for chain state database
 192015-09-13 17:36:27 * Using 65.5MiB for in-memory UTXO set
 202015-09-13 17:36:27 init message: Loading block index...
 212015-09-13 17:36:27 Wiping LevelDB in D:\Users\USERNAME\AppData\Roaming\Bitcoin\blocks\index
 222015-09-13 17:36:29 scheduler thread interrupt
 232015-09-13 17:36:29 Shutdown: In progress...
 242015-09-13 17:36:29 StopNode()
 252015-09-13 17:36:29 Shutdown: done
 262015-09-13 17:37:14 
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 462015-09-13 17:37:14 Bitcoin version v0.11.99.0-f1803c1 (2015-09-12 12:01:31 +0200)
 472015-09-13 17:37:14 AppInit2: parameter interaction: -connect set -> setting -dnsseed=0
 482015-09-13 17:37:14 GUI: "registerShutdownBlockReason: Successfully registered: Bitcoin Core didn't yet exit safely..."
 492015-09-13 17:37:14 Using OpenSSL version OpenSSL 1.0.1k 8 Jan 2015
 502015-09-13 17:37:14 Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
 512015-09-13 17:37:14 Default data directory D:\Users\USERNAME\AppData\Roaming\Bitcoin
 522015-09-13 17:37:14 Using data directory D:\Users\USERNAME\AppData\Roaming\Bitcoin
 532015-09-13 17:37:14 Using config file D:\Users\USERNAME\AppData\Roaming\Bitcoin\bitcoin.conf
 542015-09-13 17:37:14 Using at most 125 connections (2048 file descriptors available)
 552015-09-13 17:37:14 Using 4 threads for script verification
 562015-09-13 17:37:14 Using wallet wallet.dat
 572015-09-13 17:37:14 scheduler thread start
 582015-09-13 17:37:14 init message: Verifying wallet...
 592015-09-13 17:37:14 CDBEnv::Open: LogDir=D:\Users\USERNAME\AppData\Roaming\Bitcoin\database ErrorFile=D:\Users\USERNAME\AppData\Roaming\Bitcoin\db.log
 602015-09-13 17:37:14 Bound to [::]:8333
 612015-09-13 17:37:14 Bound to 0.0.0.0:8333
 622015-09-13 17:37:14 Cache configuration:
 632015-09-13 17:37:14 * Using 2.0MiB for block index database
 642015-09-13 17:37:14 * Using 32.5MiB for chain state database
 652015-09-13 17:37:14 * Using 65.5MiB for in-memory UTXO set
 662015-09-13 17:37:14 init message: Loading block index...
 672015-09-13 17:37:14 Wiping LevelDB in D:\Users\USERNAME\AppData\Roaming\Bitcoin\blocks\index
 682015-09-13 17:37:14 Opening LevelDB in D:\Users\USERNAME\AppData\Roaming\Bitcoin\blocks\index
 692015-09-13 17:37:14 Opened LevelDB successfully
 702015-09-13 17:37:14 Wiping LevelDB in D:\Users\USERNAME\AppData\Roaming\Bitcoin\chainstate
 712015-09-13 17:37:19 scheduler thread interrupt
 722015-09-13 17:37:19 Shutdown: In progress...
 732015-09-13 17:37:19 StopNode()
 742015-09-13 17:37:19 Shutdown: done
 752015-09-13 17:37:38 
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 952015-09-13 17:37:38 Bitcoin version v0.11.99.0-f1803c1 (2015-09-12 12:01:31 +0200)
 962015-09-13 17:37:38 AppInit2: parameter interaction: -connect set -> setting -dnsseed=0
 972015-09-13 17:37:38 GUI: "registerShutdownBlockReason: Successfully registered: Bitcoin Core didn't yet exit safely..."
 982015-09-13 17:37:38 Using OpenSSL version OpenSSL 1.0.1k 8 Jan 2015
 992015-09-13 17:37:38 Using BerkeleyDB version Berkeley DB 4.8.30: (April  9, 2010)
1002015-09-13 17:37:38 Default data directory D:\Users\USERNAME\AppData\Roaming\Bitcoin
1012015-09-13 17:37:38 Using data directory D:\Users\USERNAME\AppData\Roaming\Bitcoin
1022015-09-13 17:37:38 Using config file D:\Users\USERNAME\AppData\Roaming\Bitcoin\bitcoin.conf
1032015-09-13 17:37:38 Using at most 125 connections (2048 file descriptors available)
1042015-09-13 17:37:38 Using 4 threads for script verification
1052015-09-13 17:37:38 Using wallet wallet.dat
1062015-09-13 17:37:38 scheduler thread start
1072015-09-13 17:37:38 init message: Verifying wallet...
1082015-09-13 17:37:38 CDBEnv::Open: LogDir=D:\Users\USERNAME\AppData\Roaming\Bitcoin\database ErrorFile=D:\Users\USERNAME\AppData\Roaming\Bitcoin\db.log
1092015-09-13 17:37:38 Bound to [::]:8333
1102015-09-13 17:37:38 Bound to 0.0.0.0:8333
1112015-09-13 17:37:38 Cache configuration:
1122015-09-13 17:37:38 * Using 2.0MiB for block index database
1132015-09-13 17:37:38 * Using 32.5MiB for chain state database
1142015-09-13 17:37:38 * Using 65.5MiB for in-memory UTXO set
1152015-09-13 17:37:38 init message: Loading block index...
1162015-09-13 17:37:38 Wiping LevelDB in D:\Users\USERNAME\AppData\Roaming\Bitcoin\blocks\index
1172015-09-13 17:42:00 scheduler thread interrupt
1182015-09-13 17:42:00 Shutdown: In progress...
1192015-09-13 17:42:00 StopNode()
1202015-09-13 17:42:00 Shutdown: done

    The chainstate directory was deleted, as was the index directory under the blocks directory. Under my main bitcoin data dir, there was a new directory named “database” with a single file in it named log.0000000001. The contents appear binary.

  38. jamesob commented at 1:23 am on September 15, 2015: member

    @KyrosKrane sorry it’s taken me a bit to respond; I finally have some time to sit down and take a look. I’m going to add a testcase that simulates a -reindex at the CLevelDBWrapper layer to see if that will reproduce this failure for me locally.

    Thanks again for putting in the work to test this.

  39. jamesob commented at 4:39 am on September 17, 2015: member
    @KyrosKrane dang, can’t reproduce with a unittest. Any chance that running with the -debug flag yields any more information? e.g. bitcoind -reindex -debug? If not, I’ll have to try to reproduce on my end with a full dataset. As a side-note, I wonder why the behavior you’re seeing isn’t reflected in qa/rpc-tests/reindex.py…
  40. KyrosKrane commented at 9:13 am on September 18, 2015: none

    OK, now this is weird. I ran bitcoin-qt.exe -reindex -debug, and it worked like a champ. It took almost as long as re-downloading the entire blockchain again; I had to leave it running overnight. But it didn’t give any errors and it seems to have completed successfully. The debug.log file is a whopping 335MB uncompressed, so I’m not going to post that unless someone really needs it.

    I’m going to restore my original, unobfuscated data directory, then try running -reindex -debug on it to see if it causes the error. If that works, I’ll restore once again, then do just -reindex to see whether I get the error once more.

  41. KyrosKrane commented at 10:52 am on September 18, 2015: none
    OK, I’ve tested this twice now. With my original, unobfuscated data directory, if I do bitcoin-qt.exe -reindex -debug, it will work successfully. If I only do bitcoin-qt.exe -reindex, it fails with the error I posted above.
  42. jamesob commented at 2:34 am on September 21, 2015: member
    @KyrosKrane very odd… the presence of the -debug flag doesn’t materially affect the reindexing one way or the other on my platform (ubuntu 14.04). Wondering if there’s anything in this changeset that would explain -debug showing different behavior on a windows system.
  43. in src/leveldbwrapper.cpp: in b1101f8693 outdated 
    117+ * obfuscating XOR key. 
118+ */
119+std::string CLevelDBWrapper::CreateObfuscateKey() const {
120+    unsigned char buff[OBFUSCATE_KEY_NUM_BYTES];
121+    GetRandBytes(buff, OBFUSCATE_KEY_NUM_BYTES);
122+    return std::string(&buff[0], &buff[OBFUSCATE_KEY_NUM_BYTES]);
    KyrosKrane commented at 8:01 pm on September 21, 2015:
    Sorry, I’m a C++ beginner here, but I have a question about the second parameter on this string constructor call. I think (based on constuctor 5 in this link) it should just be OBFUSCATE_KEY_NUM_BYTES. Right now, you’re passing in the address of the first and last bytes of the buffer you created, rather than the address of the buffer and the number of bytes. (In fact, the second parameter would be the address of the first byte after the buffer!) Or are you using a different constructor? If this is the right constructor, wouldn’t this lead to a buffer overflow issue? (Fair warning, I probably totally misunderstood what you’re doing here, so please double check my logic!)
    jamesob commented at 8:52 pm on September 21, 2015:

    @KyrosKrane being a C++ newbie myself, I screwed this up the first time around; @pstratem gave me some guidance on the proper incantation… though upon reading the docs again, I very much agree that what you’re saying looks correct, so I gave it a shot. Unfortunately, the compiler spat back a

    0leveldbwrapper.cpp:122:57: error: call of overloaded ‘basic_string(unsigned char*, const unsigned int&)’ is ambiguous
1     return std::string(&buff[0], OBFUSCATE_KEY_NUM_BYTES);

    On further investigation, I realized this is because buff is an unsigned char array, not a char array – the two types are “unrelated” types in C++, meaning (apparently) one can’t easily be substituted for another. I believe this invocation of the constructor falls into the (7) range invocation with unsigned char* being the templated type.

    I think you might be onto something here though – could be that windows is treating the implicit conversion from unsigned char * to string oddly (anyone else think that’s plausible?). I’m going to look for a simpler, less confusing way to do this. Maybe we just keep obfuscate_key as an unsigned char array instead of a string.

    Thanks again for helping to dig into this problem.

    dexX7 commented at 9:08 pm on September 21, 2015:

    It’s 7, a range constructor. Note that the second argument points to the position after the last element in the range. Pointers can be used like iterators, and it should be fine.

    https://ideone.com/tBWxwM

    laanwj commented at 1:25 pm on September 29, 2015:
    Looks fine to me.
  44. KyrosKrane commented at 9:31 pm on September 21, 2015: none
    Thanks both for the clarifications!
  45. in src/leveldbwrapper.cpp: in b1101f8693 outdated 
    82+
83+        // Write `new_key` so we don't obfuscate the key with itself
84+        Write(OBFUSCATE_KEY_KEY, new_key);
85+        obfuscate_key = new_key;
86+
87+        LogPrintf("Wrote new obfuscate key '%s'\n", ObfuscateKeyHex());
    laanwj commented at 1:23 pm on September 29, 2015:
    This message is too vague. "Wrote new database obfuscation key for %s: %s", path.string(), ObfuscateKeyHex() maybe?
  46. in src/leveldbwrapper.cpp: in b1101f8693 outdated 
    114+
115+/**
116+ * Returns a string (consisting of 8 random bytes) suitable for use as an 
117+ * obfuscating XOR key. 
118+ */
119+std::string CLevelDBWrapper::CreateObfuscateKey() const {
    laanwj commented at 1:24 pm on September 29, 2015:
    Code style nit: { on next line for function declaration (also for the other new functions)
  47. in src/leveldbwrapper.h: in b1101f8693 outdated 
    213+    }
214+
215+    /**
216+     * Accessor for obfuscate_key.
217+     */
218+    std::string ObfuscateKey() const { return obfuscate_key; }
    laanwj commented at 1:29 pm on September 29, 2015:

    I’d make this signature const std::string& GetObfuscateKey() const

    • Otherwise it looks like an action: Obfuscate the key!
    • As this returns a field of the object, we can avoid a copy by returning a const reference

    Edit: oops, const-correctness

  48. in src/leveldbwrapper.h: in b1101f8693 outdated 
    218+    std::string ObfuscateKey() const { return obfuscate_key; }
219+
220+    /**
221+     * Return the obfuscate_key as a hex-formatted string.
222+     */
223+    std::string ObfuscateKeyHex()
    laanwj commented at 1:31 pm on September 29, 2015:
    Same here: make it GetObfuscateKeyHex(). Also don’t re-implement the hex logic here, but use HexStr(...).
    jamesob commented at 4:03 pm on September 29, 2015:
    Ah, thanks. I was looking for an existing implementation of that… :)
  49. in src/leveldbwrapper.cpp: in b1101f8693 outdated 
    83+        // Write `new_key` so we don't obfuscate the key with itself
84+        Write(OBFUSCATE_KEY_KEY, new_key);
85+        obfuscate_key = new_key;
86+
87+        LogPrintf("Wrote new obfuscate key '%s'\n", ObfuscateKeyHex());
88+    }
    laanwj commented at 1:35 pm on September 29, 2015:
    Please also log the obfuscation key if it did exist yet, or is all-zeros: this will help troubleshooting, for example when people have problems with their virusscanner and block database files we can see if obfuscation is being used.
  50. laanwj commented at 1:36 pm on September 29, 2015: member
    Looks good to me apart from the mentioned small things. Will start testing this.
  51. in src/leveldbwrapper.h: in b1101f8693 outdated 
    198@@ -168,6 +199,44 @@ class CLevelDBWrapper
199     {
200         return pdb->NewIterator(iteroptions);
201     }
202+
203+    /**
    laanwj commented at 2:09 pm on September 29, 2015:
    I’d put the implementation of these functions in leveldbwrapper.cpp instead of the header file (unless there’s a reason I’m overlooking)
    jamesob commented at 4:02 pm on September 29, 2015:
    I implemented these in the header to maintain consistency with much of the rest of CLevelDBWrapper, which is also defined here. I’ll move what’s below over to the .cpp file, but we should probably think about moving the rest of it out of the header at some point.
  52. in src/streams.h: in dbbb3ff64d outdated 
    300+    /**
301+     * XOR the contents of this stream with a certain key.
302+     *
303+     * @param[in] key    The key used to XOR the data in this stream.
304+     */
305+    void Xor(const std::string& key) {
    laanwj commented at 6:04 pm on September 29, 2015:

    To save a % operation for every byte (which is essentially a division!) and add a sanity check for empty key, I suggest:

    0    void Xor(const std::string& key)
1    {
2        assert(key.size() > 0);
3        for (size_type i = 0, j = 0; i != size(); i++) {
4            vch[i] ^= key[j++];
5            if (j == key.size())
6                j = 0;
7        }
8    }
    laanwj commented at 6:06 pm on September 29, 2015:
    As this is task-specific ideally we wouldn’t have this operation on CDataStream itself, but as a function that manipulates a CDataStream.
    jamesob commented at 4:51 am on September 30, 2015:

    Really good suggestion, will integrate. Two questions:

    • won’t a key that is "\000..." have .size() == 0? should we just return early if that’s the case instead of blowing up with an assert?
    • I put this in CDataStream because, basically, it has to mutate vch. That’s a protected member variable, so a method Xor became. Is there an alternate way to do this that you’d prefer?
    laanwj commented at 7:01 am on September 30, 2015:
    a string containing zeros is entirely different from a zero-length string. The assertion would be triggered by Xor(""), which is clearly undefined. (an empty string would be repeated infinite times to cover the buffer?)
    jamesob commented at 7:16 am on September 30, 2015:
    I think you misunderstand… we use a string consisting entirely of null-terminators (\000) as a default obfuscation key. Given https://coderpad.io/2GF6EKCD , the default obfuscation key would cause your code above to raise an assertion error. I went with an early return to avoid this.
    laanwj commented at 7:29 am on September 30, 2015:

    std::string doesn’t use ‘NULL terminators’, they are 8-bit-clean.

    Also NULL-terminated strings don’t matter here at all: you should do obfuscation with a sequence of bytes.

    The length of [0,0,0,0,0,0,0,0] is 8 bytes just as well as [255,255,255,255,255,255,255,255] is. If not, that’s a bug.

    e.g. if your obfuscation key is [1,0,1,2,3,4,5,6], you don’t want to loop just ones.

    laanwj commented at 7:34 am on September 30, 2015:

    The problem is that you use eg. std::string("\000\000\000"). If you do that, C++ will use strlen() to determine the length, which looks for 0 characters. Pass in a length explicitly.

    So do either:

    0std::string(3, '\000');  // Create a string with three 0 bytes

    Or

    0static const char data[] = {0,0,0};
1std::string(data, data+sizeof(data));

    …

    Don’t use the NUL-terminated string constructor for raw data that can contain 0.

    jamesob commented at 3:23 pm on September 30, 2015:
    Ah, I see now. Thanks for typing up the examples on coderpad. This is more of my C++ inexperience coming out :).
    jamesob commented at 3:26 pm on September 30, 2015:
    Do you think it still makes sense to do an early exit if the value of the key amounts to a char array of 0s and thus will result in a noop for the xor?
    laanwj commented at 4:18 pm on September 30, 2015:

    Do you think it still makes sense to do an early exit if the value of the key amounts to a char array of 0s and thus will result in a noop for the xor?

    Well I really don’t mind in this case. In general, if something is always a coding bug, and could result in dangerous situations if left unchecked, it needs to be asserted. But having Xor("") as a no-op seems reasonable to me too.

  53. laanwj commented at 4:21 pm on September 30, 2015: member
    BTW; seems to work great. I have encountered no problems while testing this, either with an old chainstate or after a reindex (also tested with pruning).
  54. laanwj commented at 8:10 am on October 1, 2015: member

    ACK after squash.

    I do think we need to mention this concisely in release_notes.md:

    • a database built on 0.12+ cannot be used with older versions, but downgrading an existing install is not a problem
    • third-party tools that parse the UTXO database (I don’t know of any, I have https://github.com/laanwj/blockdb-troubleshoot but that only reads the block db) have to be updated
  55. paveljanik commented at 8:17 am on October 1, 2015: contributor
    We should also describe, how to continue having unobfuscated database…
  56. dexX7 commented at 9:59 am on October 1, 2015: contributor
    Given that there were several issues with using std::string so far, I’m wondering, whether something else may be used instead, such as std::vector<unsigned char>, to prevent potential pitfalls altogether?
  57. jamesob commented at 2:48 pm on October 1, 2015: member
    @paveljanik any new chainstate databases (including those built via -reindex) will be obfuscated; it’s not optional. See discussion above. @dexX7 I think that’s a great suggestion, and I’ve been inclined to try something like that. @laanwj are you in favor of making obfuscate_key a std::vector<unsigned char> in lieu of a std::string? I think it’ll make things much less awkward.
  58. sipa commented at 2:51 pm on October 1, 2015: member
    @jamesob I’m in favor of turning it into a std::vector<unsigned char>. No reason why it would be a string.
  59. jamesob commented at 3:17 pm on October 1, 2015: member
    obfuscate_key is now a vector<unsigned char>, which feels much better. If no one else has any outstanding issues, I’ll squash.
  60. laanwj commented at 3:37 pm on October 1, 2015: member
    making it a std::vector<unsigned char> (or equivalently, std::vector<uint8_t>) is a great idea, it avoids the initializer issue completely
  61. jamesob force-pushed on Oct 1, 2015
  62. jamesob force-pushed on Oct 1, 2015
  63. jamesob commented at 3:57 pm on October 1, 2015: member
    Squashed.
  64. dexX7 commented at 5:27 pm on October 1, 2015: contributor

    I tested the obfuscation in regtest mode, and started with unobfuscated 0.10 DBs.

    It works mostly as expected, though I faced a few potential issues:

    • the block index database isn’t obfuscated (probably not needed?)
    • client hung/froze after the first reindexing (this was not reproducible)
    • after that, the DBs were corrupted, potentially due to the freeze + forced shutdown (also not reproducible)
    • the first time I disabled the txindex with an obfuscated chainstate, the DB was appearingly in a bad state (not not reproducible)

    I took some notes here: https://gist.github.com/dexX7/d757485228b3ef7e37e9

  65. jamesob commented at 6:52 pm on October 1, 2015: member

    @dexX7 thanks for the detailed notes; very helpful.

    the block index database isn’t obfuscated (probably not needed?)

    Yeah this is by design, and per discussion in the issue thread.

    not not reproducible

    Is that a typo, or do you actually mean they were reproducible issues?

  66. dexX7 commented at 7:00 pm on October 1, 2015: contributor

    Sorry, this was probably a bit confusing! :)

    I noticed each issue only once, and unfortunally (or fortunally? hehe) I was not able to reproduce them. All subsequent attempts went fine, and I successfully disabled and enabled the -txindex multiple times. I also reindexed more than once unobfuscated 0.10 based databases with the patched client.

  67. jamesob commented at 7:09 pm on October 1, 2015: member
    May be similar to what @KyrosKrane was reporting earlier, though I have to admit I’m mostly stumped. I thought it may have something to do with Reading obfuscate_keys that haven’t been written out to leveldb (in the case of the block index db), but after looking at the code I really doubt that’s the case. If anyone has guesses for likely trouble spots, I’m glad to push more changes.
  68. in src/leveldbwrapper.h: in 12a4813dd7 outdated 
    116+     */
117+    CLevelDBWrapper(const boost::filesystem::path& path, size_t nCacheSize, bool fMemory = false, bool fWipe = false, bool obfuscate = false);
118     ~CLevelDBWrapper();
119 
120+    /**
121+     * @param[in] unobfuscate   This should always be true unless we're reading
    KyrosKrane commented at 12:19 pm on October 2, 2015:
    Is this comment still correct? I don’t see this parameter referenced anywhere.
    jamesob commented at 11:34 pm on October 5, 2015:
    Good catch; will remove.
    jamesob commented at 2:51 pm on October 6, 2015:
    Fixed; thanks!
  69. CodeShark commented at 6:52 am on October 4, 2015: contributor
    utACK
  70. Add chainstate obfuscation to avoid spurious antivirus detection 
    Adds an `obfuscate` parameter to `CLevelDBWrapper` and makes use of it
for all new chainstate stores built via `CCoinsViewDB`. Also adds an
`Xor` method to `CDataStream`.

Thanks to @sipa @laanwj @pstratem @dexX7 @KyrosKrane @gmaxwell.
    42cb388167
  71. jamesob force-pushed on Oct 6, 2015
  72. laanwj commented at 3:42 pm on October 6, 2015: member
    I went over the code a few times and cannot find any changes that would cause it to hang or get in an infinite loop. Also haven’t been able to reproduce any such behavior. So I’m just going ahead and merge this.
  73. laanwj merged this on Oct 6, 2015
  74. laanwj closed this on Oct 6, 2015
  75. laanwj referenced this in commit 4fac576c61 on Oct 6, 2015
  76. jamesob deleted the branch on Oct 6, 2015
  77. MarcoFalke commented at 3:57 pm on October 7, 2015: member

    @KyrosKrane [https://github.com/bitcoin/bitcoin/pull/6650#issuecomment-141416946]: OK, I’ve tested this twice now. With my original, unobfuscated data directory, if I do bitcoin-qt.exe -reindex -debug, it will work successfully. If I only do bitcoin-qt.exe -reindex, it fails with the error I posted above.

    Can you still reproduce with bitcoin/master? Maybe this should go into a new issue then.

  78. KyrosKrane commented at 7:28 pm on October 7, 2015: none

    @MarcoFalke I used the test build from here:

    https://bitcoin.jonasschnelli.ch/nightlybuilds/2015-10-07/ File name bitcoin-0.11.99-win64.zip

    The behavior has now changed. -reindex still crashes as before, but now so does -reindex -debug. This is new.

    Just to make sure this isn’t because of some error in my process, I want to document how I got to this point. I started with my normal production environment, which I got by using the Windows x64 setup file. I ran this, completed the installation normally, started bitcoin-qt using the Start menu shortcut, and downloaded the blockchain normally, until it was all updated. Then I completely exited bitcoin-qt.exe. I downloaded the test build from the links in this thread and unzipped them. Then I just replaced the exe files in my program files directory - bitcoin-qt.exe, bitcoind.exe, and bitcoin-cli.exe. I left everything else unchanged. Then I opened a command window, switched to the bitcoin directory under program files, and ran the executable from the command line as above.

    If it would be helpful, I can upload my data directory somewhere, minus the wallet file. I’m zipping it up now, and I’ll see if I can find a file upload site that can take a file that big.

  79. KyrosKrane commented at 9:20 am on October 10, 2015: none

    I’m throwing up the file into my Dropbox now, if anyone wants it. This is my bitcoin data directory, with minimal changes (wallet removed, conf file modified to change my RPC username and password). It’s about 33GB, so it may take a bit of time to finish uploading. You can download it here.

    https://www.dropbox.com/s/k4zfs7f6qmevvbu/Bitcoin%20backup%202015-10.7z?dl=0

  80. dexX7 commented at 2:22 pm on October 15, 2015: contributor

    I really appreciate your effort, @KyrosKrane! Unfortunally, and to be honest, I’m not sure where to go from here. I saw DB corruption once too, while testing this PR, but I’m not able to reproduce it.

    Since you mentioned -reindex causes the crashes, I’m wondering, whether this is also the case with a version without obfuscation? As far as I can see the nightly build from 2015-10-06 should be the latest nightly build before this PR was merged.

    If this turns out to be the case, then it’s an issue with the master, and not with the obfuscation, which would be good to know.

  81. laanwj referenced this in commit 11d88c2ca2 on Oct 31, 2015
  82. kazcw referenced this in commit 7a539dec7c on Apr 22, 2016
  83. zkbot referenced this in commit 068e82e00a on Jan 15, 2018
  84. litecoinz-project referenced this in commit 399a0b3064 on Mar 15, 2018
  85. zkbot referenced this in commit 564119eb31 on Apr 3, 2018
  86. zkbot referenced this in commit 77669b952b on Apr 3, 2018
  87. random-zebra referenced this in commit 73d26f20e9 on May 27, 2020
  88. DrahtBot locked this on Sep 8, 2021


jamesob sipa gmaxwell luke-jr jgarzik pstratem dcousens laanwj dexX7 KyrosKrane fanquake jonasschnelli MarcoFalke paveljanik CodeShark

Labels
UTXO Db and Indexes

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-24 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me