From cbaf2906e812db1c748c93fc21166acc3218ca5e Mon Sep 17 00:00:00 2001 From: Zac Shenker Date: Mon, 19 Oct 2026 21:40:24 -0600 Subject: [PATCH] Harden COSE decoding: trailing bytes, crit, and integer ranges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reject bytes after the COSE structure, and after the encoded map inside the protected header and payload byte strings. Appended data is not covered by the signature and was silently ignored. - Enforce the crit header parameter (RFC 9052 §3.1): it must be a non-empty array of labels in the protected header, and each label must be a parameter this library processes (alg, kid). Anything else fails with the new Error::UnsupportedCriticalHeader, as §3.1 requires for critical parameters the recipient does not understand. - Convert alg with i32::try_from instead of `as i32`, so an out-of-range value like 2^32 + 5 no longer aliases HMAC 256/256 (5). Apply the same fix to the catreplay value, where 2^32 aliased 0 (replay permitted). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 11 +++ src/constants.rs | 2 + src/error.rs | 5 ++ src/header.rs | 7 +- src/token.rs | 223 ++++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 244 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e5cfd4..6112110 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 message — so without this a third party could derive a second valid signature for an unchanged token. Requiring low-S makes the signature bytes a stable identity for a token. +- `Token::from_bytes` hardens COSE structure decoding (RFC 9052): + - Bytes after the COSE structure, or after the map inside the protected + header or payload byte strings, are rejected. Appended data isn't covered + by the signature and was silently ignored. + - A `crit` header parameter (§3.1) is enforced. It must be a non-empty array + in the protected header, and every listed label must be one this library + processes (`alg`, `kid`); otherwise decoding fails with the new + `Error::UnsupportedCriticalHeader`. + - An out-of-range `alg` no longer truncates onto a valid identifier (for + example `2^32 + 5` was read as HMAC 256/256). The same fix applies to the + `catreplay` value, where `2^32` was read as "replay permitted". - PS256 rejects RSA keys whose modulus is smaller than 2048 bits, on both the signing and verification paths. Smaller moduli (e.g. 512- or 1024-bit) are too weak to be secure — a 512-bit modulus is factorable on commodity hardware — diff --git a/src/constants.rs b/src/constants.rs index 4ea0529..06cc4a5 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -216,6 +216,8 @@ pub mod cwt_keys { pub mod cose_labels { /// Algorithm (used in protected header) pub const ALG: i32 = 1; + /// Critical header parameters (protected header only; RFC 9052 §3.1) + pub const CRIT: i32 = 2; /// Key identifier (used in protected or unprotected header) pub const KID: i32 = 4; } diff --git a/src/error.rs b/src/error.rs index 637babd..705b517 100644 --- a/src/error.rs +++ b/src/error.rs @@ -31,6 +31,11 @@ pub enum Error { #[error("Invalid algorithm: {0}")] InvalidAlgorithm(String), + /// A critical header parameter (COSE `crit`, RFC 9052 §3.1) is malformed + /// or not understood by this library + #[error("Unsupported critical header parameter: {0}")] + UnsupportedCriticalHeader(String), + /// Invalid cryptographic key #[error("Invalid key: {0}")] InvalidKey(String), diff --git a/src/header.rs b/src/header.rs index 64efef3..c76a47a 100644 --- a/src/header.rs +++ b/src/header.rs @@ -314,6 +314,8 @@ pub mod labels { /// Algorithm (used in protected header) pub const ALG: i32 = cose_labels::ALG; + /// Critical header parameters (protected header only) + pub const CRIT: i32 = cose_labels::CRIT; /// Key identifier (used in protected or unprotected header) pub const KID: i32 = cose_labels::KID; } @@ -457,7 +459,10 @@ impl Header { /// ``` pub fn algorithm(&self) -> Option { if let Some(CborValue::Integer(alg)) = self.protected.get(&labels::ALG) { - Algorithm::from_identifier(*alg as i32) + // Out-of-range values must not truncate onto a valid identifier + i32::try_from(*alg) + .ok() + .and_then(Algorithm::from_identifier) } else { None } diff --git a/src/token.rs b/src/token.rs index 21ebadf..7e3d08a 100644 --- a/src/token.rs +++ b/src/token.rs @@ -3,7 +3,9 @@ use crate::claims::{Claims, ClaimsMap, RegisteredClaims}; use crate::constants::tprint_params; use crate::error::Error; -use crate::header::{Algorithm, AlgorithmClass, CborValue, Header, HeaderMap, KeyId, VerifyingKey}; +use crate::header::{ + labels, Algorithm, AlgorithmClass, CborValue, Header, HeaderMap, KeyId, VerifyingKey, +}; use crate::utils::{ compute_es256, compute_hmac_sha256, compute_ps256, current_timestamp, verify_es256, verify_hmac_sha256, verify_ps256, @@ -151,6 +153,7 @@ impl Token { protected, unprotected, }; + check_critical_headers(&header)?; // 3. Payload let claims_bytes = dec.bytes()?; @@ -164,6 +167,15 @@ impl Token { // 4. Signature let signature = dec.bytes()?.to_vec(); + // Nothing may follow the COSE structure: appended bytes are not + // covered by the signature and would otherwise be silently ignored. + if dec.position() != bytes.len() { + return Err(Error::InvalidFormat(format!( + "{} trailing bytes after the COSE structure", + bytes.len() - dec.position() + ))); + } + Ok(Self { header, claims, @@ -655,7 +667,9 @@ impl Token { // Get the replay protection value let replay_value = match catreplay_claim { - CborValue::Integer(value) => *value as i32, + CborValue::Integer(value) => i32::try_from(*value).map_err(|_| { + Error::InvalidClaimValue(format!("Invalid CATREPLAY value: {value}")) + })?, _ => { return Err(Error::InvalidClaimValue( "CATREPLAY claim is not an integer".to_string(), @@ -1647,9 +1661,63 @@ fn encode_map_direct(map: &HeaderMap, enc: &mut Encoder<&mut Vec>) -> Result Ok(()) } +/// Decode a map that must fill `bytes` exactly (a bstr-wrapped protected +/// header or payload); trailing data inside the bstr is rejected. fn decode_map(bytes: &[u8]) -> Result { let mut dec = Decoder::new(bytes); - decode_map_direct(&mut dec) + let map = decode_map_direct(&mut dec)?; + if dec.position() != bytes.len() { + return Err(Error::InvalidFormat(format!( + "{} trailing bytes after encoded map", + bytes.len() - dec.position() + ))); + } + Ok(map) +} + +/// Header parameters this library processes, and so may be marked critical. +const UNDERSTOOD_HEADER_PARAMETERS: [i32; 2] = [labels::ALG, labels::KID]; + +/// Enforce COSE `crit` (RFC 9052 §3.1): it may only appear in the protected +/// header, must be a non-empty array of labels, and every label it lists must +/// be one this library understands. Unknown critical parameters are rejected. +fn check_critical_headers(header: &Header) -> Result<(), Error> { + if header.unprotected.contains_key(&labels::CRIT) { + return Err(Error::UnsupportedCriticalHeader( + "crit must be in the protected header".to_string(), + )); + } + let crit = match header.protected.get(&labels::CRIT) { + None => return Ok(()), + Some(CborValue::Array(crit)) if !crit.is_empty() => crit, + Some(_) => { + return Err(Error::UnsupportedCriticalHeader( + "crit must be a non-empty array of labels".to_string(), + )) + } + }; + for label in crit { + match label { + CborValue::Integer(l) + if i32::try_from(*l).is_ok_and(|l| UNDERSTOOD_HEADER_PARAMETERS.contains(&l)) => {} + CborValue::Integer(l) => { + return Err(Error::UnsupportedCriticalHeader(format!( + "header parameter {l} is not understood" + ))) + } + CborValue::Text(l) => { + return Err(Error::UnsupportedCriticalHeader(format!( + "header parameter \"{l}\" is not understood" + ))) + } + _ => { + return Err(Error::UnsupportedCriticalHeader( + "crit labels must be integers or text".to_string(), + )) + } + } + } + Ok(()) } /// Decode a CBOR array @@ -1778,3 +1846,152 @@ fn decode_map_direct(dec: &mut Decoder<'_>) -> Result { Ok(map) } + +#[cfg(test)] +mod hardening_tests { + use super::*; + use crate::utils::compute_hmac_sha256; + + const KEY: &[u8] = b"cose-hardening-key"; + + /// Build a tagged COSE_Mac0 token, MACed with `KEY`, from raw parts. + fn mac0(protected: &HeaderMap, unprotected: &HeaderMap, payload: &[u8]) -> Vec { + let protected = encode_map(protected).unwrap(); + let mut input = Vec::new(); + let mut enc = Encoder::new(&mut input); + enc.array(4).unwrap(); + enc.str("MAC0").unwrap(); + enc.bytes(&protected).unwrap(); + enc.bytes(&[]).unwrap(); + enc.bytes(payload).unwrap(); + let mac = compute_hmac_sha256(KEY, &input); + + let mut out = Vec::new(); + let mut enc = Encoder::new(&mut out); + enc.tag(minicbor::data::Tag::new(61)).unwrap(); + enc.tag(minicbor::data::Tag::new(17)).unwrap(); + enc.array(4).unwrap(); + enc.bytes(&protected).unwrap(); + encode_map_direct(unprotected, &mut enc).unwrap(); + enc.bytes(payload).unwrap(); + enc.bytes(&mac).unwrap(); + out + } + + fn hs256_header() -> HeaderMap { + HeaderMap::from([(labels::ALG, CborValue::Integer(5))]) + } + + fn empty_payload() -> Vec { + encode_map(&HeaderMap::new()).unwrap() + } + + fn verify(bytes: &[u8]) -> Result<(), Error> { + Token::from_bytes(bytes)?.verify_with_key(VerifyingKey::HmacSha256(KEY)) + } + + #[test] + fn test_well_formed_token_verifies() { + assert!(verify(&mac0(&hs256_header(), &HeaderMap::new(), &empty_payload())).is_ok()); + } + + #[test] + fn test_trailing_bytes_after_structure_are_rejected() { + let mut bytes = mac0(&hs256_header(), &HeaderMap::new(), &empty_payload()); + bytes.extend_from_slice(b"junk"); + assert!(matches!( + Token::from_bytes(&bytes), + Err(Error::InvalidFormat(_)) + )); + } + + #[test] + fn test_trailing_bytes_inside_payload_are_rejected() { + let mut payload = empty_payload(); + payload.push(0x00); + let bytes = mac0(&hs256_header(), &HeaderMap::new(), &payload); + assert!(matches!( + Token::from_bytes(&bytes), + Err(Error::InvalidFormat(_)) + )); + } + + #[test] + fn test_out_of_range_alg_does_not_alias_hmac() { + // 2^32 + 5 truncates to 5 (HMAC 256/256) under an `as i32` cast + let protected = HeaderMap::from([(labels::ALG, CborValue::Integer((1 << 32) + 5))]); + let bytes = mac0(&protected, &HeaderMap::new(), &empty_payload()); + let token = Token::from_bytes(&bytes).unwrap(); + assert_eq!(token.header.algorithm(), None); + assert!(verify(&bytes).is_err()); + } + + #[test] + fn test_understood_critical_parameters_are_accepted() { + let mut protected = hs256_header(); + protected.insert( + labels::CRIT, + CborValue::Array(vec![CborValue::Integer(labels::ALG as i64)]), + ); + assert!(verify(&mac0(&protected, &HeaderMap::new(), &empty_payload())).is_ok()); + } + + #[test] + fn test_unknown_critical_parameter_is_rejected() { + for label in [CborValue::Integer(99), CborValue::Text("ext".into())] { + let mut protected = hs256_header(); + protected.insert(99, CborValue::Integer(1)); + protected.insert(labels::CRIT, CborValue::Array(vec![label])); + let bytes = mac0(&protected, &HeaderMap::new(), &empty_payload()); + assert!(matches!( + Token::from_bytes(&bytes), + Err(Error::UnsupportedCriticalHeader(_)) + )); + } + } + + #[test] + fn test_malformed_or_unprotected_crit_is_rejected() { + let malformed = [ + CborValue::Array(vec![]), + CborValue::Integer(1), + CborValue::Array(vec![CborValue::Null]), + ]; + for crit in malformed { + let mut protected = hs256_header(); + protected.insert(labels::CRIT, crit); + let bytes = mac0(&protected, &HeaderMap::new(), &empty_payload()); + assert!(matches!( + Token::from_bytes(&bytes), + Err(Error::UnsupportedCriticalHeader(_)) + )); + } + + let unprotected = HeaderMap::from([( + labels::CRIT, + CborValue::Array(vec![CborValue::Integer(labels::ALG as i64)]), + )]); + let bytes = mac0(&hs256_header(), &unprotected, &empty_payload()); + assert!(matches!( + Token::from_bytes(&bytes), + Err(Error::UnsupportedCriticalHeader(_)) + )); + } + + #[test] + fn test_out_of_range_catreplay_does_not_alias_permitted() { + // 2^32 truncates to 0 (replay permitted) under an `as i32` cast + let token = TokenBuilder::new() + .algorithm(Algorithm::HmacSha256) + .custom_int(crate::constants::cat_keys::CATREPLAY, 1 << 32) + .sign(KEY) + .unwrap(); + let options = VerificationOptions::new() + .verify_catreplay(true) + .token_seen_before(true); + assert!(matches!( + token.verify_claims(&options), + Err(Error::InvalidClaimValue(_)) + )); + } +}