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

Today I got famous "Recent blocks [-nnnnn]" in client interface. Two times in row.
So I start lurking sources where pushBlock() and popBlock() called.

Lets assume doPost() with "processBlock" executes simutaneously with thread that load blocks from some peer (line 4463 and below).
That two code branches is not syncronized on blocks completely.
Loading thread can do a lot of changes in local blockchain, not only appending new blocks, but reorganizing of chain tail in case of choosing different fork.
doPost() at the same time can process block from request almost completely. It calls pushBlock() that:
- create block
- check it validity (!)
- check block.previousBlock == lastBlock (!!)
- sync on blocks, waiting for loading thread complete (!!!)
- reset lastBlock to it's block, completely ignore what loading thread done (!!!!)

Seems like next call of loading thread can't fix that broken blockchain and f*ck up in loop at line 4642
which cause "Recent blocks [-nnnnn]" I mention in beginning of this post.

Am I miss something?

Edit: ricot lurking for the same in the beginning

So since my local 0.5.0 clients started crashing (i.e. showing negative recent blocks numbers, etc.) I started analyzing a little...
I started by getting the blocks that it currently knows from the api...
The block ids from the top of the list were:
16120160395225814370
17250788183583576225
11124825242748196662
14790117364446866715
8810693157391937532

So according to the block explorer, my top 4 blocks are from a forked chain. Nothing tragic, happens...
Let's look at the cumulative difficulty:
my client says 877033680606371.
Let's check some other node in the network, take node10.nxtbase.com, and we get: 877619990555689

