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

[Zerocoin][UNIT TEST][RPC] Wrapped serials. #837

Merged
merged 25 commits into from Mar 22, 2019

Conversation

@furszy
Copy link
Collaborator

furszy commented Mar 15, 2019

This PR fixes the wrapped serials vulnerability.

There were two main distinguished issues which enabled this complex attack (requiring a deep understanding of the inner workings of the zerocoin protocol). They are described below.

  1. The serial number S was represented on the system with the BigNumclass [/src/libzerocoin/Coin.h#L139] which allows to store (unlike uint256) values with size bigger than 256 bits. The transaction signature check was performed only over the last 256 bits of the BigNum object [/src/libzerocoin/CoinSpend.cpp#L140] using the getuint256() method mentioned before, and not over the whole object (which should have only 256 bits anyway).

  2. The Signature of Knowledge verification, as it is done after the coin spend signature verification, was not checking the bitsize of the serial number [/src/libzerocoin/SerialNumberSignatureOfKnowledge.cpp#L125] (assuming it's already checked a priori in the coin spend signature verification) and thus allowed for serials with more than 256 bits.

Fur further information, article detailing the construction and results of the exploited vulnerability:
https://medium.com/@dev.pivx/report-wrapped-serials-attack-5f4bf7b51701

furszy and others added 10 commits Mar 7, 2019
use only upper bound and only mainnet
@giaki3003

This comment has been minimized.

Copy link

giaki3003 commented Mar 15, 2019

Great! Another example of why you should always keep track of different var size. Go PIVX!

@random-zebra random-zebra self-assigned this Mar 15, 2019
@APinkPanther

This comment has been minimized.

Copy link

APinkPanther commented Mar 15, 2019

Hello gentlemen,

Your assessment of this CV is flattering. I am pleased to see that you captured, diagnosed, and unravelled my bug. I was hoping the eventual discovery of this bug would earn the title "Serial Overflow" for the double entendre.

I have three reasons for posting.

(1) There was a claim of piv/zpiv inflation. This is not technically correct. The double-spend does drain the zpiv privacy pool but it does not create new tokens. It would however cause outbound zpiv transactions to fail if too many people spend their zpiv and bump into the zpiv floor which is maintained by a sanity check of the zpiv supply. Example of your code which maintains this floor:

// zerocoin failsafe
if (pindex->mapZerocoinSupply.at(denom) < 0)
   return state.DoS(100, error("Block contains zerocoins that spend more than are in the available supply to spend"));

To prevent this condition you may need to add the fake serial zpiv balances back into the supply counters. Read: You would need to deliberately cause the inflation on your end to prevent the zpiv supply from potentially bottoming.

(2) The grand total in your final assessment is a long leap from what I actually acquired. I could have hastened the pace re - #828 and chose not to. It would appear that you may have tipped your hand to someone who rushed to yank downright sinful amounts before the zpiv pool was frozen. The signature of the attack appears to have changed around this time as well. My serials always start with hex 0xf for the precise purpose of looking at least somewhat authentic despite the attention of #828. Further, I never minted a 5000 denomination token. So the 90+ mints you see of that denomination are someone else... possibly someone who became privy to knowledge of the bug.

(3) I chose exploitation instead of disclosure because exploitation pays well, reliably, and anonymously. Disclosure does not. HackerOne's ToS: https://www.hackerone.com/terms/finder

Reads as follows:

You may remain anonymous by using a pseudonym. To be eligible to receive a Bounty, however, you must provide HackerOne with accurate, complete and up-to-date information about you, including your mailing address, social security number (if applicable) and any other information that HackerOne reasonably requests, to allow HackerOne to legally send any Bounty to you and file the appropriate tax form following year end. If you do not provide this information to HackerOne, any Bounty that would otherwise be paid to you will be paid to a charity of HackerOne's choosing.

Any cryptosphere bounty program which requires disclosure to government should not to be taken seriously.

Despite the perceived malice of this exploit I'm hoping your project is successful moving forward.

I will leave you with the signed message corresponding to the address of the first invalid transaction mentioned in your article: https://explorer.pivx.link/tx/caf7ee443214b4fb3162cd753b1faa35fce7c42c7520b825246d6968d0ddff34

Sincerely,
APinkPanther

verifymessage "DUAfNnwjJmTRbmqpvKhPsNLKi47Kd64Ean" "ILuOmSKomqRFlVhw5/YHs15D4UV/xZFiW7x+xiu2vCZKNx6xFiUovb8exzjGwp8rO0Z0qJ1mfYsSFcpp7Zrs6rk=" "APinkPanther"

@furszy furszy force-pushed the furszy:wrapped_serials branch 2 times, most recently from 534eafa to ad3a3c7 Mar 15, 2019
@random-zebra

This comment has been minimized.

Copy link
Collaborator

random-zebra commented Mar 16, 2019

Hello @APinkPanther

Thanks for your post and -I guess- for your attack too as it allowed us to catch this dangerous bug. I will address your points below.

  1. This is probably just semantics. If we look at the total supply you are right. Since each spend reduces the zPIV pool while increasing the PIV pool, the net result is zero.
    So no (apparent) inflation.
    But, in a wider sense, since the zPIV spent with fake serials "generated" some PIV while the corresponding amount of PIV was not "burnt" in a previous mint transaction, we do have inflation.
    It's just not reflected in the mapZerocoinSupply as it grows with zerocoin mints and decreases with zerocoin spends.
    Having more than one spend for a number of coins means that the inflated supply needs to be hard-coded into the map (and it will be addressed soon).
    That's not the same as "deliberately cause the inflation on your end to prevent the zpiv supply from potentially bottoming" even though it will indeed prevent the condition outlined.
    The cause of the inflation is the act of "duplicating" a coin by creating a new serial for it. It's not like other valid serials have been spent.

  2. That is quite interesting information.
    Issue 828 must have attracted more eyes on this and someone got carried away.
    Thank you for not taking sinful amounts as well then. It is somewhat honorable, given your early discovery (which is proven by the signed message).

  3. Understandable. Not much can be done regarding the size of the bounty (due to market conditions) unfortunately, but for sure I agree on the need for anonymity.
    I was not aware about those ToS.
    A crypto bug bounty program requiring the disclosure of all that information is pretty useless.

Even though it is unfortunate that you chose exploitation, must confess that it was a quite challenging problem to discover and analyze.
If you'd like to reach out, we would be pleased to discuss further details of the attack.
dev.pivx [at] protonmail [dot] com

@APinkPanther

This comment has been minimized.

Copy link

APinkPanther commented Mar 17, 2019

@random-zebra

It would be my pleasure. Email sent.

Copy link

presstab left a comment

Should also be checking for failure in the zerocoin challenge verification, in which tprime is a serialized and sent from peers.

tprime[i] = challengeCalculation(coinSerialNumber, s_notprime[i], SeedTo1024(sprime[i].getuint256()));

@furszy furszy dismissed presstab’s stale review Mar 18, 2019

This is covered on commit b3660cb..

furszy and others added 3 commits Mar 18, 2019
@random-zebra random-zebra force-pushed the furszy:wrapped_serials branch from 99a6972 to bd6b26b Mar 18, 2019
Copy link
Collaborator

Warrows left a comment

Looks good, ACK.

Copy link
Collaborator

Fuzzbawls left a comment

quick change to the nFakeSerialBlockheightEnd variable value for testnet/regtest

src/chainparams.cpp Outdated Show resolved Hide resolved
src/chainparams.cpp Outdated Show resolved Hide resolved
and add visual status feedback to UI interface
and fix percent of reindex feedback in uiInterface
@random-zebra random-zebra force-pushed the furszy:wrapped_serials branch from 2478a54 to da43659 Mar 20, 2019
@furszy

This comment has been minimized.

Copy link
Collaborator Author

furszy commented Mar 21, 2019

@zero24x run the wrapped serials scan script that it's on the article, take the result values and follow the PR code, if you are a developer it will be easy to understand where to put them.

Copy link
Collaborator

Fuzzbawls left a comment

couple log output formatting fixups and a concern about needing an extra client restart

src/init.cpp Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
furszy and others added 2 commits Mar 22, 2019
@random-zebra random-zebra force-pushed the furszy:wrapped_serials branch from 44c670b to 6eff5fa Mar 22, 2019
Copy link
Collaborator

Fuzzbawls left a comment

ACK 6eff5fa

@Fuzzbawls Fuzzbawls merged commit 6eff5fa into PIVX-Project:master Mar 22, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Fuzzbawls added a commit that referenced this pull request Mar 22, 2019
6eff5fa fix zPIV supply recalculation (random-zebra)
2676ca3 newline character added to recalculate wrapped serials supply logging (furszy)
da43659 Initialize nSupplyBeforeFakeSerial to 0 in TestNet and RegTest (random-zebra)
c46de31 Fix Wrapped Serials inflated zPIV supply recalculation (random-zebra)
1369186 zerocoin contextual spend check log invalid serials (furszy)
0240f27 log rejection serial block height (furszy)
ab6a134 invalid fake serial rejection (furszy)
010d035 zpiv recalculation moved in init and added in connectblock (furszy)
bd6b26b fix tabs in SoK (random-zebra)
41a2bcc fix bitSize typo in bignum for openssl (random-zebra)
29c1791 inflation methods moved to cpp (furszy)
027c9ff wrapped serial inflation trigger recalculation (furszy)
a3d725a Zerocoin supply, wrapped serials inflation + some minor modifications (furszy)
b3660cb SoK invalid range check. (furszy)
ad3a3c7 remove tab spaces (furszy)
ed15e21 Fix isBlockBetweenFakeSerialAttackRange check (random-zebra)
8e7cf5a Fix IsValidCommitmentToCoinRange check (random-zebra)
a936e03 getserials ambiguous Pair constructor in bitsize blocking clang compilation fix (furszy)
bd529a0 [UNIT TEST] wrapped serial coinSpend check (furszy)
1217868 [RPC] Add getserials method (random-zebra)
9fe8dab fix isValidSerial always true check (random-zebra)
913e48c wrapped range fix (furszy)
46a5231 wrapped serials check in acceptToMemPool method (furszy)
edfdb4c prints in console commented (furszy)
18e55ed fake serials attack enforcement (furszy)

Tree-SHA512: fe6eaf26257c25377982d44ff1cdd839edf6f71daa5607fc98397b4a3fcd26ae78db54c781038e4f9ff8a7ad1d460ce109909184852bb620435e10dcba6a638c
@ghost ghost removed the review label Mar 22, 2019
@Carbon-Zero

This comment has been minimized.

Copy link

Carbon-Zero commented on 18e55ed Mar 27, 2019

You guys blow me away. How you respond to something like this so quickly and in such detail is mind-boggling.

mbroemme added a commit to Galilel-Project/galilel that referenced this pull request Mar 28, 2019
- PIVX-Project/PIVX#812: [Regtest][Tests][RPC] Regtest mode + Test suite.
- PIVX-Project/PIVX#822: [Tests] Integrate fake stake tests into parent test suite
- PIVX-Project/PIVX#837: [Zerocoin][UNIT TEST][RPC] Wrapped serials.
- PIVX-Project/PIVX#838: [RPC][Test] spendrawzerocoin + wrapped serials functional test
mbroemme added a commit to Galilel-Project/galilel that referenced this pull request Jan 6, 2020
- PIVX-Project/PIVX#812: [Regtest][Tests][RPC] Regtest mode + Test suite.
- PIVX-Project/PIVX#822: [Tests] Integrate fake stake tests into parent test suite
- PIVX-Project/PIVX#837: [Zerocoin][UNIT TEST][RPC] Wrapped serials.
- PIVX-Project/PIVX#838: [RPC][Test] spendrawzerocoin + wrapped serials functional test
mbroemme added a commit to Galilel-Project/galilel that referenced this pull request Jan 6, 2020
- PIVX-Project/PIVX#812: [Regtest][Tests][RPC] Regtest mode + Test suite.
- PIVX-Project/PIVX#822: [Tests] Integrate fake stake tests into parent test suite
- PIVX-Project/PIVX#837: [Zerocoin][UNIT TEST][RPC] Wrapped serials.
- PIVX-Project/PIVX#838: [RPC][Test] spendrawzerocoin + wrapped serials functional test
mbroemme added a commit to Galilel-Project/galilel that referenced this pull request Jan 6, 2020
- PIVX-Project/PIVX#812: [Regtest][Tests][RPC] Regtest mode + Test suite.
- PIVX-Project/PIVX#822: [Tests] Integrate fake stake tests into parent test suite
- PIVX-Project/PIVX#837: [Zerocoin][UNIT TEST][RPC] Wrapped serials.
- PIVX-Project/PIVX#838: [RPC][Test] spendrawzerocoin + wrapped serials functional test
mbroemme added a commit to Galilel-Project/galilel that referenced this pull request Jan 6, 2020
- PIVX-Project/PIVX#812: [Regtest][Tests][RPC] Regtest mode + Test suite.
- PIVX-Project/PIVX#822: [Tests] Integrate fake stake tests into parent test suite
- PIVX-Project/PIVX#837: [Zerocoin][UNIT TEST][RPC] Wrapped serials.
- PIVX-Project/PIVX#838: [RPC][Test] spendrawzerocoin + wrapped serials functional test
mbroemme added a commit to Galilel-Project/galilel that referenced this pull request Jan 6, 2020
- PIVX-Project/PIVX#812: [Regtest][Tests][RPC] Regtest mode + Test suite.
- PIVX-Project/PIVX#822: [Tests] Integrate fake stake tests into parent test suite
- PIVX-Project/PIVX#837: [Zerocoin][UNIT TEST][RPC] Wrapped serials.
- PIVX-Project/PIVX#838: [RPC][Test] spendrawzerocoin + wrapped serials functional test
mbroemme added a commit to Galilel-Project/galilel that referenced this pull request Jan 6, 2020
- PIVX-Project/PIVX#812: [Regtest][Tests][RPC] Regtest mode + Test suite.
- PIVX-Project/PIVX#822: [Tests] Integrate fake stake tests into parent test suite
- PIVX-Project/PIVX#837: [Zerocoin][UNIT TEST][RPC] Wrapped serials.
- PIVX-Project/PIVX#838: [RPC][Test] spendrawzerocoin + wrapped serials functional test
mbroemme added a commit to Galilel-Project/galilel that referenced this pull request Jan 6, 2020
- PIVX-Project/PIVX#812: [Regtest][Tests][RPC] Regtest mode + Test suite.
- PIVX-Project/PIVX#822: [Tests] Integrate fake stake tests into parent test suite
- PIVX-Project/PIVX#837: [Zerocoin][UNIT TEST][RPC] Wrapped serials.
- PIVX-Project/PIVX#838: [RPC][Test] spendrawzerocoin + wrapped serials functional test
mbroemme added a commit to Galilel-Project/galilel that referenced this pull request Jan 6, 2020
- PIVX-Project/PIVX#812: [Regtest][Tests][RPC] Regtest mode + Test suite.
- PIVX-Project/PIVX#822: [Tests] Integrate fake stake tests into parent test suite
- PIVX-Project/PIVX#837: [Zerocoin][UNIT TEST][RPC] Wrapped serials.
- PIVX-Project/PIVX#838: [RPC][Test] spendrawzerocoin + wrapped serials functional test
mbroemme added a commit to Galilel-Project/galilel that referenced this pull request Jan 6, 2020
- PIVX-Project/PIVX#812: [Regtest][Tests][RPC] Regtest mode + Test suite.
- PIVX-Project/PIVX#822: [Tests] Integrate fake stake tests into parent test suite
- PIVX-Project/PIVX#837: [Zerocoin][UNIT TEST][RPC] Wrapped serials.
- PIVX-Project/PIVX#838: [RPC][Test] spendrawzerocoin + wrapped serials functional test
mbroemme added a commit to Galilel-Project/galilel that referenced this pull request Jan 6, 2020
- PIVX-Project/PIVX#812: [Regtest][Tests][RPC] Regtest mode + Test suite.
- PIVX-Project/PIVX#822: [Tests] Integrate fake stake tests into parent test suite
- PIVX-Project/PIVX#837: [Zerocoin][UNIT TEST][RPC] Wrapped serials.
- PIVX-Project/PIVX#838: [RPC][Test] spendrawzerocoin + wrapped serials functional test
mbroemme added a commit to Galilel-Project/galilel that referenced this pull request Jan 6, 2020
- PIVX-Project/PIVX#812: [Regtest][Tests][RPC] Regtest mode + Test suite.
- PIVX-Project/PIVX#822: [Tests] Integrate fake stake tests into parent test suite
- PIVX-Project/PIVX#837: [Zerocoin][UNIT TEST][RPC] Wrapped serials.
- PIVX-Project/PIVX#838: [RPC][Test] spendrawzerocoin + wrapped serials functional test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.