[WIP] zPIV Encryption #413

rejectedpromise · 2017-11-26T05:06:16Z

Currently, zPIV mints are stored, unencrypted in wallet. Functionality has been rerouted through CWallet -> CCryptoKeyStore to enable access to the master wallet passphrase when wallet unlocked. zPIV mints are then encrypted the same way that wallets private keys are before saving to wallet.

Currently zPIV is stored as plaintext. Created new class CZerocoinCrypter which implements the AES256CBC routine same as CCrypter, but operated over a CBigNum inplace. This allows for CZerocoinMint to initialize and set key data for CZerocoinCrypter on construction. The cipher is derived from a pub/priv keypair that the wallet owns.

rejectedpromise · 2017-11-26T06:22:53Z

Currently hitting Segfault on -reindex. Currently working toward a solution

rejectedpromise

currently segfaulting and putting testnet DB into errorstate

rejectedpromise · 2017-11-26T06:52:26Z

progress update: I forgot to check for InitialChainSync ;)

Will reopen if full sync and no error minting/spending

presstab

Havent run it yet, looks like some cool code. Hoping to run it soon.

presstab · 2017-11-26T06:27:31Z

src/primitives/zerocoin.cpp

+    }
+
+    //inialize IV for 32 bytes
+    vector<unsigned char> chIV(crypter.KEY_SIZE);


You should follow our current naming conventions. A vector will start with v and camel case after. A vector of unsigned chars should start with vch. Descriptive names are also a good idea.

This function appears to use no member variables of CZeromint at all (plus it uses a global pointer) so I wonder why it is not a free function or part of the CWallet class. Personally I would have added this, Encrypt and Decrypt as separate functions so there would be separate plain CZeromint objects and encrypted objects. I don't see the benefit of encrypting an object if the un-encrypted member variables are still accessible. However, with separate objects you can dispose of the un-encrypted ones in favor of the encrypted (once they've been encrypted) or vice-versa. This may just be a lack of understanding of the bigger picture here but it just doesn't seem like a good separation of concerns. Hope this is clear. So essentially a separate layer or separate encrypted objects that is all done outside of this class

presstab · 2017-11-26T06:31:44Z

src/primitives/zerocoin.cpp

