Nxt source code flaw reports - page 14.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: liyi3c on January 20, 2014, 11:44:19 AM

the second 'for' loop assgin adjustedWeight to peer whose peer.date != validDate.
if one account attached to huge amount hallmarks, and send out them. as more and more its hallmarks out of date(peer.date != validDate), we should set their adjustedWeight = 0; but here it's not this way. the sum(peer.weight)/totalWeight will not equal to 1.
someone can increase its chosen possibility by send as much as hallmarks

I don't see a bug there...

liyi3c

newbie

Activity: 26

Merit: 0

Code:

for (Peer peer : groupedPeers) {
							
							if (peer.date == validDate) {
								
								totalWeight += peer.weight;
								
							} else {
								
								peer.adjustedWeight = 0;
								peer.updateWeight_UserBroadCast();
								
							}
							
						}
						
						for (Peer peer : groupedPeers) {
							
							peer.adjustedWeight = 1000000000L * peer.weight / totalWeight;
							peer.updateWeight_UserBroadCast();
							
						}

the second 'for' loop assgin adjustedWeight to peer whose peer.date != validDate.
if one account attached to huge amount hallmarks, and send out them. as more and more its hallmarks out of date(peer.date != validDate), we should set their adjustedWeight = 0; but here it's not this way. the sum(peer.weight)/totalWeight will not equal to 1.
someone can increase its chosen possibility by send as much as hallmarks

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: nervermind on January 20, 2014, 08:57:50 AM

Does security flaw count?

Only if injected ones.

nervermind

newbie

Activity: 16

Merit: 0

Does security flaw count?

