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.

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

Obfuscate chainstate #6650

Merged
merged 1 commit into from
Oct 6, 2015
Merged

Obfuscate chainstate #6650

merged 1 commit into from
Oct 6, 2015

Conversation

jamesob
Copy link
Contributor

@jamesob jamesob commented Sep 7, 2015

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

@@ -40,7 +40,7 @@ void static BatchWriteHashBestChain(CLevelDBBatch &batch, const uint256 &hash) {
batch.Write(DB_BEST_BLOCK, hash);
}

CCoinsViewDB::CCoinsViewDB(size_t nCacheSize, bool fMemory, bool fWipe) : db(GetDataDir() / "chainstate", nCacheSize, fMemory, fWipe) {
CCoinsViewDB::CCoinsViewDB(size_t nCacheSize, bool fMemory, bool fWipe) : db(GetDataDir() / "chainstate", nCacheSize, fMemory, fWipe, GetArg("-obfuscatechainstate", 0)) {
Copy link
Member

Choose a reason for hiding this comment

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

Use GetBoolArg, so it enables the -no prefix etc.

@sipa
Copy link
Member

sipa commented Sep 8, 2015

Concept ACK, this looks pretty good already.

@gmaxwell
Copy link
Contributor

gmaxwell commented Sep 8, 2015

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.

@sipa
Copy link
Member

sipa commented Sep 8, 2015

@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.

@jamesob
Copy link
Contributor Author

jamesob commented Sep 8, 2015

@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?

@luke-jr
Copy link
Member

luke-jr commented Sep 8, 2015

This is why supporting mixed obfuscated/unobfuscated is nice: no slow upgrade process. :)

@jamesob
Copy link
Contributor Author

jamesob commented Sep 8, 2015

@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.

@jgarzik
Copy link
Contributor

jgarzik commented Sep 8, 2015

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.

std::string CLevelDBWrapper::CreateObfuscateKey() const {
unsigned char buff[8];
GetRandBytes(buff, 8);
return std::string((const char *)buff);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be std::string(&buff[0], &buff[8]);

@jamesob
Copy link
Contributor Author

jamesob commented Sep 8, 2015

I've integrated @gmaxwell's suggestion and I think it's looking much better.

*/
bool IsEmpty()
{
leveldb::Iterator* it = NewIterator();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have to be allocated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there an alternative? apologies, I'm a C++ beginner :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, looks like this only comes allocated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clearing that up @jamesob

Copy link
Member

Choose a reason for hiding this comment

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

Please use a boost::scoped_ptr<leveldb::Iterator> here, like in txdb.cpp, to automatically free the iterator when it goes out of scope.

@jamesob
Copy link
Contributor Author

jamesob commented Sep 8, 2015

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.

@jamesob
Copy link
Contributor Author

jamesob commented Sep 8, 2015

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?

}

Write(OBFUSCATE_KEY_KEY, obfuscate_key);
LogPrintf("Wrote new obfuscate key '%s'\n", obfuscate_key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. obfuscate_key is not necessarily printable. Maybe it would be better to log a hexified version of the key?

@jamesob
Copy link
Contributor Author

jamesob commented Sep 9, 2015

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.

@jamesob jamesob closed this Sep 9, 2015
@jamesob jamesob reopened this Sep 9, 2015
template <typename K, typename V>
bool Read(const K& key, V& value) const throw(leveldb_error)
bool Read(const K& key, V& value, bool unobfuscate = true) const throw(leveldb_error)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@jamesob
Copy link
Contributor Author

jamesob commented Sep 10, 2015

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.

@KyrosKrane
Copy link

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?

@fanquake
Copy link
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

contrib/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.

@jonasschnelli
Copy link
Contributor

@KyrosKrane
Copy link

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.

@jamesob
Copy link
Contributor Author

jamesob commented Sep 12, 2015

@KyrosKrane that sounds great, thanks for your help.

@maflcko
Copy link
Member

maflcko commented Sep 12, 2015

@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.

@KyrosKrane
Copy link

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.

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.
@jamesob jamesob force-pushed the obfuscate_chainstate branch from 12a4813 to 42cb388 Compare October 6, 2015 14:50
@laanwj
Copy link
Member

laanwj commented Oct 6, 2015

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.

@laanwj laanwj merged commit 42cb388 into bitcoin:master Oct 6, 2015
laanwj added a commit that referenced this pull request Oct 6, 2015
42cb388 Add chainstate obfuscation to avoid spurious antivirus detection (James O'Beirne)
@jamesob jamesob deleted the obfuscate_chainstate branch October 6, 2015 16:50
@maflcko
Copy link
Member

maflcko commented Oct 7, 2015

@KyrosKrane [https://github.com//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.

@KyrosKrane
Copy link

@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.

@KyrosKrane
Copy link

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

@dexX7
Copy link
Contributor

dexX7 commented Oct 15, 2015

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.

laanwj pushed a commit to laanwj/bitcoin that referenced this pull request Oct 31, 2015
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.

Conflicts:
	src/Makefile.test.include

Github-Pull: bitcoin#6650
Rebased-From: 42cb388
kazcw added a commit to kazcw/bitcoin that referenced this pull request Apr 22, 2016
This was implemented some time ago in bitcoin#6650 to fix bitcoin#6613 but does not appear
ever to have been enabled under any circumstances outside of test/
zkbot added a commit to zcash/zcash that referenced this pull request Jan 15, 2018
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
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.
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request May 27, 2020
82f9088 Refactor: Remove using namespace <xxx> from /dbwrapper_tests (random-zebra)
6db0b37 rpc: make `gettxoutsettinfo` run lock-free (random-zebra)
f009cf4 Do not shadow members in dbwrapper (random-zebra)
b7e540c dbwrapper: Move `HandleError` to `dbwrapper_private` (random-zebra)
43004d0 leveldbwrapper file rename to dbwrapper.* (random-zebra)
c882dd9 leveldbwrapper symbol rename: Remove "Level" from class, etc. names (random-zebra)
f6496da leveldbwrapper: Remove unused .Prev(), .SeekToLast() methods (random-zebra)
cacf3c2 Fix chainstate serialized_size computation (random-zebra)
a2a3d33 Add tests for CLevelDBBatch, CLevelDBIterator (random-zebra)
94150ac Encapsulate CLevelDB iterators cleanly (random-zebra)
21df7cc [DB] Refactor leveldbwrapper (random-zebra)
2251db3 change hardcoded character constants to a set of descriptive named co… (random-zebra)

Pull request description:

  This backports a series of updates and cleanups to the LevelDB wrapper from:

  - bitcoin#5707
  - bitcoin#6650 [`*`]
  - bitcoin#6777 [`*`]
  - bitcoin#6865
  - bitcoin#6873
  - bitcoin#7927 [`*`]
  - bitcoin#8467
  - bitcoin#6290
  - bitcoin#9281

  PIVX-specific edits were required to keep the sporks and zerocoin databases in line.

  [`*`] NOTE: excluding the obfuscation of databases by xoring data, as we might not want this feature (e.g. as zcash/zcash#2598). Otherwise it can be discussed, and added, with a separate PR.

ACKs for top commit:
  furszy:
    Re ACK 82f9088 .
  Fuzzbawls:
    ACK 82f9088

Tree-SHA512: 1e4a75621d2ec2eb68e01523d15321d1d2176b81aac0525617852899ab38c9b4980daecb9056d054e7961fc758a22143edf914c40d1819144a394f2869a8ad57
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Obfuscate database files