Pages:
Author

Topic: Nxt source code flaw reports - page 23. (Read 113369 times)

legendary
Activity: 2142
Merit: 1010
Newbie
January 12, 2014, 03:06:56 AM
Code:
if (Block.getLastBlock().cumulativeDifficulty.compareTo(curCumulativeDifficulty) < 0) {

    Block.loadBlocks("blocks.nxt.bak");
    Transaction.loadTransactions("transactions.nxt.bak");
    peer.blacklist();

}

Why not <= instead of

If an adversary wants to upload a lot of traffic, so be it. For other peers inbound traffic is much cheaper.
newbie
Activity: 26
Merit: 0
January 11, 2014, 09:31:28 PM
Code:
if (Block.getLastBlock().cumulativeDifficulty.compareTo(curCumulativeDifficulty) < 0) {

    Block.loadBlocks("blocks.nxt.bak");
    Transaction.loadTransactions("transactions.nxt.bak");
    peer.blacklist();

}

Why not <= instead of
cumulativeDifficulty is not determined by someone's mind, but based on the time gap between neighbor blocks. while the generation time will be checked in verifyGenerationSignature() in which the attacker must satisfy hit
legendary
Activity: 866
Merit: 1002
January 11, 2014, 08:14:32 PM
This is only possible if he has 51% of the money, so his cumulated difficulty would rise faster than the one of the network with 49%.
And it should be fixed by transparent forging.

I don't think I understand your reasoning, can you elaborate?
newbie
Activity: 56
Merit: 0
January 11, 2014, 07:51:18 PM
This is only possible if he has 51% of the money, so his cumulated difficulty would rise faster than the one of the network with 49%.
And it should be fixed by transparent forging.
legendary
Activity: 866
Merit: 1002
January 11, 2014, 07:37:00 PM

In this case, let's say B is malicious and wants to replace A(1, 2, 2) with a different block. In this case, B(1, 2, 10) does not chain to the last block in A so it is put in as a future block. The two possible chains for A are:
A.1: { (0, 1, 1), (1, 2, 2), (2, 3, 3) }
A.2: { (0, 1, 1), (1, 2, 10) }

Since A.2 has a higher difficulty, it will be chosen. As far as I can tell, I don't see anything preventing a malicious attacker from manipulating cumulative difficulty scores in order to get blocks dropped.

Maybe I haven't slept enough, but I think you're right, but...

