Check block integrity when necessary #1764

Gilthoniel · 2019-03-14T16:44:19Z

The proof verification was not checking that the last forward link was pointing on the actual latest block provided by the proof which that a malicious peer can in some extend modify the block. This check has been added in the Java and the Go code.

Another issue was that only the forward links of a request skipblock were checked so any other fields could be modified as we were not checking the received skipblock hash. This has been fixed in the Java library.

Fixes #1703

Gilthoniel · 2019-03-15T08:59:35Z

I think this covers the different request of blocks we do in the Java library. @kc1212 Please let me know if I forgot something.

I'm investigating the skipchain service on the Go side..

kc1212 · 2019-03-15T09:05:12Z

The java side looks good

This fixes the Java and Go verification of a proof to add a check for the last forward link to point on the actual hash of the latest block to be sure the latest block has not been tempered. Fixes #1703

This adds a check to the skipchain rpc when asking for a skipblock so that the hash must match the block identifier Fixes #1703

This adds block integrity checks among the skipchain service when blocks are requested from a peer which means it could be malicious data. Fixes #1703

This adds further checkings in byzcoin and skipchain services so that a request to a distant node is verified before usage.

The value returned must be assumed untrusted because the service is lacking the context to verify it belongs to the chain. Verifications must be done upstream.

This adds a verification step to make sure the returned genesis block has been created with the same proposed parameters

Gilthoniel · 2019-03-20T10:56:24Z

So the conclusion of this PR is that mainly the byzcoin and skipchain API were assuming data coming from the service is safe and then I added integrity checks where it was possible to verify. Sometimes you lack the context and then the verification must be done upstream, like when you ask for a single block (maybe we should always return a proof alongside?).

Aside of that the main goal of this PR was to fix the proof verification to insure the last forward link points to the correct returned latest block.

kc1212 · 2019-03-20T13:55:53Z

byzcoin/api.go

 func (c *Client) StreamTransactions(handler func(StreamingResponse, error)) error {
 	req := StreamingRequest{
 		ID: c.ID,
 	}
+	// TODO: what if the leader is down ?


the error handler handler(StreamingResponse{}, err) should be called and the function returns

kc1212 · 2019-03-20T13:56:56Z

byzcoin/api.go

+	// for testing purpose
+	if testCorruptNewLedgerResponse != nil {
+		reply = testCorruptNewLedgerResponse
+	}


I think it's easier to just test verifyGenesisBlock by itself. then you don't need to insert test code into production code.

another possibility: can we register a corrupted byzcoin service that always just returns a wrong genesis block (e.g., embed byzcoin.Service inside byzcoin.BadService and overwrite the create genesis block function), it doesn't even have to do consensus or anything like that, just returns the bad block? just a few ideas to think about

Yes for the verifyGenesisBlock but I really want to make sure the request is checking this, and that no one removed this check accidentally.

another posibility...
I tried that first, but the service name is a constant (which is fine) so you can't change which service is going to be used, or am I missing something ? I would love to do it that way..

Nevermind, I found a way to go around without too many complex mocking

kc1212 · 2019-03-20T13:57:45Z

byzcoin/api.go

+			// send the block only if the integrity is correct
+			handler(resp, nil)
+		} else {
+			log.Warnf("got a corrupted block from %v", c.Roster.List[0])


probably you should call the handler here too, to say there is an error, like handler(StreamingResponse{}, err)

kc1212 · 2019-03-20T14:00:05Z

byzcoin/api.go

@@ -417,3 +450,54 @@ func DefaultGenesisMsg(v Version, r *onet.Roster, rules []string, ids ...darc.Id
 	}
 	return &m, nil
 }
+
+func verifyGenesisBlock(sb *skipchain.SkipBlock, req *CreateGenesisBlock) error {


nit: I would name those parameters like: actual *skipchain.SkipBlock, expected *CreateGenesisBlock, so it's obvious which input is trusted.

kc1212 · 2019-03-20T14:03:24Z

byzcoin/api.go

+	}
+
+	if len(data.TxResults) != 1 || len(data.TxResults[0].ClientTransaction.Instructions) != 1 {
+		return nil, errors.New("didn't get only one instruction")


Suggested change

return nil, errors.New("didn't get only one instruction")

return nil, errors.New("genesis darc tx should only have one instruction")

kc1212 · 2019-03-20T14:12:28Z

external/java/src/main/java/ch/epfl/dedis/skipchain/SkipchainRPC.java

+                // we don't want a malicious peer to send a block with tempered
+                // data like a different roster with different public keys
+                throw new CothorityCommunicationException("invalid block integrity");
+            }


