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

fix(dpp): panic when generating more than 12 identity keys #2195

Open
wants to merge 4 commits into
base: v1.6-dev-ugly
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/rs-dpp/src/identity/identity_public_key/random.rs
Original file line number Diff line number Diff line change
Expand Up @@ -615,15 +615,15 @@ impl IdentityPublicKey {
used_key_matrix[4] = true; //also a master key
used_key_matrix[8] = true; //also a master key
used_key_matrix[12] = true; //also a master key
main_keys.extend((3..key_count).map(|i| {
Self::random_authentication_key_with_private_key_with_rng(
for i in 6..key_count {
let privkey = Self::random_authentication_key_with_private_key_with_rng(
i,
rng,
Some((i, &mut used_key_matrix)),
platform_version,
)
.unwrap()
}));
)?;
main_keys.push(privkey);
}
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Possible index out of bounds when key_count exceeds 16

The used_key_matrix is initialized with a fixed size of 16:

let mut used_key_matrix = [false; 16].to_vec();

If key_count exceeds 16, accessing indices beyond 15 will cause an index out of bounds error.

Consider initializing used_key_matrix based on key_count to accommodate all possible key IDs:

-let mut used_key_matrix = [false; 16].to_vec();
+let mut used_key_matrix = vec![false; key_count as usize];

Ensure that all indices set to true are within the bounds of the new used_key_matrix.


⚠️ Potential issue

Potential skipping of key IDs 3 to 5 due to loop starting at index 6

The for loop starts at i = 6, which skips key IDs 3, 4, and 5. This means that keys with these IDs are not generated or added to main_keys, which might not be the intended behavior.

To include all sequential keys, consider starting the loop at i = 3:

-for i in 6..key_count {
+for i in 3..key_count {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for i in 6..key_count {
let privkey = Self::random_authentication_key_with_private_key_with_rng(
i,
rng,
Some((i, &mut used_key_matrix)),
platform_version,
)
.unwrap()
}));
)?;
main_keys.push(privkey);
}
for i in 3..key_count {
let privkey = Self::random_authentication_key_with_private_key_with_rng(
i,
rng,
Some((i, &mut used_key_matrix)),
platform_version,
)?;
main_keys.push(privkey);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the loop

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lklimek, great to hear that you've fixed the loop! 😊


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

Ok(main_keys)
}

Expand Down
39 changes: 32 additions & 7 deletions packages/rs-dpp/src/identity/identity_public_key/v0/random.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,29 +70,54 @@ impl IdentityPublicKeyV0 {
platform_version: &PlatformVersion,
) -> Result<(Self, Vec<u8>), ProtocolError> {
// we have 16 different permutations possible
let mut binding = [false; 16].to_vec();
const MAX_KEYS: KeyCount = 16;

let mut binding = [false; MAX_KEYS as usize].to_vec();
let (key_count, key_matrix) = used_key_matrix.unwrap_or((0, &mut binding));
if key_count > 16 {

// input validation
if key_count != key_matrix.iter().filter(|x| **x).count() as u32 {
return Err(ProtocolError::PublicKeyGenerationError(
"invalid argument: key count in used_key_matrix.0 doesn't match actual number of used keys".to_string(),
));
}

// we need space for at least one additional key
if key_count > MAX_KEYS - 1 {
return Err(ProtocolError::PublicKeyGenerationError(
"too many keys already created".to_string(),
));
}
let key_number = rng.gen_range(0..(12 - key_count as u8));
// now we need to find the first bool that isn't set to true

// max_key_number is the number of keys that can be created
let max_key_number = MAX_KEYS - key_count;
let key_number = rng.gen_range(0..max_key_number);
// now we need to find n'th not used key of this key_matrix (where `n = key_number`),
// that is: the first bool that isn't set to true
let mut needed_pos = None;
let mut counter = 0;
let mut used = 0;
key_matrix.iter_mut().enumerate().for_each(|(pos, is_set)| {
if !*is_set {
if counter == key_number {
needed_pos = Some(pos as u8);
*is_set = true;
}
counter += 1;
} else {
used += 1; // for debugging purposes
}
});
let needed_pos = needed_pos.ok_or(ProtocolError::PublicKeyGenerationError(
"too many keys already created".to_string(),
))?;
// should never happen, as we have input validation above
assert_eq!(
used, key_count,
"incorrect number of used keys in key_matrix {:?}",
key_matrix,
);
let needed_pos = needed_pos.ok_or(ProtocolError::PublicKeyGenerationError(format!(
"too many keys already created: , key_count: {}, random key number: {}, unused key counter: {}, used key counter: {}",
key_count, key_number, counter, used,
)))?;
let key_type = needed_pos.div(&4);
let security_level = needed_pos.rem(&4);
let security_level = SecurityLevel::try_from(security_level).unwrap();
Expand Down
1 change: 0 additions & 1 deletion packages/rs-drive-abci/tests/strategy_tests/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ mod tests {
use crate::execution::{continue_chain_for_strategy, run_chain_for_strategy};
use crate::query::QueryStrategy;
use crate::strategy::{FailureStrategy, MasternodeListChangesStrategy};
use assert_matches::assert_matches;
use dashcore_rpc::dashcore::hashes::Hash;
use dashcore_rpc::dashcore::BlockHash;
use dashcore_rpc::json::QuorumType;
Expand Down
Loading