Doesn't confirm_processed_parentchain_block need extra security check ?

[tdelabro]

#[weight = (<T as Config>::WeightInfo::confirm_processed_parentchain_block(), DispatchClass::Normal, Pays::Yes)]
pub fn confirm_processed_parentchain_block(origin, block_hash: H256, trusted_calls_merkle_root: H256) -> DispatchResult {
    let sender = ensure_signed(origin)?;
    Self::is_registered_enclave(&sender)?;
    log::debug!("Processed parentchain block confirmed for mrenclave {:?}, block hash {:?}", sender, block_hash);
    Self::deposit_event(RawEvent::ProcessedParentchainBlock(sender, block_hash, trusted_calls_merkle_root));
    Ok(())
}

This extrinsic is called from within the enclave. The call is created and signed in the trusted environment, so we can be sure of it's authenticity. But if we imagine a malicious or corrupted untrusted part of the worker, the very block received by enclave could have been tampered or straight up created from nowhere. So I'm surprise this function is not doing any check on the block_hash argument in order to verify that the block the enclave added to it's internal light client, is indeed an legit block that have emitted by the blockchain.

Am I missing something that make this kind of scenario impossible ? Or should we think of an extra security ? Like unregister enclaves that make a call to confirm_processed_parentchain_block with an illegitimate block_hash.

[haerdib]

The most important part is

let sender = ensure_signed(origin)?; (1)
Self::is_registered_enclave(&sender)?; (2)

(1) The call must be signed by the sender. (2) This sender must be a registered enclave.

So we're effectively only allowing a registered enclave to make such a call. Any other will not be processed. Tampering of the block by the untrusted part is prevented by the signing part: If the call is changed after the (trusted) enclave has created it, the signature will not be valid anymore and the call will fail at (1).

[tdelabro]

worker/service/src/parentchain_block_syncer.rs

impl<ParentchainApi, EnclaveApi> SyncParentchainBlocks
    for ParentchainBlockSyncer<ParentchainApi, EnclaveApi>
where
    ParentchainApi: ChainApi,
    EnclaveApi: Sidechain,
{
    fn sync_parentchain(&self, last_synced_header: Header) -> Header {
        trace!("Getting current head");
        let curr_block: SignedBlock = self.parentchain_api.last_finalized_block().unwrap().unwrap();
        let curr_block_number = curr_block.block.header.number;

        let mut until_synced_header = last_synced_header;
        loop {
            let block_chunk_to_sync = self
                .parentchain_api
                .get_blocks(
                    until_synced_header.number + 1,
                    min(until_synced_header.number + BLOCK_SYNC_BATCH_SIZE, curr_block_number),
                )
                .unwrap();
            println!("[+] Found {} block(s) to sync", block_chunk_to_sync.len());
            if block_chunk_to_sync.is_empty() {
                return until_synced_header
            }

            if let Err(e) = self.enclave_api.sync_parentchain(block_chunk_to_sync.as_slice(), 0) {
                error!("{:?}", e);
                // enclave might not have synced
                return until_synced_header
            };
            until_synced_header = block_chunk_to_sync
                .last()
                .map(|b| b.block.header.clone())
                .expect("Chunk can't be empty; qed");
            println!(
                "Synced {} out of {} finalized parentchain blocks",
                until_synced_header.number, curr_block_number,
            )
        }
    }

In this part of the code the untrusted part of the worker get new blocks form the chain and pass them to the enclave. In this implementation it's not modifying the blocks. But a malicious/corrupted environment could be altering the blocks before passing it to the enclave. Or feed the enclave with a custom fork of the chain instead of the original one. You just have to change the arguments used in this call self.enclave_api.sync_parentchain(block_chunk_to_sync.as_slice(), 0). Because there is no verification in confirm_processed_parentchain_block of what is the actual block being imported by the enclave light client, we have no way to detect this, and the enclave will be kept registered even when fed with this malicious blocks.

[haerdib]

That's correct, this is a problem. But thankfully not only for our light client, but for all blockchain light clients. By default, we do not trust the other party in a blockchain environment. Hence, for light clients that only import block headers, smart people thought of the merkle tree proof. According to https://www.parity.io/blog/what-is-a-light-client/ :

Now, back to our light clients. As a starting point, a light client needs to download the block headers of the blockchain. The light client does not need to trust the full node for every request that it makes to the full node. This is because the block headers contain a piece of information called the Merkle tree root. The Merkle tree root is like a fingerprint of all information on the blockchain about account balances and smart contract storage. If any tiny bit of information changes, this fingerprint will change as well. Assuming that the majority of the miners are honest, block headers and therefore the fingerprints they contain are assumed to be valid. A light client may need to request information from full nodes such as the balance of a specific account. Knowing the fingerprints for each block, a light client can verify whether the answer given by the full node matches with the fingerprint it has. This is a powerful tool to prove the authenticity of information without knowing it beforehand.

Long story short: The enclave's light client itself must check that only valid blocks are imported.

[tdelabro]

By checking the merkle proof the lightclient can ensure that the block he is processing is indeed related to the previous one. But if a malicious untrusted environment start feeding the enclave with headers from a fork of the parentchain, those blocks will seem legit to the lightclient. And it will send confirm_processed_parentchain_block calls to the parentchain. But the block it will have registered and their hash, will not be the one of block existing on the parentchain, it will be hash of block from the forked chain. That's why I think the confirm_processed_parentchain_block should verify that the hash of the processed block is indeed the hash of one of the block it has produced. Not block from a fork

[haerdib]

Ah, I see your point. This could be mitigated by only importing finalized header on the enclave side. Then again, some use cases might not want to wait for finalization, so such an extra check might be necessary, you're right.

[tdelabro]

Perfect. I could do a branch and propose something but don't know how to do it. Is there a Substrate feature/call to retrieve the list of all previous block/headers in the chain ? There should be one somewhere

[tdelabro]

Never mind I think I found id: frame_system::pallet::BlockHash. https://docs.substrate.io/rustdocs/latest/frame_system/pallet/type.BlockHash.html It map block number to block hashes. Would you like me to implement a check inside confirm_processed_parentchain_block to make sure that the block_hash argument value can be found inside the BlockHash storage ?

And if the value is not found, I guess it would unregister the enclave

[haerdib]

Could you wait until tomorrow before starting implementing? I'd love to hear a third opinion on this matter.

[tdelabro]

Sure I will ! Keep me in touch. It's nothing urgent anyway

[haerdib]

Sorry for being late. I was finally able to catch a sparring partner. We think it would be definitely useful. So if you would like to implement such a check, that'd be great.

[tdelabro]

Nice. We will come with a pull request soon

[tdelabro]

we did this: https://github.com/integritee-network/pallets/pull/33 Problem is: we are looping over the block in no specific order. Bad efficiency. Is there a better way ?

[haerdib]

Hm. I agree, this is not ideal. @clangenb do you know of a better way?

[tdelabro]

A way could be for the enclave to call this extrinsic with one extra argument: the block number. Then it become O(1) to retrieve the right blockhash from the storage and do the comparison. And I think the light client is able to tell the block height