Nxt source code flaw reports

[Come-from-Beyond]

...still trying to figure out why CfB couldn't answer that question about Genesis when the client starts...

Don't stick to one place, move to other and come back later with new ideas.

[1715314384]

1715314384

[1715314384]

1715314384

[Pouncer]

@CfB,
Appreciate what the team is doing. Since someone is managing the sizeable unclaimed genesis funds, can I suggest stakeholders who want to help but are unable to do so directly for lack of time, skills or other reasons be allowed to channel donations into this fund. There will be no extra work for the fund manager, just more available resources. If that's OK, I'll be the first to send the 1M Nxt pledged for s/w dev but currently sitting idle.

I sent you another donation ricot for your amazing work

+1

We need guys like him in our team. Hey, big stakeholders, do u hear me?

[vamdor]

smaragda, ricot, vamdor, ImmortAlex, rlh: Please PM your NXT tip account. Thanks for pouring through the code.

FrictionlessCoin send yours too cuz you need a NXT hug.

Post your addresses guys (gals?) I wanna reward your efforts...

ricot : 100K
ImmortAlex :50K
vamdor: 50K
rlh: 50K
smaragda: 50K

wow, thank you!

2083517044157347473

(ps.: atm I am just passively following the thread due to lack of time, but there are still a few issues that I feel haven't been thoroughly checked, so beware:) )

[Jaguar0625]

There's a potential for an easy DOS attack here because you're using a request provided value when making an allocation:

Code:

	InputStream inputStream = req.getInputStream();
	ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
	byte[] buffer = new byte[65536];
	int numberOfBytes;
	while ((numberOfBytes = inputStream.read(buffer)) > 0) {
		
		byteArrayOutputStream.write(buffer, 0, numberOfBytes);
		
	}
	inputStream.close();
	request = (JSONObject)JSONValue.parse(byteArrayOutputStream.toString("UTF-8"));
	
	...
	
	int payloadLength = ((Long)request.get("payloadLength")).intValue();
	byte[] payloadHash = convert((String)request.get("payloadHash"));
	byte[] generatorPublicKey = convert((String)request.get("generatorPublicKey"));
	byte[] generationSignature = convert((String)request.get("generationSignature"));
	byte[] blockSignature = convert((String)request.get("blockSignature"));

	Block block = new Block(version, blockTimestamp, previousBlock, numberOfTransactions, totalAmount, totalFee, payloadLength, payloadHash, generatorPublicKey, generationSignature, blockSignature);
			
	ByteBuffer buffer = ByteBuffer.allocate(BLOCK_HEADER_LENGTH + payloadLength); // <<<

So, if someone wants to slow you down they can stream in garbage blocks with large payloadLength parameters and force you to do large allocations, which would probably slow down the server to a crawl.

Not to mention, they could also pass in a negative number and cause allocate to fail.

[Jaguar0625]

For people late to the this thread, I just wanted to remind everyone of a parallel thread going on that also contains a good discussion about the code (https://bitcointalk.org/index.php?topic=397214.0). It's mostly focused on more minor flaws than this thread, but some good contributions have been made there.

[ricot]

smaragda, ricot, vamdor, ImmortAlex, rlh: Please PM your NXT tip account. Thanks for pouring through the code.

FrictionlessCoin send yours too cuz you need a NXT hug.

Post your addresses guys (gals?) I wanna reward your efforts...

ricot : 100K
ImmortAlex :50K
vamdor: 50K
rlh: 50K
smaragda: 50K

  Thanks a lot!
Address is in the sig

[msin]

For people late to the this thread, I just wanted to remind everyone of a parallel thread going on that also contains a good discussion about the code (https://bitcointalk.org/index.php?topic=397214.0). It's mostly focused on more minor flaws than this thread, but some good contributions have been made there.

Thanks Jaguar0625.  Any flaws for bounty found yet?

[msin]

smaragda, ricot, vamdor, ImmortAlex, rlh: Please PM your NXT tip account. Thanks for pouring through the code.

FrictionlessCoin send yours too cuz you need a NXT hug.

Post your addresses guys (gals?) I wanna reward your efforts...

ricot : 100K
ImmortAlex :50K
vamdor: 50K
rlh: 50K
smaragda: 50K

That's awesome Klee, thank you for rewarding these guys with what they deserve.

[msin]

@CfB,
Appreciate what the team is doing. Since someone is managing the sizeable unclaimed genesis funds, can I suggest stakeholders who want to help but are unable to do so directly for lack of time, skills or other reasons be allowed to channel donations into this fund. There will be no extra work for the fund manager, just more available resources. If that's OK, I'll be the first to send the 1M Nxt pledged for s/w dev but currently sitting idle.

I sent you another donation ricot for your amazing work

+1

We need guys like him in our team. Hey, big stakeholders, do u hear me?

I'm not a stakeholder, but I'll give 250k.  I would suggest hitting up bybitcoin, Noitev, and buybitcoinscanada.  They all seem to have an interest in helping the community.

[klee]

@CfB,
Appreciate what the team is doing. Since someone is managing the sizeable unclaimed genesis funds, can I suggest stakeholders who want to help but are unable to do so directly for lack of time, skills or other reasons be allowed to channel donations into this fund. There will be no extra work for the fund manager, just more available resources. If that's OK, I'll be the first to send the 1M Nxt pledged for s/w dev but currently sitting idle.

I sent you another donation ricot for your amazing work

+1

We need guys like him in our team. Hey, big stakeholders, do u hear me?

I'm not a stakeholder, but I'll give 250k.  I would suggest hitting up bybitcoin, Noitev, and buybitcoinscanada.  They all seem to have an interest in helping the community.

Problem is only them, me and neer.g (maybe one two more I forget) do so...

[ferment]

@CfB,
Appreciate what the team is doing. Since someone is managing the sizeable unclaimed genesis funds, can I suggest stakeholders who want to help but are unable to do so directly for lack of time, skills or other reasons be allowed to channel donations into this fund. There will be no extra work for the fund manager, just more available resources. If that's OK, I'll be the first to send the 1M Nxt pledged for s/w dev but currently sitting idle.

I sent you another donation ricot for your amazing work

+1

We need guys like him in our team. Hey, big stakeholders, do u hear me?

I'm not a stakeholder, but I'll give 250k.  I would suggest hitting up bybitcoin, Noitev, and buybitcoinscanada.  They all seem to have an interest in helping the community.

Problem is only them, me and neer.g (maybe one two more I forget) do so...

bybitcoin and buybitcoinscanada have supported the 100 node project.

[kunibopl]

might not be the right thread - you can ignore..
right now there's 1 MB needed to store 5000 transactions in transactions.nxt
how compares NXT blockchain size to BTC chainsize in the future? I guess it's similar?
when CfB talkes about possible 1000 ta/s in the future, how can any decentralized network store this?
distributed storage? HD storage and internet bandwith will not scale up infinitely.

[Jaguar0625]

If looks like you are blacklisting peers that send a block with a difficulty less than the current difficulty:

Code:

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

Two thoughts:
(1) Couldn't this lead to blacklisting good peers just due to network latency (thinking of the future when a lot of transactions are being made)?
(2) Couldn't a rouge peer send out blocks with really high difficulties to get other (good) peers blacklisted? It doesn't look like the difficulties are being validated anywhere.

[Jaguar0625]

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.

[Jaguar0625]

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.

Sorry, I made one mistake. NXT is using 64-bit values as ids instead of 32-bit values (so just replace 32 with 64 everywhere in my comment). The rest of my point still stands.

[ImmortAlex]

For people late to the this thread, I just wanted to remind everyone of a parallel thread going on that also contains a good discussion about the code (https://bitcointalk.org/index.php?topic=397214.0). It's mostly focused on more minor flaws than this thread, but some good contributions have been made there.

I try to focus that thread not on flaws/bugs, but on code quality. I failed a bit because Jean-Luc do a lot of refactorings already, or have some of them in TODO list. I'm still not finished to read code, so maybe there will be some posts more.

[ImmortAlex]

I would suggest hitting up bybitcoin, Noitev, and buybitcoinscanada.  They all seem to have an interest in helping the community.

They do good job selling their stake at the very beginning. At least I remember bybitcoin sell a lot of 1M for 0.6 BTC. I was so stupid, lazy and ignoramus that time...

[wakasaki808]

I would suggest hitting up bybitcoin, Noitev, and buybitcoinscanada.  They all seem to have an interest in helping the community.

They do good job selling their stake at the very beginning. At least I remember bybitcoin sell a lot of 1M for 0.6 BTC. I was so stupid, lazy and ignoramus that time...

same lol.

I just didn't have the BTC at the time(just started crypto's).  Would have bought 1M if I had the BTC...

Was seeing the prices go up and up while waiting for my conversion of $ to BTC from coinbase....

[ImmortAlex]

might not be the right thread - you can ignore..
right now there's 1 MB needed to store 5000 transactions in transactions.nxt
how compares NXT blockchain size to BTC chainsize in the future? I guess it's similar?
when CfB talkes about possible 1000 ta/s in the future, how can any decentralized network store this?
distributed storage? HD storage and internet bandwith will not scale up infinitely.

Blockchain shrinking was promised by BCNext.
Blockchain structure is much simplier in Nxt, than Bitcoin, so it can be programmed with much less effort, I think.

[ImmortAlex]

initializeKeyPair returns an account id that is used to unlock an account.

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

It was discussed since the beginning
If you do at least one transaction (even alias), account public key will be revealed to blockchain, so it protect you account from stealing. The only problem remains is account ids collision. C-f-B promised that when such collisions appears in real life, devs just increase number of used bytes from hash.
So it's not a bug, but a feature.