So why doesn't my client catch up?
Let's have a look with wireshark, what's going on...
The first thing that we see is... ehm... 192.161.48.32, aka node00.nxtbase.org.
This node is spamming me with all kinds of slightly weird stuff, e.g. duplicate acks.
Ok, whatever, let's look at our stuff...
My client seems to only do Post requests to one single address... 192.161.48.32 WTF?!?
The address answers fast with a nice cumulative difficulty of 866067307974274.
Wait a sec... that's quite low? What's going on there? Well, later...
First: Why the hell am I only asking that weird address???
Let's see whom I'm connected to. getPeers from my account and for each of them getPeer and look for connected:1, because only from them I will query new blocks.
There are 10 of them, one is node00.nxtbase.org, the other 9 look a bit more legit... but wait!
All of those 9 have a negative weight... doesn't that? Yes, yes, it does... If a node's weight is negative, it isn't considered for blockchain scanning. Ok, but then, why isn't it connecting to other nodes?
Well, it only connects to other nodes if it isn't connected to 10 nodes already... And why does it keep the connection to nodes with a negative weight? I don't know... Really, why don't you cancel connections to nodes that you'll never use???
Anyways... How did the weight become negative?
Let's start by looking at peer.weight... This is set in analyze hallmark, and only there... and before it is set, it's checked whether it is smaller than 0... WTF?!? Ah! getWeight() is not a getter for weight... well, obviously, why would I expect any senseful naming from that code! Thanks for taking my time looking into the wrong direction, again. (shift+alt+r getBalanceAdjustedWeight) [for those, that don't know eclipse: i just renamed all occurrences of getWeight with getBalanceAdjustedWeight, what it really is, so I won't be confused by that in the future... Why CfB and others didn't do that? IDK.]
But on that topic, I also saw an updateWeight... hmm, is that going to update the weight or the adjustedWeight? Well, neither, it doesn't take a parameter and it sends a processNewData request to all Users... Guys, seriously?!? MVC - Model, View, Controller, not Mix Various Crap :p
Back to our attacker, or the weight, or sth...
How can it be negative?
Well, it calculates the following: adjustedWeight * (account.getBalanceCent() / 100) / 1000000000 [I added Cent to the functions that return amount in 0.01 NXT, because some did, some didn't]
adjustedWeight is calculated whenever a new hallmark is analyzed and it is basically just the fraction of this peer's weight within all peer's weight, nothing that could be negative.
But the balance might be... at least I know one account off the top of my head that has a negative balance, the genesis account.
So let's take a node with a negative balance and look at the account.
Randomness has chosen: node81.nxtbase.com with a current weight of -25046. So which account does it have?
Well, the account ID are the first 32 bytes of the hallmark, well , actually the last 32, but let's not go into that topic...
Anyway, the accountID is 11243542237777034551, so it's not the genesis account and it should have a balance of around 315k according to the block explorer. So what does my client say?
Well, it's not that easy. Unfortunately we can't query an account's balance with any API call... bummer.
So let's look at the transactions instead... All of them have a lot of confirmations, so just accumulate them, subtract the fees for sent transactions... aaaand... 315402... looks legit, definitely not negative.
So what happent to Account.balance?
Let's look at where that is set. We've got a total of 6 occurrences...
When we analyze a block:
- the generator account is credited with the fee. (Let's hope noone gets credited more than once)
- for every transaction, the sender account gets the transaction amount and fee deducted (the sender account is expected to already exist, let's hope that's always the case and doesn't crash with a NullPointer because then the recipients wouldn't get their balance updated)
- for every transaction, the recipient is credited with the transaction amount
When we popLastBlock:
Same procedure, just all the + are now -. Say something about duplicate code, but hey...

Ok, so far so good, let's go deeper
Let's change the order of things and first look at where popLastblock is called... because that gets only called once.
It's called when we're getting a block chain, in case the last common block of us and the peer is not either of our last blocks, i.e. there has been a fork. Should happen regularly, right?
So what do we do in that case?
We write blocks and transactions to a backup file, then we pop our blocks until we find the common ancestor, then we push the peers blocks, and then we check if the cumulative difficulty really has increased, as promised earlier by the peer.
So what happens, if the peer lied about the cumulative difficulty and just sent us a few (say 2) blocks, branched from very far (say 700) blocks back?
We'll pop 700 blocks, push 2 blocks, realize that we got bulls^&t, blacklist the peer and load our backups. And then everything is fine, isn't it? ISN'T IT?
No, it's not.
While popping the 700 blocks, we adjusted the balance of all accounts to the state 700 blocks ago. Then when pushing the 2 blocks, we took whatever transactions the "intruder" wanted us to know and included into his blocks, and adjust our account's balances accordingly.
And when we realize, everything was crap? Do we reset the accounts to what they were before? No, we don't. We keep the crap balances and hope for the best.

So if you want to take down a client, that is *any client*, just analyze the hall marks in the network, wait for some nice transactions, and generate a few blocks where you may or may not include the transactions to set the balance to a negative value which then causes the attacked client to not talk to that part of the network anymore and the *validation of new blocks to also be corrupted*, which enables you to send some nicely crafted blocks that only the attacked clients will accept and strengthen your position as the only peer it wants to communicate with.

So is that the injected flaw to take down a forked currency within a day? If yes, why does it work in 0.5.0? ^^

(Btw: Only posting this publicly because it is already widely used)


[edit]
Totally forgot: Kudos to whoever attacked using that way, nice find.

[edit2]
@immortAlex: That's what's causing the negative numbers...

No. It's just 2⁶⁴.

0.4.7e source code was modified before publishing.

hmpf....so I misunderstood the phrase "3 flaws were injected into 0.4.7e".

No, I can't.

yes, of course.

edit:

Yes, logical flaws. But if I understand CfB correct, he states that the original 0.4.7e (the version which was distributed to the nxt users) and subsequent versions do NOT contain those logical flaws. Can you confirm that, CfB?

Logical flaws.

It does contain these flaws. Logical flaws, not programming bugs.

Lol, are you really sure that the published code on bitbucket contains 3 flaws?  
I have done some local blackbox-testing with 2 machines and everything works fine.

Furthermore I have checked the classes User, Peer, Transaction, Block, Account and the method init, doGet and everything looks very clean there (beside the slightly odd use of oop that was discussed here enough). Some more classes to check..let's go.

Transactions from orphaned blocks r sorted by height of the block they were included into. This line tells that non-orphaned transactions should be included into blocks AFTER orphaned ones.

What is the purpose of:
in Transaction, line 2981 ?

thx.

Let me see if I'm reading this right: This assumption is based on Alice and Bob generating the same block within a 15s time-frame, correct? And you're saying it would be more beneficial for either Alice or Bob to send the block the latest as possible to include as many transactions as possible into their block? As someone mentioned earlier, say Alice is sending it right before the 15s window closes, many peers (on average, 50%?) would reject this block because their time is slightly behind hers, as it looks like 16s to them. Bob sends it right on time (around 0s, depending on how off his clock is) and will capture 90+% of the network with his block. So then we end up in this situation: Alice has a few peers with her "longer" block, and Bob has a majority of peers with his "shorter" block. Would Alice still win even if Bob has 90% of the network on his side? Sorry if I'm asking something obvious, I'm not too well versed in how the network handles this stuff

How would this affect TF? (Or how will TF affect this scenario?) With instant transactions, the user/client/merchant will send their transactions directly to the trusted forger, knowing that they will generate the next block. Let's say Alice is guaranteed to generate the block first, and decides to wait 15s before sending. Bob knows that Alice will beat him, but the client still submits his block (?) because Alice waited until after his block is submitted before broadcasting her block to the network. If the answer to my first paragraph is that Alice will win (because she has the longest block), then wouldn't the network start routing transactions directly to the next guaranteed forger, say Charlie, right at Alice's block deadline? (ie. it doesn't care when the block is actually received) Alice could be malicious and decide to never send the block (meaning she would literally be handing her money to Bob who would put out the block anyways), but then she wouldn't be considered a "trusted forger" and have all sorts of negative financial consequences outlined in the TF forum post.

What's the state of play?  Guess it's Bazaar rather than Cathedral, but (other than ricot's last two issues) does anyone have a list of outstanding bugs and weaknesses found?   

If it was done and flawless, there would be no opportunity.

ricot: great work, keep tearing it apart