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

Remove some Identity conversions that could panic #1966

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 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
4 changes: 2 additions & 2 deletions crates/core/src/sql/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -719,8 +719,8 @@ pub(crate) mod tests {
Ok(())
})?;

let server = Identity::from_hashing_bytes("server");
let client = Identity::from_hashing_bytes("client");
let server = Identity::from_claims("issuer", "server");
let client = Identity::from_claims("issuer", "client");

let internal_auth = AuthCtx::new(server, server);
let external_auth = AuthCtx::new(server, client);
Expand Down
39 changes: 13 additions & 26 deletions crates/lib/src/identity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use blake3;
use core::mem;
use spacetimedb_bindings_macro::{Deserialize, Serialize};
use spacetimedb_sats::hex::HexString;
use spacetimedb_sats::{hash, impl_st, u256, AlgebraicType, AlgebraicValue};
use spacetimedb_sats::{impl_st, u256, AlgebraicType, AlgebraicValue};
use std::{fmt, str::FromStr};

pub type RequestId = u32;
Expand Down Expand Up @@ -98,27 +98,6 @@ impl Identity {
self.__identity__
}

/// Returns an `Identity` defined as the given byte `slice`.
/// The slice is assumed to be in BIG-ENDIAN format.
///
/// This method is the correct choice if you have converted the bytes of a hexadecimal-formatted `Identity`
/// to a byte array in the following way:
/// ```ignore
/// "0xb0b1b2..."
/// ->
/// [0xb0, 0xb1, 0xb2, ...]
/// ```
pub fn from_be_slice(slice: &[u8]) -> Self {
Self::from_be_byte_array(slice.try_into().unwrap())
}

/// Returns an `Identity` defined as the given byte `slice`.
/// The slice is assumed to be in LITTLE-ENDIAN format.
/// If you are parsing an `Identity` from a string, you probably want `from_be_slice` instead.
pub fn from_slice(slice: &[u8]) -> Self {
Self::from_byte_array(slice.try_into().unwrap())
}

