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

TOB-WORM-2: Hashing scheme of dynamically sized data could allow collisions #1530

Open
tbjump opened this issue Sep 6, 2022 · 0 comments
Open

Comments

@tbjump
Copy link
Contributor

tbjump commented Sep 6, 2022

Target: solana/bridge/program/src/api/post_vaa.rs#L219-L238
Severity: Low

Description
Whenever a message has received a quorum from guardians, relayers will call post_vaa to announce the message’s validity. The post_vaa function performs several validation checks one of which hashes the PostVAAData structure to ensure that it matches what the guardians signed. Each field of the structure is written into a vector of bytes and then hashed.
Improper usage of write_all where adjacent, dynamically-sized fields in a structure are hashed allows for collisions, breaking the assumption that each VAA corresponds to one message. If the fields emitter_address and payload in Figure 2.1 are moved in the future, this would be exacerbated.

let body = {
    let mut v = Cursor::new(Vec::new());
    v.write_u32::<BigEndian>(vaa.timestamp)?;
    v.write_u32::<BigEndian>(vaa.nonce)?;
    v.write_u16::<BigEndian>(vaa.emitter_chain)?;
    v.write_all(&vaa.emitter_address)?;
    v.write_u64::<BigEndian>(vaa.sequence)?;
    v.write_u8(vaa.consistency_level)?;
    v.write_all(&vaa.payload)?;
    v.into_inner()
};
// Hash this body, which is expected to be the same as the hash currently stored in
the
// signature account, binding that set of signatures to this VAA.
let body_hash: [u8; 32] = {
    let mut h = sha3::Keccak256::default();
    h.write(body.as_slice())
        .map_err(|_| ProgramError::InvalidArgument)?;
    h.finalize().into()
};

Exploit Scenario
An attacker generates VAA data that is invalid but results in the same hash as one which has received signatures from a quorum of guardians . Then, the attacker submits the VAA data to a third-party program that uses the core messaging protocol. Since the program assumes only one valid structure exists for each VAA hash, it permits an invalid input, executing actions based on falsified data. The following is an example of a hash collision:

let vaa0 = PostVAAData {
    version: 1,
    guardian_set_index: 2,
// Body part
    timestamp: 3,
    nonce: 4,
    emitter_chain: 5,
    emitter_address: Vec::from([2, 3, 4]),
    sequence: 7,
    consistency_level: 255,
    payload: Vec::from([0, 0, 0, 0, 0, 0, 0, 7, 255, 5, 6, 7, 8]),
};
let vaa1 = PostVAAData {
    version: 1,
    guardian_set_index: 2,
// Body part
    timestamp: 3,
    nonce: 4,
    emitter_chain: 5,
    emitter_address: Vec::from([2, 3, 4, 0, 0, 0, 0, 0, 0, 0, 7, 255]),
    sequence: 7,
    consistency_level: 255,
    payload: Vec::from([5, 6, 7, 8]),
};

Figure 2.2: an example hash collision
Short term, insert the length of dynamically sized fields into the vector prior to hashing the
vector of bytes. Doing so will help avoid the possibility of hash collisions.
Long term, review the cardinality assumptions of VAA’s and messages to ensure that undefined behavior is not present.

@aadam-10 aadam-10 added temp and removed temp labels Nov 21, 2022
@aadam-10 aadam-10 added this to the Solana refactor/rewrite milestone Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants