Add Secp address sanity check (#438)

* Add Secp address sanity check * Bump versions * Add length tests
ChainSafe · May 22, 2020 · 9ef6e67 · 9ef6e67
1 parent c58aeb4
commit 9ef6e67
Show file tree

Hide file tree

Showing 11 changed files with 54 additions and 24 deletions.
diff --git a/crypto/Cargo.toml b/crypto/Cargo.toml
@@ -2,13 +2,13 @@
 name = "forest_crypto"
 description = "Filecoin crypto utilities for use in Forest"
 license = "MIT OR Apache-2.0"
-version = "0.1.0"
+version = "0.2.0"
 authors = ["ChainSafe Systems <info@chainsafe.io>"]
 edition = "2018"
 repository = "https://github.com/ChainSafe/forest"
 
 [dependencies]
-address = { package = "forest_address", path = "../vm/address", version = "0.1" }
+address = { package = "forest_address", path = "../vm/address", version = "0.2" }
 encoding = { package = "forest_encoding", path = "../encoding", version = "0.1" }
 libsecp256k1 = "0.3.4"
 bls-signatures = "0.6.0"

diff --git a/crypto/src/signature.rs b/crypto/src/signature.rs
@@ -196,7 +196,7 @@ fn ecrecover(hash: &[u8; 32], signature: &[u8; 65]) -> Result<Address, Error> {
 
     let key = recover(&message, &sig, &rec_id)?;
     let ret = key.serialize();
-    let addr = Address::new_secp256k1(&ret);
+    let addr = Address::new_secp256k1(&ret)?;
     Ok(addr)
 }
 

diff --git a/utils/test_utils/src/chain_structures.rs b/utils/test_utils/src/chain_structures.rs
@@ -37,7 +37,7 @@ fn template_header(
         .parents(TipsetKeys {
             cids: vec![cids[0].clone()],
         })
-        .miner_address(Address::new_secp256k1(&ticket_p))
+        .miner_address(Address::new_actor(&ticket_p))
         .timestamp(timestamp)
         .ticket(Ticket {
             vrfproof: VRFProof::new(ticket_p),

diff --git a/vm/Cargo.toml b/vm/Cargo.toml
@@ -2,14 +2,14 @@
 name = "forest_vm"
 description = "Forest VM types"
 license = "MIT OR Apache-2.0"
-version = "0.1.0"
+version = "0.2.0"
 authors = ["ChainSafe Systems <info@chainsafe.io>"]
 edition = "2018"
 repository = "https://github.com/ChainSafe/forest"
 
 [dependencies]
 num-bigint = { package = "forest_bigint", path = "../utils/bigint", version = "0.1" }
-address = { package = "forest_address", path = "./address", version = "0.1" }
+address = { package = "forest_address", path = "./address", version = "0.2" }
 encoding = { package = "forest_encoding", path = "../encoding", version = "0.1" }
 serde = { version = "1.0", features = ["derive"] }
 cid = { package = "forest_cid", path = "../ipld/cid", version = "0.1", features = ["cbor"] }

diff --git a/vm/actor/tests/account_actor_test.rs b/vm/actor/tests/account_actor_test.rs
@@ -60,7 +60,7 @@ macro_rules! account_tests {
 
 account_tests! {
     happy_construct_secp256k1_address: (
-        Address::new_secp256k1(&[1, 2, 3]),
+        Address::new_secp256k1(&[2; address::SECP_PUB_LEN]).unwrap(),
         ExitCode::Ok
     ),
     happy_construct_bls_address: (

diff --git a/vm/address/Cargo.toml b/vm/address/Cargo.toml
@@ -2,7 +2,7 @@
 name = "forest_address"
 description = "Filecoin addresses for use in Forest"
 license = "MIT OR Apache-2.0"
-version = "0.1.0"
+version = "0.2.0"
 authors = ["ChainSafe Systems <info@chainsafe.io>"]
 edition = "2018"
 repository = "https://github.com/ChainSafe/forest"

diff --git a/vm/address/src/errors.rs b/vm/address/src/errors.rs
@@ -1,7 +1,7 @@
 // Copyright 2020 ChainSafe Systems
 // SPDX-License-Identifier: Apache-2.0, MIT
 
-use super::{BLS_PUB_LEN, PAYLOAD_HASH_LEN};
+use super::{BLS_PUB_LEN, PAYLOAD_HASH_LEN, SECP_PUB_LEN};
 use data_encoding::DecodeError;
 use encoding::{CodecProtocol, Error as EncodingError};
 use leb128::read::Error as Leb128Error;
@@ -23,6 +23,8 @@ pub enum Error {
     InvalidPayloadLength(usize),
     #[error("Invalid BLS pub key length, wanted: {} got: {0}", BLS_PUB_LEN)]
     InvalidBLSLength(usize),
+    #[error("Invalid SECP pub key length, wanted: {} got: {0}", SECP_PUB_LEN)]
+    InvalidSECPLength(usize),
     #[error("Invalid address checksum")]
     InvalidChecksum,
     #[error("Decoding for address failed: {0}")]

diff --git a/vm/address/src/lib.rs b/vm/address/src/lib.rs
@@ -23,9 +23,18 @@ const ADDRESS_ENCODER: Encoding = new_encoding! {
     padding: None,
 };
 
-pub const BLS_PUB_LEN: usize = 48;
+/// Hash length of payload for Secp and Actor addresses.
 pub const PAYLOAD_HASH_LEN: usize = 20;
+
+/// Uncompressed secp public key used for validation of Secp addresses.
+pub const SECP_PUB_LEN: usize = 65;
+
+/// BLS public key length used for validation of BLS addresses.
+pub const BLS_PUB_LEN: usize = 48;
+
+/// Length of the checksum hash for string encodings.
 pub const CHECKSUM_HASH_LEN: usize = 4;
+
 const MAX_ADDRESS_LEN: usize = 84 + 2;
 const MAINNET_PREFIX: &str = "f";
 const TESTNET_PREFIX: &str = "t";
@@ -69,11 +78,14 @@ impl Address {
     }
 
     /// Generates new address using Secp256k1 pubkey
-    pub fn new_secp256k1(pubkey: &[u8]) -> Self {
-        Self {
+    pub fn new_secp256k1(pubkey: &[u8]) -> Result<Self, Error> {
+        if pubkey.len() != 65 {
+            return Err(Error::InvalidSECPLength(pubkey.len()));
+        }
+        Ok(Self {
             network: NETWORK_DEFAULT,
             payload: Payload::Secp256k1(address_hash(pubkey)),
-        }
+        })
     }
 
     /// Generates new address using the Actor protocol

diff --git a/vm/address/tests/address_test.rs b/vm/address/tests/address_test.rs
@@ -5,22 +5,38 @@ use data_encoding::{DecodeError, DecodeKind};
 use encoding::{from_slice, Cbor};
 use forest_address::{
     checksum, validate_checksum, Address, Error, Network, Protocol, BLS_PUB_LEN, PAYLOAD_HASH_LEN,
+    SECP_PUB_LEN,
 };
 use std::str::FromStr;
 
 #[test]
 fn bytes() {
-    let data = &[0, 3, 2, 2, 4, 3, 2, 1, 3, 2, 1, 1, 3, 5, 7, 2, 4, 2, 1, 4];
-    let new_addr = Address::new_secp256k1(data);
+    let data = [0; SECP_PUB_LEN];
+    let new_addr = Address::new_secp256k1(&data).unwrap();
     let encoded_bz = new_addr.to_bytes();
 
     // Assert decoded address equals the original address and a new one with the same data
     let decoded_addr = Address::from_bytes(&encoded_bz).unwrap();
     assert!(decoded_addr == new_addr);
-    assert!(decoded_addr == Address::new_secp256k1(data));
+    assert!(decoded_addr == Address::new_secp256k1(&data).unwrap());
 
     // Assert different types don't match
-    assert!(decoded_addr != Address::new_actor(data));
+    assert!(decoded_addr != Address::new_actor(&data));
+}
+
+#[test]
+fn key_len_validations() {
+    // Short
+    assert!(Address::new_bls(&[8; BLS_PUB_LEN - 1]).is_err());
+    assert!(Address::new_secp256k1(&[8; SECP_PUB_LEN - 1]).is_err());
+
+    // Equal
+    assert!(Address::new_bls(&[8; BLS_PUB_LEN]).is_ok());
+    assert!(Address::new_secp256k1(&[8; SECP_PUB_LEN]).is_ok());
+
+    // Long
+    assert!(Address::new_bls(&[8; BLS_PUB_LEN + 1]).is_err());
+    assert!(Address::new_secp256k1(&[8; SECP_PUB_LEN + 1]).is_err());
 }
 
 #[test]
@@ -114,7 +130,7 @@ fn secp256k1_address() {
     ];
 
     for t in test_vectors.iter() {
-        let addr = Address::new_secp256k1(t.input);
+        let addr = Address::new_secp256k1(t.input).unwrap();
         test_address(addr, Protocol::Secp256k1, t.expected);
     }
 }

diff --git a/vm/message/Cargo.toml b/vm/message/Cargo.toml
@@ -2,18 +2,18 @@
 name = "forest_message"
 description = "Filecoin message types"
 license = "MIT OR Apache-2.0"
-version = "0.1.0"
+version = "0.2.0"
 authors = ["ChainSafe Systems <info@chainsafe.io>"]
 edition = "2018"
 repository = "https://github.com/ChainSafe/forest"
 
 [dependencies]
-vm = { package = "forest_vm", path = "../../vm", version = "0.1" }
-address = { package = "forest_address", path = "../address", version = "0.1" }
+vm = { package = "forest_vm", path = "../../vm", version = "0.2" }
+address = { package = "forest_address", path = "../address", version = "0.2" }
 cid = { package = "forest_cid", path = "../../ipld/cid", version = "0.1" }
 num-bigint = { path = "../../utils/bigint", package = "forest_bigint", version = "0.1" }
 encoding = { package = "forest_encoding", path = "../../encoding", version = "0.1" }
-crypto = { package = "forest_crypto", path = "../../crypto", version = "0.1" }
+crypto = { package = "forest_crypto", path = "../../crypto", version = "0.2" }
 derive_builder = "0.9"
 serde = { version = "1.0", features = ["derive"] }
 

diff --git a/vm/state_tree/tests/state_tree_tests.rs b/vm/state_tree/tests/state_tree_tests.rs
@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0, MIT
 
 use actor::{init, ActorState, INIT_ACTOR_ADDR};
-use address::Address;
+use address::{Address, SECP_PUB_LEN};
 use cid::{multihash::Identity, Cid};
 use ipld_blockstore::BlockStore;
 use ipld_hamt::Hamt;
@@ -87,7 +87,7 @@ fn get_set_non_id() {
     );
 
     // Register new address
-    let addr = Address::new_secp256k1(&[0, 2]);
+    let addr = Address::new_secp256k1(&[2; SECP_PUB_LEN]).unwrap();
     let secp_state = ActorState::new(e_cid.clone(), e_cid.clone(), Default::default(), 0);
     let assigned_addr = tree
         .register_new_address(&addr, secp_state.clone())