Safer string prefix checking (#299)

* Add negative tests for bad string slicing * Subslice strings more safely
spruceid · Sep 18, 2021 · 400effa · 400effa
1 parent c669722
commit 400effa
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 17 deletions.
diff --git a/did-pkh/src/lib.rs b/did-pkh/src/lib.rs
@@ -51,10 +51,10 @@ async fn resolve_tz(did: &str, account_address: String) -> ResolutionResult {
     if account_address.len() < 3 {
         return resolution_error(&ERROR_INVALID_DID);
     }
-    let (vm_type, vm_type_iri) = match &account_address[0..3] {
-        "tz1" => ("Ed25519PublicKeyBLAKE2BDigestSize20Base58CheckEncoded2021", "https://w3id.org/security#Ed25519PublicKeyBLAKE2BDigestSize20Base58CheckEncoded2021"),
-        "tz2" => ("EcdsaSecp256k1RecoveryMethod2020", "https://identity.foundation/EcdsaSecp256k1RecoverySignature2020#EcdsaSecp256k1RecoveryMethod2020"),
-        "tz3" => ("P256PublicKeyBLAKE2BDigestSize20Base58CheckEncoded2021", "https://w3id.org/security#P256PublicKeyBLAKE2BDigestSize20Base58CheckEncoded2021"),
+    let (vm_type, vm_type_iri) = match account_address.get(0..3) {
+        Some("tz1") => ("Ed25519PublicKeyBLAKE2BDigestSize20Base58CheckEncoded2021", "https://w3id.org/security#Ed25519PublicKeyBLAKE2BDigestSize20Base58CheckEncoded2021"),
+        Some("tz2") => ("EcdsaSecp256k1RecoveryMethod2020", "https://identity.foundation/EcdsaSecp256k1RecoverySignature2020#EcdsaSecp256k1RecoveryMethod2020"),
+        Some("tz3") => ("P256PublicKeyBLAKE2BDigestSize20Base58CheckEncoded2021", "https://w3id.org/security#P256PublicKeyBLAKE2BDigestSize20Base58CheckEncoded2021"),
         _ => return resolution_error(&ERROR_INVALID_DID),
     };
     let blockchain_account_id = BlockchainAccountId {
@@ -642,6 +642,17 @@ mod tests {
         );
     }
 
+    #[tokio::test]
+    async fn test_glyph_split() {
+        // Subslicing this expected Tezos address by byte range 0..3 would break a char boundary.
+        // https://doc.rust-lang.org/std/ops/struct.Range.html#impl-SliceIndex%3Cstr%3E
+        let bad_did = "did:pkh:tz:💣️";
+        let (res_meta, _doc_opt, _meta_opt) = DIDPKH
+            .resolve(bad_did, &ResolutionInputMetadata::default())
+            .await;
+        assert_ne!(res_meta.error, None);
+    }
+
     async fn test_resolve_error(did: &str, error_expected: &str) {
         let (res_meta, doc_opt, _meta_opt) = DIDPKH
             .resolve(did, &ResolutionInputMetadata::default())

diff --git a/did-tezos/src/lib.rs b/did-tezos/src/lib.rs
@@ -81,7 +81,16 @@ impl DIDResolver for DIDTz {
             }
         };
 
-        let prefix = &address[0..3];
+        let prefix = match address.get(0..3) {
+            Some(prefix) => prefix,
+            None => {
+                return (
+                    ResolutionMetadata::from_error(&ERROR_INVALID_DID),
+                    None,
+                    None,
+                )
+            }
+        };
         let (_curve, proof_type, proof_type_iri) = match prefix_to_curve_type(prefix) {
             Some(addr) => addr,
             None => {
@@ -571,6 +580,17 @@ mod tests {
         assert_eq!(did, "did:tz:tz3agP9LGe2cXmKQyYn6T68BHKjjktDbbSWX");
     }
 
+    #[tokio::test]
+    async fn test_glyph_split() {
+        // Subslicing this method-specific id by byte range 0..3 would break a char boundary.
+        // https://doc.rust-lang.org/std/ops/struct.Range.html#impl-SliceIndex%3Cstr%3E
+        let bad_did = "did:tz:💣️00000000000000000000000000000";
+        let (res_meta, _doc_opt, _meta_opt) = DIDTZ
+            .resolve(bad_did, &ResolutionInputMetadata::default())
+            .await;
+        assert_ne!(res_meta.error, None);
+    }
+
     #[tokio::test]
     async fn test_derivation_tz1() {
         let (res_meta, doc_opt, _meta_opt) = DIDTZ

diff --git a/src/jsonld.rs b/src/jsonld.rs
@@ -340,7 +340,7 @@ pub fn is_blank_node_identifier(value: &JsonValue) -> bool {
         Some(value_str) => value_str,
         None => return false,
     };
-    value_str.get(..2) == Some("_:")
+    value_str.starts_with("_:")
 }
 
 #[derive(Debug, Clone, Default)]

diff --git a/src/tzkey.rs b/src/tzkey.rs
@@ -59,8 +59,8 @@ pub fn jwk_from_tezos_key(tz_pk: &str) -> Result<JWK, Error> {
     if tz_pk.len() < 4 {
         return Err(Error::KeyPrefix);
     }
-    let (alg, params) = match &tz_pk[..4] {
-        "edpk" => (
+    let (alg, params) = match tz_pk.get(..4) {
+        Some("edpk") => (
             Algorithm::EdBlake2b,
             Params::OKP(OctetParams {
                 curve: "Ed25519".into(),
@@ -70,7 +70,7 @@ pub fn jwk_from_tezos_key(tz_pk: &str) -> Result<JWK, Error> {
                 private_key: None,
             }),
         ),
-        "edsk" => {
+        Some("edsk") => {
             let sk_bytes = bs58::decode(&tz_pk).with_check(None).into_vec()?[4..].to_owned();
             let pk_bytes;
             #[cfg(feature = "ring")]
@@ -107,14 +107,14 @@ pub fn jwk_from_tezos_key(tz_pk: &str) -> Result<JWK, Error> {
             )
         }
         #[cfg(feature = "secp256k1")]
-        "sppk" => {
+        Some("sppk") => {
             let pk_bytes = bs58::decode(&tz_pk).with_check(None).into_vec()?[4..].to_owned();
             let jwk =
                 crate::jwk::secp256k1_parse(&pk_bytes).map_err(|e| Error::Secp256k1Parse(e))?;
             (Algorithm::ESBlake2bK, jwk.params)
         }
         #[cfg(feature = "p256")]
-        "p2pk" => {
+        Some("p2pk") => {
             let pk_bytes = bs58::decode(&tz_pk).with_check(None).into_vec()?[4..].to_owned();
             let jwk = crate::jwk::p256_parse(&pk_bytes)?;
             (Algorithm::ESBlake2b, jwk.params)
@@ -200,13 +200,15 @@ pub fn decode_tzsig(sig_bs58: &str) -> Result<(Algorithm, Vec<u8>), DecodeTezosS
             sig_bs58.to_string(),
         ));
     }
-    let (algorithm, sig) = match &sig_bs58[0..5] {
-        "edsig" => (Algorithm::EdBlake2b, tzsig[5..].to_vec()),
-        "spsig" => (Algorithm::ESBlake2bK, tzsig[5..].to_vec()),
-        "p2sig" => (Algorithm::ESBlake2b, tzsig[4..].to_vec()),
-        prefix => {
+    // sig_bs58 has been checked as base58. But use the non-panicking get function anyway, for good
+    // measure.
+    let (algorithm, sig) = match sig_bs58.get(0..5) {
+        Some("edsig") => (Algorithm::EdBlake2b, tzsig[5..].to_vec()),
+        Some("spsig") => (Algorithm::ESBlake2bK, tzsig[5..].to_vec()),
+        Some("p2sig") => (Algorithm::ESBlake2b, tzsig[4..].to_vec()),
+        _ => {
             return Err(DecodeTezosSignatureError::SignaturePrefix(
-                prefix.to_string(),
+                sig_bs58.to_string(),
             ))
         }
     };
@@ -222,6 +224,14 @@ mod tests {
     use crate::blakesig::hash_public_key;
     use serde_json::json;
 
+    #[test]
+    fn test_jwk_from_tezos_key_glyph_split() {
+        // Attempt to decode tzsig that would involve subslicing
+        // through a char boundary.
+        let bad_tzk = "xx💣️";
+        jwk_from_tezos_key(&bad_tzk).unwrap_err();
+    }
+
     #[test]
     fn edpk_jwk_tz_edsig() {
         let tzpk = "edpkuxZ5AQVCeEJ9inUG3w6VFhio5KBwC22ekPLBzcvub3QY2DvJ7n";