Skip to content

Commit

Permalink
crypto: enforce correct hash prefix during base58check decoding
Browse files Browse the repository at this point in the history
Previously, hashes could accidentally be parsed with an incorrect hash
type, provided:
- the lengths of the encoded were the same
- the lengths of the base58 prefixes were the same

This would manifest, for example, by the following succeeding:
`ContractTz1Hash::from_base58check("tz4FENGt5zkiGaHPm1ya4MgLomgkL1k7Dy7q")`

This will now return
`Err(FromBase58CheckError::IncorrectBase58Prefix)` instead.
  • Loading branch information
emturner committed Dec 13, 2023
1 parent fd398b0 commit 169f0ae
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 4 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

### Added

- Nothing.
- Add `FromBase58CheckError::IncorrectBase58Prefix` variant.

### Changed

Expand All @@ -25,6 +25,7 @@ parameterized by the lifetime of the input byte slice.
### Fixed

- Fix prefix used in `SeedEd25519` encoding.
- Add explicit prefix check during base58check decoding.

### Security

Expand Down
3 changes: 3 additions & 0 deletions crypto/src/base58.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ pub enum FromBase58CheckError {
DataTooLong,
#[error("mismatched data lenght: expected {expected}, actual {actual}")]
MismatchedLength { expected: usize, actual: usize },
/// Prefix does not match expected.
#[error("incorrect base58 prefix for hash type")]
IncorrectBase58Prefix,
}

/// Possible errors for ToBase58Check
Expand Down
29 changes: 26 additions & 3 deletions crypto/src/hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,14 +482,19 @@ impl HashType {
return Ok(hash.to_vec());
}
}
} else if !hash.starts_with(self.base58check_prefix()) {
println!("expected: {:?}, found: {hash:?}", self.base58check_prefix());
return Err(FromBase58CheckError::IncorrectBase58Prefix);
}

let expected_len = self.size() + self.base58check_prefix().len();
if expected_len != hash.len() {
return Err(FromBase58CheckError::MismatchedLength {
expected: expected_len,
actual: hash.len(),
});
}

// prefix is not present in a binary representation
hash.drain(0..self.base58check_prefix().len());
Ok(hash)
Expand Down Expand Up @@ -1056,9 +1061,10 @@ mod tests {
}

#[test]
fn test_b58_to_hash_mismatched_lenght() -> Result<(), anyhow::Error> {
let b58 = HashType::ChainId.hash_to_b58check(&[0, 0, 0, 0])?;
let result = HashType::BlockHash.b58check_to_hash(&b58);
fn test_b58_to_hash_mismatched_length() -> Result<(), anyhow::Error> {
let b58 = "BwKZdq9yAc1ucmPPoUeRxRQRUeks64eswrLoSa2eZipYwB3UftmTd1pmg4uyiwU6Ge3guh7CoZdpL4YPm35Ajvu5gQu5mYwEgwA8UmjZNaXV7ecc7qkcoe6xro";
let result = HashType::BlockHash.b58check_to_hash(b58);
println!("{result:?}");
assert!(matches!(
result,
Err(FromBase58CheckError::MismatchedLength {
Expand Down Expand Up @@ -1268,6 +1274,23 @@ mod tests {
);
}

#[test]
fn from_base58check_incorrect_prefix() {
let h = ContractTz1Hash::from_base58_check("tz4FENGt5zkiGaHPm1ya4MgLomgkL1k7Dy7q");

assert!(matches!(
h,
Err(FromBase58CheckError::IncorrectBase58Prefix)
));

let h = ContractTz4Hash::from_base58_check("tz1ei4WtWEMEJekSv8qDnu9PExG6Q8HgRGr3");

assert!(matches!(
h,
Err(FromBase58CheckError::IncorrectBase58Prefix)
));
}

#[test]
fn block_payload_hash() {
let operation_0 = "oom9d3PpjjaMzgg9mZ1pDrF8kjdyzDb41Bd2XE6Y3kRtFHXLku3";
Expand Down

0 comments on commit 169f0ae

Please sign in to comment.