Nxt source code flaw reports

[Come-from-Beyond]

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.

[1714243511]

1714243511

[1714243511]

1714243511

[1714243511]

1714243511

[1714243511]

1714243511

[1714243511]

1714243511

[User705]

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?

[Come-from-Beyond]

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.

[gsan1]

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.

[Come-from-Beyond]

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.

[gsan1]

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?

[Come-from-Beyond]

Where?

https://bitbucket.org/JeanLucPicard/nxt-public/src/4073c21098076d3469b3f74d49e73ffabe3a2001/Nxt.java?at=master#cl-3537

[gsan1]

Where?

https://bitbucket.org/JeanLucPicard/nxt-public/src/4073c21098076d3469b3f74d49e73ffabe3a2001/Nxt.java?at=master#cl-3537

Thank you 

[nxtru]

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.

[gimre]

I am on it but I think it will a lot slower than you want

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

[jl777]

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

[Come-from-Beyond]

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

[gsan1]

Code:

if (Block.getLastBlock().cumulativeDifficulty.compareTo(curCumulativeDifficulty) < 0) {
														
    Block.loadBlocks("blocks.nxt.bak");
    Transaction.loadTransactions("transactions.nxt.bak");
    peer.blacklist();
														
}

Why not <= instead of <? A peer with malicious intent could tell everyone that his cumulativeDifficulty is higher. Now all peers would start this intense process of asking for his blocks, popping the own blocks, pushing the others just to detect that the difficulty is at least the same. And the peer will not be blacklisted so he could do this the whole time generating traffic. Why doesn't he get penalized for sending a wrong cumulativeDifficulty and doing this procedure to every peer in the network?

[Jaguar0625]

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

[tk808]

Besides small errors or bugs, has anyone found a critical* flaw in this code?

[gimre]

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

[ricot]

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.

[gimre]

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?

[liyi3c]

Code:

if (Block.getLastBlock().cumulativeDifficulty.compareTo(curCumulativeDifficulty) < 0) {
														
    Block.loadBlocks("blocks.nxt.bak");
    Transaction.loadTransactions("transactions.nxt.bak");
    peer.blacklist();
														
}

Why not <= instead of <? A peer with malicious intent could tell everyone that his cumulativeDifficulty is higher. Now all peers would start this intense process of asking for his blocks, popping the own blocks, pushing the others just to detect that the difficulty is at least the same. And the peer will not be blacklisted so he could do this the whole time generating traffic. Why doesn't he get penalized for sending a wrong cumulativeDifficulty and doing this procedure to every peer in the network?

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<target. attacker also need have better cumulativeDifficulty than all others. So only if attacker is the the real generator with best cumulativeDifficulty, otherwise he will be backlisted

[Come-from-Beyond]

Code:

if (Block.getLastBlock().cumulativeDifficulty.compareTo(curCumulativeDifficulty) < 0) {
														
    Block.loadBlocks("blocks.nxt.bak");
    Transaction.loadTransactions("transactions.nxt.bak");
    peer.blacklist();
														
}

Why not <= instead of <? A peer with malicious intent could tell everyone that his cumulativeDifficulty is higher. Now all peers would start this intense process of asking for his blocks, popping the own blocks, pushing the others just to detect that the difficulty is at least the same. And the peer will not be blacklisted so he could do this the whole time generating traffic. Why doesn't he get penalized for sending a wrong cumulativeDifficulty and doing this procedure to every peer in the network?

If an adversary wants to upload a lot of traffic, so be it. For other peers inbound traffic is much cheaper.