This would imply two things:
  • He would have to be generator of (1,2,10) - ok he can predict that
  • but he would also have to be generator of (1,2,2) (coz verify generation signature would't pass)

so basically he would invalidate his own block...


So, isn't this something that can be taken advantage of by a large account holder? The largest account holder right now has 70M NXT, which means that he should be able to forge 7% of blocks. At this rate, it seems very possible that he can generate two blocks within the block height of 720 that clients look at when syncing. (Not to mention that this could also be exploited if there was collusion by among some of the top account holders).

But keeping in mind what I've written above... I'm not sure, what would they gain by this?
The fact that this (1,2,10) would replace (1,2,2) IMHO implies that they would invalidate all transactions that followed (1,2,2)...

Does it imply large account holder could rewrite history? (That's bad he could buy something material, and than "roll it back"...)

EDIT I think we both are missing something, as this would be bad...
legendary
Activity: 1512
Merit: 1124
Invest in your knowledge
January 11, 2014, 07:16:35 PM
Besides small errors or bugs, has anyone found a critical* flaw in this code?
sr. member
Activity: 299
Merit: 250
January 11, 2014, 07:07:32 PM

In this case, let's say B is malicious and wants to replace A(1, 2, 2) with a different block. In this case, B(1, 2, 10) does not chain to the last block in A so it is put in as a future block. The two possible chains for A are:
A.1: { (0, 1, 1), (1, 2, 2), (2, 3, 3) }
A.2: { (0, 1, 1), (1, 2, 10) }

Since A.2 has a higher difficulty, it will be chosen. As far as I can tell, I don't see anything preventing a malicious attacker from manipulating cumulative difficulty scores in order to get blocks dropped.

Maybe I haven't slept enough, but I think you're right, but...

This would imply two things:
  • He would have to be generator of (1,2,10) - ok he can predict that
  • but he would also have to be generator of (1,2,2) (coz verify generation signature would't pass)

so basically he would invalidate his own block...

This would also work if B was manipulated to have both new chaining blocks and future blocks, e.g.
B: { (0, 1, 1), (1, 2, 2), (2, 3, 3), (3, 4, 4), (1, 2, 10) }

In effect, this would keep transactions (B(3, 4, 4) in the example completely hidden from A.

Or am I missing something?

I think this is good example, that those situations (adding to end of chain AND invalidating tail of chain) are (or rather should, as they are not now)
mutually exclusive cases...


So, isn't this something that can be taken advantage of by a large account holder? The largest account holder right now has 70M NXT, which means that he should be able to forge 7% of blocks. At this rate, it seems very possible that he can generate two blocks within the block height of 720 that clients look at when syncing. (Not to mention that this could also be exploited if there was collusion by among some of the top account holders).
newbie
Activity: 50
Merit: 0
January 11, 2014, 06:15:52 PM
Code:
if (Block.getLastBlock().cumulativeDifficulty.compareTo(curCumulativeDifficulty) < 0) {

    Block.loadBlocks("blocks.nxt.bak");
    Transaction.loadTransactions("transactions.nxt.bak");
    peer.blacklist();

}

Why not <= instead of
legendary
Activity: 2142
Merit: 1010
Newbie
January 11, 2014, 03:57:23 PM
CfB,

line 4274:
         lastBlock = GENESIS_BLOCK_ID;
         long curBlockId = GENESIS_BLOCK_ID;
         do {
            
            Block curBlock = tmpBlocks.get(curBlockId);
            long nextBlockId = curBlock.nextBlock;
            curBlock.analyze();
            curBlockId = nextBlockId;
            
         } while (curBlockId != 0);

lastBlock is set to GENESIS_BLOCK_ID, but it is not updated inside the loop with "lastBlock = curBlock;" right after curBlock.analyze(); This means the test on line 4581 fails if we are on any block other than the one right after the genesis block.

line 4581:
if (block.previousBlock == lastBlock) {...}

Is this one of the three flaws?

James


No
legendary
Activity: 1176
Merit: 1134
January 11, 2014, 03:33:26 PM
CfB,

line 4274:
         lastBlock = GENESIS_BLOCK_ID;
         long curBlockId = GENESIS_BLOCK_ID;
         do {
            
            Block curBlock = tmpBlocks.get(curBlockId);
            long nextBlockId = curBlock.nextBlock;
            curBlock.analyze();
            curBlockId = nextBlockId;
            
         } while (curBlockId != 0);

lastBlock is set to GENESIS_BLOCK_ID, but it is not updated inside the loop with "lastBlock = curBlock;" right after curBlock.analyze(); This means the test on line 4581 fails if we are on any block other than the one right after the genesis block.

line 4581:
if (block.previousBlock == lastBlock) {...}

Is this one of the three flaws?

James
legendary
Activity: 866
Merit: 1002
January 11, 2014, 01:00:23 PM
I am on it but I think it will a lot slower than you want Sad

We'll choose the fastest and will pay part of the bounty then.

If the fastest will be chosen, there's no point in publishing solution, before 20th...

That is, if I'd publish it earlier, someone could tune-up my solution and claim he has faster one...
newbie
Activity: 37
Merit: 0
January 11, 2014, 07:21:53 AM
If file blocks.nxt doesn't exist and transactions.nxt is loaded successfully then the client tries to create the genesis block with all transactions loaded from transactions.nxt. As a result the client throws IllegalArgumentException:
java.lang.IllegalArgumentException: attempted to create a block with 78229 transactions

The client is not functional after this; you get NullPointerException if you try to open web interface. The workaround is to delete transactions.nxt too.
newbie
Activity: 50
Merit: 0
January 11, 2014, 05:48:40 AM
initializeKeyPair returns an account id that is used to unlock an account:

Code:
case "unlockAccount":
{

String secretPhrase = req.getParameter("secretPhrase");
BigInteger accountId = user.initializeKeyPair(secretPhrase);
...

However, the account number is comprised of only the first 8 bytes of the hash of the account's public key:

Code:
BigInteger initializeKeyPair(String secretPhrase) throws Exception {

this.secretPhrase = secretPhrase;
byte[] publicKeyHash = MessageDigest.getInstance("SHA-256").digest(Crypto.getPublicKey(secretPhrase));
BigInteger bigInteger = new BigInteger(1, new byte[] {publicKeyHash[7], publicKeyHash[6], publicKeyHash[5], publicKeyHash[4], publicKeyHash[3], publicKeyHash[2], publicKeyHash[1], publicKeyHash[0]});

return bigInteger;
}

The SHA-256 hash is secure because it creates a 256-bit number and a negligible (albeit non-zero) hash collision probability. In practice, hash collisions can usually be ignored (although in this case since it is dealing with currency, the implications of a hash collision are especially concerning since people would be able to use other's money or block them from using their money.

However, by reducing the identifier from 256-bit to 32-bit the possibility for hash collisions is exponentially greater. Also, there's no guarantee that a hash algorithm (i.e. SHA-256) guarantees that subsets of its produced hashes are also hashes. What this means is that there's no guarantee that the first 32-bits of SHA-256 hashes are even as good as 32-bit hashes.

Even BitCoin addresses are much more secure in that they are 160-bit (real) hashes (http://bitcoin.stackexchange.com/questions/7724/what-happens-if-your-bitcoin-client-generates-an-address-identical-to-another-pe).

I think it's critical that we make NXT at least as secure as Bitcoin.

All 256 bits r checked after the very 1st outgoing transaction is made.

Where?
legendary
Activity: 2142
Merit: 1010
Newbie
January 11, 2014, 05:30:26 AM
This could lead to problems when the peer that received the transactions is the forger of the next block. What if there are double spending transactions and the forger does not get the second transaction.

The only issue is that next forger won't get max fee. It doesn't matter which transaction is chosen among double-spending ones.
newbie
Activity: 50
Merit: 0
January 11, 2014, 05:13:36 AM
Line 6707:

Code:
counter++;
if (counter >= 255) {

    break;

}

Why is there a counter, so that only 255 transactions get returned? I know that this is the maximum amount of transactions in a block, but there is no reason why I could get more unconfirmed transactions by another peer, isn't it?

This is to limit volume of data sent between nodes.

To limit data volume? I see a problem there: Because of this counter other peers might not get all new transactions. This could lead to problems when the peer that received the transactions is the forger of the next block. What if there are double spending transactions and the forger does not get the second transaction.

Beside that getUnconfirmedTransactionIds does not have this counter.

Sorry but "limit volume of data" seems to be a bad argument. I think this is a bigger problem when the list of unconfirmed transactions gets distributed from peer to peer incomplete.
legendary
Activity: 2142
Merit: 1010
Newbie
January 11, 2014, 03:06:46 AM
Just marking the thread to follow.
Are all 3 flaws still not found?  Is there a certain time limit after which these will get fixed even if not found by anyone else?

The critical flaw was almost found but the guy who wrote about it just asked a question, not said that this might be a flaw.

The other flaws r not found yet. In April they'll be revealed.
legendary
Activity: 896
Merit: 1006
First 100% Liquid Stablecoin Backed by Gold
January 11, 2014, 02:55:17 AM
Just marking the thread to follow.
Are all 3 flaws still not found?  Is there a certain time limit after which these will get fixed even if not found by anyone else?
legendary
Activity: 2142
Merit: 1010
Newbie
January 11, 2014, 02:42:59 AM
But the public node who received this huge transaction, will busy to broadcasting it, can't do anything else.
The attacker can cause more damage by using less data. Depends on the peers connected to this public node.

Public nodes won't broadcast such transactions.
Pages:
Jump to:
© 2020, Bitcointalksearch.org