This is a good example of Often Misused: Authentication (see https://www.owasp.org/index.php/Often_Misused:_Authentication)
5995: if (allowedUserHosts == null && !InetAddress.getByName(req.getRemoteAddr()).isLoopbackAddress()) {
6030: if (allowedUserHosts == null && !InetAddress.getByName(req.getRemoteAddr()).isLoopbackAddress()) {
6065: if (allowedUserHosts == null && !InetAddress.getByName(req.getRemoteAddr()).isLoopbackAddress()) {

This is a bad idea to rely on remoteAddr (which can spoofed) when you do authorization checks.
I didn't do any risk assessment of removePeer, removeActivePeer and removeBlacklistedPeer so have no idea how critical are these functions. But it looks like the idea was to restrict this functionality only to requests coming from localhost. And the way it is done... Bit insecure I'd say.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: FrictionlessCoin on January 20, 2014, 08:15:33 AM

Time to defect to the winning team folks!

1st we must find the 3rd flaw... How will we split 100k btw?

FrictionlessCoin

legendary

Activity: 868

Merit: 1000

Cryptotalk.org - Get paid for every post!

Quote from: Come-from-Beyond on January 20, 2014, 08:12:45 AM

Quote from: FrictionlessCoin on January 20, 2014, 08:09:06 AM

Seeking Additional Java Developers

We have successfully decompiled the Nxt source and are now in the process of performing a massive refactoring and cleanup of the code base.

Short term goals.

Remove the primitive addressing scheme and use a scheme that looks like Bitcoin. Bitcoin addresses are more readable, have a checksum and have many tools to generate them. Nxt addresses that are all numerical are something that came out of the 1950's.

Secure Wallet. Nxt wallet is fundamentally insecure. Exposing a wallet to the public internet for everyone to hack is just idiotic. No wonder so many Nxt users have lost money!

Nxt does not use an internal database like every other alt coin. This is idiotic because NXT nodes easily fail and run out of memory. Furthermore, if you accidentally turn off your node... your wallet can get corrupted!

NEX strives not only to be a fairer distributed Nxt variant, but a technologically super version.

Join the enterprise... participate with your coding skills!!!

We will not ship SHIT like the Nxt folks. We will ship product that will secure our users coins!

May I join?

Time to defect to the winning team folks!

Schollepetzer

newbie

Activity: 17

Merit: 0

Hey boyz,

could someone give a short summary plz. about the 2 flaws already been found?
Not in the mood right now the read through 55p...

Thank you

nervermind

newbie

Activity: 16

Merit: 0

OK. Here we go:

1209:   ObjectOutputStream objectOutputStream = new ObjectOutputStream(fileOutputStream);
same story here. If Exception is thrown - stream stays unreleased.

2877:    OutputStream outputStream = connection.getOutputStream();
there is a catch block at 2919, maybe close it there?

2883:          InputStream inputStream = connection.getInputStream();
same as above

3299:    static void loadTransactions(String fileName) throws Exception {
FileInputStream fileInputStream = new FileInputStream(fileName);
         ObjectInputStream objectInputStream = new ObjectInputStream(fileInputStream);

and

3437: saveTransactions

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: nervermind on January 19, 2014, 03:40:20 PM

805:   static void loadBlocks(String fileName) throws Exception {

         FileInputStream fileInputStream = new FileInputStream(fileName);
         ObjectInputStream objectInputStream = new ObjectInputStream(fileInputStream);
         blockCounter = objectInputStream.readInt();
         blocks = (HashMap)objectInputStream.readObject();
         lastBlock = objectInputStream.readLong();
         objectInputStream.close();
         fileInputStream.close();

      }
Streams are about to close at the end of the method. But if there is an Exception thrown (any of .read* can do that) - they are not.
Unreleased resources in servlets => DOS.

I've found few more such things if you're interested.

Yes, plz.

nervermind

newbie

Activity: 16

Merit: 0

805:   static void loadBlocks(String fileName) throws Exception {

         FileInputStream fileInputStream = new FileInputStream(fileName);
         ObjectInputStream objectInputStream = new ObjectInputStream(fileInputStream);
         blockCounter = objectInputStream.readInt();
         blocks = (HashMap)objectInputStream.readObject();
         lastBlock = objectInputStream.readLong();
         objectInputStream.close();
         fileInputStream.close();

      }
Streams are about to close at the end of the method. But if there is an Exception thrown (any of .read* can do that) - they are not.
Unreleased resources in servlets => DOS.

I've found few more such things if you're interested.

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: nervermind on January 19, 2014, 03:21:50 PM

6098: String recipientValue = req.getParameter("recipient"), amountValue = req.getParameter("amount"), feeValue = req.getParameter("fee"), deadlineValue = req.getParameter("deadline");

6109: deadline = (short)(Double.parseDouble(deadlineValue) * 60);

deadlineValue comes from HttpServletRequest and passed to parseDouble which is vulnerable to DOS prior to JRE 6.23

see http://www.oracle.com/technetwork/topics/security/alert-cve-2010-4476-305811.html

Interesting catch! Fortunatelly, NRS doesn't work on Java 6.

nervermind

newbie

Activity: 16

Merit: 0

6098: String recipientValue = req.getParameter("recipient"), amountValue = req.getParameter("amount"), feeValue = req.getParameter("fee"), deadlineValue = req.getParameter("deadline");

6109: deadline = (short)(Double.parseDouble(deadlineValue) * 60);

deadlineValue comes from HttpServletRequest and passed to parseDouble which is vulnerable to DOS prior to JRE 6.23

see http://www.oracle.com/technetwork/topics/security/alert-cve-2010-4476-305811.html

bitcoinpaul

hero member

Activity: 910

Merit: 1000

This silence is calming Wink

BitByteBite

member

Activity: 101

Merit: 10

Quote from: msin on January 19, 2014, 01:39:08 AM

CFB, would be cool if you could update the OP, just so people who come here know that the first two flaws have been found and the fatal flaw has yet to be found.

This is true, but now you have to find the posts that found the flaw to find the answer. A relatively small rabbit hole if you ask me Wink

msin

legendary

Activity: 1470

Merit: 1004

CFB, would be cool if you could update the OP, just so people who come here know that the first two flaws have been found and the fatal flaw has yet to be found.

BitByteBite

member

Activity: 101

Merit: 10

Quote from: Come-from-Beyond on January 17, 2014, 03:50:44 PM

Quote from: tyz on January 17, 2014, 03:40:50 PM

Interesting callenge Grin

Ill take a closer look to the source code later. Has someone found a flaw yet or are they sill undiscoverd?

2 flaws were found. The fatal flaw with 100K reward is still waiting for u.

Sounds interesting Wink

bitcoinpaul

hero member

Activity: 910

Merit: 1000

TF to the rescue!!!

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: ?? on ??

Is there a minimum amount of transactions/recipient accounts required to generate the Genesis Block in order to establish a secure 100% POS currency? Could all 1 Billion NXT have gone to one recipient account; thus having one transaction in the Genesis Block? How would things progress from there in regard to acceptance of chains, signing, etc. in such case? Would it just progress the same way as it did with the existing Genesis Block?

thnx

If u give 51% of all coins to one entity and don't have TF enabled then this entity can do 51% attack at any moment.

ricot

newbie

Activity: 56

Merit: 0

exactly...

Btw: I still have quite a bit of code ahead of me. Wink

Come-from-Beyond

legendary

Activity: 2142

Merit: 1009

Newbie

Quote from: rlh on January 17, 2014, 05:03:42 PM

Quote from: ricot on January 17, 2014, 04:42:39 PM

Yes, the key should only be saved if it's in a block...

Yes, my assumption would be that the one that "wins" is the first transaction with the matched key that is received by the node that forges the block. So, you send out 30 transactions, each with different keys, but which ever node forges the inclusive block should only select the first TX that it received, and ignore all of the others.

Make sense?

Yes, just don't forget to reset the key if the block becomes orphaned.

Topic: Nxt source code flaw reports - page 14. (Read 113312 times)