Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement tari_crypto hashing API: ConsensusHasher::default() #4396

Closed
hansieodendaal opened this issue Aug 5, 2022 · 5 comments
Closed
Assignees
Labels
A-security Area - Security related

Comments

@hansieodendaal
Copy link
Contributor

hansieodendaal commented Aug 5, 2022

Effective GitHub hash: d25d726
# Reference Function/Area Yes/No URL URL long
2 base_layer/core/src/blocks/block_header.rs:220 ConsensusHasher::default() [pub fn merged_mining_hash] No ? block_header.rs:220 https://github.com/tari-project/tari/blob/development/base_layer/core/src/blocks/block_header.rs#L220
3 base_layer/core/src/blocks/block_header.rs:297 ConsensusHasher::default() [Hashable for BlockHeader] Yes block_header.rs:297 https://github.com/tari-project/tari/blob/development/base_layer/core/src/blocks/block_header.rs#L297
8 base_layer/core/src/transactions/transaction_components/mod.rs:97 ConsensusHasher::default() [pub(super) fn hash_output(] Yes mod.rs:97 https://github.com/tari-project/tari/blob/development/base_layer/core/src/transactions/transaction_components/mod.rs#L97
14 base_layer/core/src/transactions/transaction_components/transaction_input.rs:370 ConsensusHasher::default() [pub fn canonical_hash] Yes transaction_input.rs:370 https://github.com/tari-project/tari/blob/development/base_layer/core/src/transactions/transaction_components/transaction_input.rs#L370
15 base_layer/core/src/transactions/transaction_components/transaction_kernel.rs:193 ConsensusHasher::default() [Hashable for TransactionKernel] Yes transaction_kernel.rs:193 https://github.com/tari-project/tari/blob/development/base_layer/core/src/transactions/transaction_components/transaction_kernel.rs#L193
16 base_layer/core/src/transactions/transaction_components/transaction_output.rs:398 ConsensusHasher::default() [pub fn witness_hash] Yes transaction_output.rs:398 https://github.com/tari-project/tari/blob/development/base_layer/core/src/transactions/transaction_components/transaction_output.rs#L398
@hansieodendaal hansieodendaal self-assigned this Aug 12, 2022
@SWvheerden SWvheerden moved this to In Progress in Tari Esme Testnet Aug 15, 2022
@SWvheerden SWvheerden removed the status in Tari Esme Testnet Aug 15, 2022
@hansieodendaal hansieodendaal added the A-security Area - Security related label Aug 15, 2022
@hansieodendaal
Copy link
Contributor Author

Hi @AaronFeickert, would using the domain separated hashing API for the above hashes be required? We were debating the issue and would like your opinion. Just search for ConsensusHasher::default() in the code base and you will find where it is used.

ConsensusHasher::default() implements standard Blake2b hashing.

@stringhandler stringhandler moved this to Backlog in Tari Esme Testnet Aug 16, 2022
@jorgeantonio21 jorgeantonio21 moved this from Backlog to In Progress in Tari Esme Testnet Aug 17, 2022
@hansieodendaal hansieodendaal removed their assignment Aug 18, 2022
@stringhandler stringhandler moved this from In Progress to On Hold in Tari Esme Testnet Aug 18, 2022
@hansieodendaal hansieodendaal self-assigned this Aug 18, 2022
@AaronFeickert
Copy link
Collaborator

Using (or perhaps even requiring) domain separation seems like an easy win here, unless there is a compelling reason not to. As discussed with @hansieodendaal earlier, the complexity of consensus hashing for the various underlying structs and types would likely make direct use of the hashing API challenging, and probably unnecessarily duplicate a lot of the existing work on those structs' and types' consensus encodings. A better use of engineering effort may be a thorough analysis of those consensus encodings, in order to be certain they are canonical and cannot lead to malicious or accidental collision.

I did observe several uses of copy_into_fixed_array_lossy in consensus hasher operations, which should be carefully examined. If the input to this function exceeds the given array length, it will be truncated; if the input is shorter, it will be padded with a default element. In both cases, it is generally possible to induce collisions. A safer approach is to either use copy_into_fixed_array (which enforces fixed-length input), or use a safe length prepending in case variable-length input is required.

@hansieodendaal hansieodendaal moved this from On Hold to In Progress in Tari Esme Testnet Aug 22, 2022
@sdbondi
Copy link
Member

sdbondi commented Aug 22, 2022

Some context on the copy_into_fixed_array_lossy "hack". This is required because we used a variable length vector type (Vec<u8>) in the block header types and for the Hashable trait where it should have been fixed. Perhaps we should change those types to the FixedHash type and then we can remove copy_into_fixed_array_lossy.

@AaronFeickert
Copy link
Collaborator

Some context on the copy_into_fixed_array_lossy "hack". This is required because we used a variable length vector type (Vec<u8>) in the block header types and for the Hashable trait where it should have been fixed. Perhaps we should change those types to the FixedHash type and then we can remove copy_into_fixed_array_lossy.

Yeah, I noticed how the typing was being used in those cases. Agreed that switching to a fixed-length type seems like a wise idea to prevent any future changes that unintentionally introduce collision vectors.

stringhandler pushed a commit that referenced this issue Aug 23, 2022
…onsensus hashing (#4522)

Description
---
- Added domain hashing to all previous uses of default consensus hashing.
- Changed `output_mr`, `witness_mr`, `kernel_mr` and `input_mr` types in  `pub struct BlockHeader` to be `FixedHash` instead of variable length  `BlockHash`.

Motivation and Context
---
As per issue #4396.

How Has This Been Tested?
---
- Passed all unit tests
- Passed all cucumber tests
@hansieodendaal
Copy link
Contributor Author

Implemented with #4522 and #4527

Repository owner moved this from In Progress to Done in Tari Esme Testnet Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-security Area - Security related
Projects
Archived in project
Development

No branches or pull requests

3 participants