the function comment says the block is not verified, is that still the case?

I'll change the comment. The integrity is verified but the caller must verify if it belongs to the chain.

kc1212 · 2019-03-20T14:18:30Z

skipchain/api.go

+
+	if testCorruptSSBResponse != nil {
+		sb = testCorruptSSBResponse
+	}


see comment above regarding the possibility of creating a bad service

kc1212 · 2019-03-20T14:20:31Z

skipchain/api.go

 	return sb.Latest, nil
 }

+// CompareGenesisBlocks compares the content of two blocks and returns an error
+// if there is any difference
+func compareGenesisBlocks(prop *SkipBlock, ret *SkipBlock) error {


why can't you re-calculate the hash? if the hash is different then the block content should also be different right?

I wish I could but the service is adding information to the genesis like a random backlink

Gilthoniel self-assigned this Mar 14, 2019

Gilthoniel force-pushed the java_skipblock_hash_1703 branch from 10b0618 to 83475a0 Compare March 15, 2019 15:27

Gilthoniel mentioned this pull request Mar 19, 2019

Fix the roster ID generation dedis/onet#528

Merged

Gaylor Bosson added 6 commits March 20, 2019 10:18

Proof verify checks hash of the latest block

7bf0097

This fixes the Java and Go verification of a proof to add a check for the last forward link to point on the actual hash of the latest block to be sure the latest block has not been tempered. Fixes #1703

Check skipblock's hash coming from outside

410d995

This adds a check to the skipchain rpc when asking for a skipblock so that the hash must match the block identifier Fixes #1703

Add block hash verification

a99fb98

This adds block integrity checks among the skipchain service when blocks are requested from a peer which means it could be malicious data. Fixes #1703

Check integrity of replies

ce4ca90

This adds further checkings in byzcoin and skipchain services so that a request to a distant node is verified before usage.

Update onet to v3.0.2

361bc37

Remove verification for createNewBlock

dd4195b

The value returned must be assumed untrusted because the service is lacking the context to verify it belongs to the chain. Verifications must be done upstream.

Gilthoniel force-pushed the java_skipblock_hash_1703 branch from 8c8057a to dd4195b Compare March 20, 2019 09:19

Verify the integrity of the genesis block

d04dc26

This adds a verification step to make sure the returned genesis block has been created with the same proposed parameters

kc1212 reviewed Mar 20, 2019

View reviewed changes

Address Kelong's comments

e635f32

kc1212 approved these changes Mar 21, 2019

View reviewed changes

jeffallen merged commit c5b18f2 into master Mar 21, 2019

jeffallen deleted the java_skipblock_hash_1703 branch March 21, 2019 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check block integrity when necessary #1764

Check block integrity when necessary #1764

Gilthoniel commented Mar 14, 2019

Gilthoniel commented Mar 15, 2019

kc1212 commented Mar 15, 2019

Gilthoniel commented Mar 20, 2019

kc1212 Mar 20, 2019

kc1212 Mar 20, 2019

kc1212 Mar 20, 2019

Gilthoniel Mar 20, 2019 •

edited

Loading

Gilthoniel Mar 21, 2019

kc1212 Mar 20, 2019

kc1212 Mar 20, 2019

kc1212 Mar 20, 2019

kc1212 Mar 20, 2019

Gilthoniel Mar 21, 2019

kc1212 Mar 20, 2019

kc1212 Mar 20, 2019

Gilthoniel Mar 20, 2019

	return nil, errors.New("didn't get only one instruction")
	return nil, errors.New("genesis darc tx should only have one instruction")

Check block integrity when necessary #1764

Check block integrity when necessary #1764

Conversation

Gilthoniel commented Mar 14, 2019

Gilthoniel commented Mar 15, 2019

kc1212 commented Mar 15, 2019

Gilthoniel commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gilthoniel Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gilthoniel Mar 20, 2019 •

edited

Loading