+        if(!crypter.Decrypt(serialNumber) ||
+           !crypter.Decrypt(randomness)) {
+
+            LogPrintf("%s : failed to encrypt zerocoin mint\n", __func__);


typo in log print

presstab · 2017-11-26T06:36:37Z

src/rpcwallet.cpp

    for (CZerocoinMint mint : vMintsToUpdate) {
+
+        if(pwalletMain->IsCrypted() && !mint.Encrypt()){
+            LogPrintf("Error: encryption of mint failed");


It would be beneficial to add context to these logs so that if there is a failure to mint we know where exactly in the code it is happening. Also you need to terminate the string with \n.

presstab · 2017-11-26T06:38:57Z

src/rpcwallet.cpp

        denomination = libzerocoin::IntToZerocoinDenomination(params[1].get_int());
    list<CZerocoinMint> listMints = walletdb.ListMintedCoins(!fIncludeSpent, false, false);

+    for(CZerocoinMint mint : listMints) {


Seems like bad form to have two identical for loops right next to each other. Any particular reason not to decrypt the mint within the loop a few lines below?

The loop below was const objects. Wasn't sure if there is reasoning around when to treat the objects as constants and when to treat them as mutable. Is there any particular reason in the loop below it is marked const?

It's marked const since there is no need to change the object itself. Thus code is clearer as to what is going on. Now you are decrypting, so you could either remove the 'const' restriction or have Decrypt make new object. BTW, shouldn't the error message be "Decryption of mint failed?"

presstab · 2017-11-26T06:43:53Z

src/walletdb.cpp

        CZerocoinMint mint;
        ssValue >> mint;

+        if(pwalletMain->IsCrypted() && !mint.Decrypt())


Why do we want to decrypt the coins everytime we get the list of mints?

does pwalletMain->IsCrypted() returning false not terminate the statement?

presstab · 2017-11-26T06:45:54Z

src/zerocoincrypter.cpp

@@ -0,0 +1,79 @@
+//
+// Created by rejectedpromise on 11/25/17.


Replace this with the PIVX Devlopers copyright heading.

presstab · 2017-11-26T06:50:08Z

src/zerocoincrypter.h

@@ -0,0 +1,61 @@
+//
+// Created by rejectedpromise on 11/25/17.


rejectedpromise · 2017-11-26T21:19:45Z

Reopen. Still may have problems with DB on -reindex causing runaway exception. BE WARNED.

rejectedpromise · 2017-11-26T21:21:46Z

May also be problems in logic for syncing after encryptions have happened

Fuzzbawls · 2017-11-28T11:00:33Z

Just off the top of my head, I think this should bump the wallet version no?
Does this "play nice" with existing wallets or does it only apply to NEW wallets? (ie, are existing wallets "converted" to a full encryption state or does the encryption of the zPIV space only apply to newly encrypted wallets?)

Move cryption functionality into crypter, and crypting same time as is currently implemented in wallet. Discarding plain mints and returning ciphered copies. Crashing on open of wallet.

rejectedpromise · 2017-12-09T00:44:39Z

.gitignore

 CMakeLists.txt
 cmake-build-debug
+db4/
+db-4*


PIVX-Project · 2017-12-18T19:32:04Z

Where are we on this??

failure on GetCursor() from listzerocoinmints called on unlock

rejectedpromise · 2017-12-18T20:19:14Z

Current status of ezPIV:

CCrypter methods for EncryptZerocoinMint and DecryptZerocoinMint
wallet->EncryptWallet encrypts zerocoinmints same way as it encrypts keys
wallet->Unlock decrypts zerocoinmints same way as it encrypts keys

Still debugging issues with reading and writing to disk
Seems to be having trouble with walletdb->ListZerocoinMints(true, false, false)
rooting from the GetCursor() call

… to vMasterKey for encryption Log statements in new methods for debugging no guarantees on no segfaults during zerocoin minting on this commit

presstab · 2017-12-28T22:26:34Z

What is the current status of this. Still waiting for this so that I can finish deterministic zpiv.

…rocoin_encryption_tests.cpp Haven't tested live on testnet yet, but think it should be working properly through wallet::CCryptoKeyStore Crypto actions should happen automatically, as with the wallet priv keys, and while unlocked (walletpassphrase kept in memory) decrypts crypted mints that are stored in a private map in CCryptoKeyStore Hoping to get some good testnet runs tomorrow

rejectedpromise · 2017-12-31T07:01:38Z

src/crypter.cpp

        return false;
-
+//    vector<unsigned char> x(vchPlaintext.begin(), vchPlaintext.end());
+//    CBigNum t(x);


rejectedpromise · 2017-12-31T07:01:49Z

src/crypter.cpp


    vchCiphertext.resize(nCLen + nFLen);
+
+    //CBigNum y(vchCiphertext);


rejectedpromise · 2017-12-31T07:02:17Z

src/crypter.cpp

+    vector<unsigned char> key(vMasterKey.begin(), vMasterKey.end());
+    vector<unsigned char> plain(vchPlaintext.begin(), vchPlaintext.end());
+
+


remove spaces

rejectedpromise · 2017-12-31T07:02:28Z

src/crypter.cpp

+//    if (!fKeySet)
+//        return false;
+//
+//    //already encrypted


rejectedpromise · 2017-12-31T07:02:50Z

src/crypter.cpp


        fUseCrypto = true;
        BOOST_FOREACH (KeyMap::value_type& mKey, mapKeys) {
+


remove space

rejectedpromise · 2017-12-31T07:03:23Z

src/crypter.h


 public:
+    enum CryptionMethod {
+        ENC,


rejectedpromise · 2017-12-31T07:03:41Z

src/crypter.h

+enum ZerocoinSecrets {
+    SERIAL,
+    RANDOM
+};


rejectedpromise · 2017-12-31T07:05:04Z

src/rpcwallet.cpp

        mint.SetHeight(nHeight);
+
+//        if(pwalletMain->IsCrypted() && !mint.Encrypt()){
+//            LogPrintf("Error: encryption of mint failed");


rejectedpromise · 2017-12-31T07:08:15Z

@presstab
see recent commit 25df0827647f92d98317e45e15cbab80640d6ec6

hoping to get it up on testnet tomorrow to do some live runs

rejectedpromise · 2018-01-08T04:19:54Z

Status update:
GUI has been tested fairly well in minting and spending across a couple different wallets on testnet.
While testing RPC calls (specifically exportzerocoins) I have run into a couple of bugs that I am hoping to resolve soon before sending to QA.

@Fuzzbawls I do not think this would require a version bump, as it should be backwards compatible, as it hooks up to the current encryption scheme and uses the walletpassphrase same as the private keys, therefore not requiring bump AFAIK

…let for encryption

Fuzzbawls

A few changes needed from what I can see at a glance, otherwise great progress!

Fuzzbawls · 2018-01-19T01:32:23Z

src/rpcwallet.cpp

 #include <boost/assign/list_of.hpp>

-#include <univalue.h>
+//#include <univalue.h>


hmm, think your IDE was giving you some unique errors if was needed.

#include <univalue.h> should be enough.

Fuzzbawls · 2018-01-19T01:33:07Z

src/test/zerocoin_encryption_tests.cpp

+#include "../sync.h"
+#include "../main.h"
+#include "../crypter.h"
+#include "../wallet.h"


not digging the relative includes here

Ahhh, nice catch. I have just recently configured CMakeLists.txt in CLion to work off of absolute paths. Forgot to actually crawl through and change all the relatives 👍

Fuzzbawls · 2018-01-19T01:35:27Z

src/walletdb.cpp

    ss << zerocoinMint.GetValue();
    uint256 hash = Hash(ss.begin(), ss.end());

+    LogPrintf("pub: %s\nserial: %s\nrand: %s\ncrypted: %b\n", zerocoinMint.GetValue().ToString(16), zerocoinMint


suggest making this a LogPrint("zero", ...); instead of a LogPrintf() to reduce log spam.

rejectedpromise · 2018-01-21T00:21:02Z

Status update

Failing on block indexing since trying to read serials from walletDB, checking for spent/owned/etc., while they are still encrypted (main.h so not yet unlocked).

Initial approach

Keep a list of serial hashes in pwallet to enable checking if serial is owned by this wallet, without having to expose private data.

rejectedpromise · 2018-04-22T03:35:27Z

pulled into 3.1.0

rejectedpromise requested a review from presstab November 26, 2017 05:06

ghost assigned rejectedpromise Nov 26, 2017

ghost added the review label Nov 26, 2017

rejectedpromise mentioned this pull request Nov 26, 2017

Encryption of zPIV #372

Closed

rejectedpromise added enhancement High Priority zPiv Wallet Security labels Nov 26, 2017

rejectedpromise force-pushed the zPIVEncryption branch 2 times, most recently from 1476995 to 71b7c70 Compare November 26, 2017 05:34

rejectedpromise force-pushed the zPIVEncryption branch from 71b7c70 to 021fb50 Compare November 26, 2017 05:35

rejectedpromise commented Nov 26, 2017

View reviewed changes

rejectedpromise closed this Nov 26, 2017

ghost removed the review label Nov 26, 2017

presstab reviewed Nov 26, 2017

View reviewed changes

rejectedpromise changed the title ~~[Security] zPIV Encryption~~ [WIP] zPIV Encryption Nov 26, 2017

rejectedpromise reopened this Nov 26, 2017

ghost added the review label Nov 26, 2017

Fuzzbawls mentioned this pull request Nov 28, 2017

[WIP] Deterministic zPIV Generation #416

Closed

Fuzzbawls added this to the 3.1.0 milestone Nov 30, 2017

rejectedpromise added 2 commits December 8, 2017 17:06

Merge branch 'upstream-master' into zPIVEncryption

3274427

[WIP-UNSTABLE] Working on separation of concerns.

22d7657

Move cryption functionality into crypter, and crypting same time as is currently implemented in wallet. Discarding plain mints and returning ciphered copies. Crashing on open of wallet.

rejectedpromise commented Dec 9, 2017

View reviewed changes

.gitignore

CMakeLists.txt

cmake-build-debug

db4/

db-4*

Copy link

Author

rejectedpromise Dec 9, 2017

Choose a reason for hiding this comment

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

revert

consolidation of logic into Unlock/Encrypt methods

82ef205

Debugging

e351407

failure on GetCursor() from listzerocoinmints called on unlock

[WIP] CZerocoinMints routed through CCryptoKeyStore to recieve access…

8e26086

… to vMasterKey for encryption Log statements in new methods for debugging no guarantees on no segfaults during zerocoin minting on this commit

rejectedpromise force-pushed the zPIVEncryption branch from 59697a1 to 25df082 Compare December 31, 2017 07:00