#[doc(hidden)]
pub fn __dummy() -> Self {
Self::ZERO
Expand Down Expand Up @@ -176,10 +155,6 @@ impl Identity {
pub fn from_hex(hex: impl AsRef<[u8]>) -> Result<Self, hex::FromHexError> {
hex::FromHex::from_hex(hex)
}

pub fn from_hashing_bytes(bytes: impl AsRef<[u8]>) -> Self {
Self::from_byte_array(hash::hash_bytes(bytes).data)
}
}

impl fmt::Display for Identity {
Expand Down Expand Up @@ -269,6 +244,17 @@ mod tests {
);
}

// Make sure the checksum is valid.
jsdt marked this conversation as resolved.
Show resolved Hide resolved
fn validate_checksum(id: &[u8; 32]) -> bool {
let checksum_input = &id[6..];
let mut checksum_input_with_prefix = [0u8; 28];
checksum_input_with_prefix[2..].copy_from_slice(checksum_input);
checksum_input_with_prefix[0] = 0xc2;
checksum_input_with_prefix[1] = 0x00;
let checksum_hash = &blake3::hash(&checksum_input_with_prefix);
checksum_hash.as_bytes()[0..4] == id[2..6]
}

proptest! {
#[test]
fn identity_conversions(w0: u128, w1: u128) {
Expand All @@ -288,6 +274,7 @@ mod tests {
fn from_claims_formats_correctly(s1 in string_regex(r".{3,5}").unwrap(), s2 in string_regex(r".{3,5}").unwrap()) {
let id = Identity::from_claims(&s1, &s2);
prop_assert!(id.to_hex().starts_with("c200"));
prop_assert!(validate_checksum(&id.to_be_byte_array()));
}
}
}
38 changes: 26 additions & 12 deletions crates/standalone/src/control_db.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use anyhow::Context;
use spacetimedb::identity::Identity;
use spacetimedb::messages::control_db::{Database, EnergyBalance, Node, Replica};
use spacetimedb::{energy, stdb_path};
Expand Down Expand Up @@ -36,6 +37,8 @@ pub enum Error {
DomainRegistrationFailure(DomainName),
#[error("failed to decode data")]
Decoding(#[from] bsatn::DecodeError),
#[error("failed to encode data")]
Encoding(#[from] bsatn::EncodeError),
#[error(transparent)]
DomainParsing(#[from] DomainParsingError),
#[error(transparent)]
Expand Down Expand Up @@ -76,12 +79,23 @@ impl ControlDb {
}
}

/// A helper to convert a `sled::IVec` into an `Identity`.
/// This expects the identity to be in LITTLE_ENDIAN format.
/// This fails if the `sled::IVec` is not 32 bytes long.
fn identity_from_le_ivec(ivec: &sled::IVec) -> Result<Identity> {
let identity_bytes: [u8; 32] = ivec
.as_ref()
.try_into()
.map_err(|_| anyhow::anyhow!("invalid size for identity: {}", ivec.len()))?;
Ok(Identity::from_byte_array(identity_bytes))
}

impl ControlDb {
pub fn spacetime_dns(&self, domain: &DomainName) -> Result<Option<Identity>> {
let tree = self.db.open_tree("dns")?;
let value = tree.get(domain.to_lowercase().as_bytes())?;
if let Some(value) = value {
return Ok(Some(Identity::from_slice(&value[..])));
return Ok(Some(identity_from_le_ivec(&value)?));
}
Ok(None)
}
Expand Down Expand Up @@ -177,7 +191,9 @@ impl ControlDb {
let current_owner = tree.get(&key)?;
match current_owner {
Some(owner) => {
if Identity::from_slice(&owner[..]) == owner_identity {
let current_owner =
identity_from_le_ivec(&owner).context("Invalid current owner in top_level_domains")?;
if current_owner == owner_identity {
Ok(RegisterTldResult::AlreadyRegistered { domain: tld })
} else {
Ok(RegisterTldResult::Unauthorized { domain: tld })
Expand All @@ -197,7 +213,7 @@ impl ControlDb {
pub fn spacetime_lookup_tld(&self, domain: impl AsRef<TldRef>) -> Result<Option<Identity>> {
let tree = self.db.open_tree("top_level_domains")?;
match tree.get(domain.as_ref().to_lowercase().as_bytes())? {
Some(owner) => Ok(Some(Identity::from_slice(&owner[..]))),
Some(owner) => Ok(Some(identity_from_le_ivec(&owner)?)),
None => Ok(None),
}
}
Expand All @@ -208,7 +224,7 @@ impl ControlDb {
let scan_key: &[u8] = b"";
for result in tree.range(scan_key..) {
let (_key, value) = result?;
let database = compat::Database::from_slice(&value).unwrap().into();
let database = compat::Database::from_slice(&value)?.into();
databases.push(database);
}
Ok(databases)
Expand All @@ -228,7 +244,7 @@ impl ControlDb {
let key = identity.to_be_byte_array();
let value = tree.get(&key[..])?;
if let Some(value) = value {
let database = compat::Database::from_slice(&value[..]).unwrap().into();
let database = compat::Database::from_slice(&value[..])?.into();
return Ok(Some(database));
}
Ok(None)
Expand All @@ -245,7 +261,7 @@ impl ControlDb {

database.id = id;

let buf = sled::IVec::from(compat::Database::from(database).to_vec().unwrap());
let buf = sled::IVec::from(compat::Database::from(database).to_vec()?);

tree.insert(key, buf.clone())?;

Expand Down Expand Up @@ -277,7 +293,7 @@ impl ControlDb {
let scan_key: &[u8] = b"";
for result in tree.range(scan_key..) {
let (_key, value) = result?;
let replica = bsatn::from_slice(&value[..]).unwrap();
let replica = bsatn::from_slice(&value[..])?;
replicas.push(replica);
}
Ok(replicas)
Expand Down Expand Up @@ -377,7 +393,7 @@ impl ControlDb {
pub fn _update_node(&self, node: Node) -> Result<()> {
let tree = self.db.open_tree("node")?;

let buf = bsatn::to_vec(&node).unwrap();
let buf = bsatn::to_vec(&node)?;

tree.insert(node.id.to_be_bytes(), buf)?;
Ok(())
Expand Down Expand Up @@ -409,10 +425,8 @@ impl ControlDb {
given: balance_entry.1.len(),
})?;
let balance = i128::from_be_bytes(arr);
let energy_balance = EnergyBalance {
identity: Identity::from_slice(balance_entry.0.iter().as_slice()),
balance,
};
let identity = identity_from_le_ivec(&balance_entry.0).context("invalid identity in energy_budget")?;
let energy_balance = EnergyBalance { identity, balance };
balances.push(energy_balance);
}
Ok(balances)
Expand Down
5 changes: 3 additions & 2 deletions crates/standalone/src/control_db/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ use std::str::FromStr;

use once_cell::sync::Lazy;
use spacetimedb::messages::control_db::HostType;
use spacetimedb_client_api::auth::LOCALHOST;
use spacetimedb_lib::error::ResultTest;
use spacetimedb_lib::Hash;
use tempfile::TempDir;

use super::*;

static ALICE: Lazy<Identity> = Lazy::new(|| Identity::from_hashing_bytes("alice"));
static BOB: Lazy<Identity> = Lazy::new(|| Identity::from_hashing_bytes("bob"));
static ALICE: Lazy<Identity> = Lazy::new(|| Identity::from_claims(LOCALHOST, "alice"));
static BOB: Lazy<Identity> = Lazy::new(|| Identity::from_claims(LOCALHOST, "bob"));

#[test]
fn test_register_tld() -> anyhow::Result<()> {
Expand Down
Loading