diff --git a/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs b/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs index ddb72c9a71f..cee28c91353 100644 --- a/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs +++ b/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs @@ -3,6 +3,7 @@ use crate::consensus::state::data_contract::document_type_update_error::Document use crate::data_contract::document_type::accessors::{ DocumentTypeV0Getters, DocumentTypeV2Getters, }; +use crate::data_contract::document_type::property::{ByteArrayPropertySizes, DocumentPropertyType}; use crate::data_contract::document_type::schema::validate_schema_compatibility; use crate::data_contract::document_type::DocumentTypeRef; use crate::data_contract::errors::DataContractError; @@ -33,10 +34,84 @@ impl DocumentTypeRef<'_> { return Ok(result); } + // Validate that no byte array property changes its on-disk encoding + let result = self.validate_byte_array_encoding_stability(new_document_type); + + if !result.is_valid() { + return Ok(result); + } + // Validate schema compatibility self.validate_schema(new_document_type, platform_version) } + /// A byte array property whose `minItems == maxItems` is serialized as raw, + /// fixed-length bytes with no length prefix; any other size bounds make it + /// serialized with a variable-length (varint) length prefix. Crossing that + /// boundary -- or changing the fixed length itself -- silently changes the + /// on-disk layout of every already-stored document, so re-decoding old bytes + /// against the new type misreads them. JSON-schema compatibility treats + /// widening/removing `maxItems` as compatible, so this layout invariant must + /// be enforced separately. Runs before `validate_schema` so it cannot be + /// bypassed by a JSON-schema-compatible widening. + fn validate_byte_array_encoding_stability( + &self, + new_document_type: DocumentTypeRef, + ) -> SimpleConsensusValidationResult { + // Mirror the encoder/decoder exactly (see `encode_value_ref_with_size`): + // the raw, no-length-prefix path is used ONLY when BOTH bounds are present + // and equal. Any other shape -- including an omitted `minItems` (`None`) -- + // is varint length-prefixed, so an implicit `minItems: 0` must NOT be + // treated as fixed-length here or this guard would diverge from the actual + // on-disk layout. `Some(n)` => fixed raw encoding of length `n`; `None` => + // variable (varint length-prefixed) encoding. + fn fixed_length(sizes: &ByteArrayPropertySizes) -> Option { + match (sizes.min_size, sizes.max_size) { + (Some(min), Some(max)) if min == max => Some(min), + _ => None, + } + } + + let new_properties = new_document_type.flattened_properties(); + + for (path, old_property) in self.flattened_properties() { + let DocumentPropertyType::ByteArray(old_sizes) = &old_property.property_type else { + continue; + }; + + let Some(new_property) = new_properties.get(path) else { + continue; + }; + + let DocumentPropertyType::ByteArray(new_sizes) = &new_property.property_type else { + continue; + }; + + if fixed_length(old_sizes) != fixed_length(new_sizes) { + return SimpleConsensusValidationResult::new_with_error( + DocumentTypeUpdateError::new( + self.data_contract_id(), + self.name(), + format!( + "document type can not change the byte array encoding of property \ + '{}': changing its size bounds from (minItems: {:?}, maxItems: {:?}) \ + to (minItems: {:?}, maxItems: {:?}) alters the on-disk layout of \ + existing documents", + path, + old_sizes.min_size, + old_sizes.max_size, + new_sizes.min_size, + new_sizes.max_size, + ), + ) + .into(), + ); + } + } + + SimpleConsensusValidationResult::new() + } + fn validate_config( &self, new_document_type: DocumentTypeRef, @@ -1325,4 +1400,141 @@ mod tests { ); } } + + mod validate_byte_array_encoding { + use super::*; + use std::collections::BTreeMap; + + fn document_type_with_byte_array( + byte_array: platform_value::Value, + platform_version: &PlatformVersion, + ) -> DocumentType { + let schema = platform_value!({ + "type": "object", + "properties": { "blob": byte_array }, + "additionalProperties": false, + }); + let config = DataContractConfig::default_for_version(platform_version) + .expect("should create a default config"); + DocumentType::try_from_schema( + Identifier::random(), + 1, + config.version(), + "test", + schema, + None, + &BTreeMap::new(), + &config, + false, + &mut Vec::new(), + platform_version, + ) + .expect("failed to create document type") + } + + // Exercises the PUBLIC `validate_update` dispatcher (latest protocol + // version), so it also covers the dispatch into v0. + fn validate_update_latest( + old_ba: platform_value::Value, + new_ba: platform_value::Value, + ) -> SimpleConsensusValidationResult { + let platform_version = PlatformVersion::latest(); + let old = document_type_with_byte_array(old_ba, platform_version); + let new = document_type_with_byte_array(new_ba, platform_version); + old.as_ref() + .validate_update(new.as_ref(), platform_version) + .expect("validate_update should not error") + } + + fn assert_rejected(old_ba: platform_value::Value, new_ba: platform_value::Value) { + let result = validate_update_latest(old_ba, new_ba); + assert_matches!( + result.errors.as_slice(), + [ConsensusError::StateError(StateError::DocumentTypeUpdateError(e))] + if e.additional_message().contains("byte array encoding") + ); + } + + fn assert_accepted(old_ba: platform_value::Value, new_ba: platform_value::Value) { + let result = validate_update_latest(old_ba, new_ba); + assert!( + result.is_valid(), + "expected the update to be accepted, got {:?}", + result.errors + ); + } + + #[test] + fn rejects_widening_fixed_byte_array_max_items() { + // The exact attack: a fixed (raw, no length prefix) 32-byte field + // widened to min 32 / max 64 flips it to the varint length-prefixed + // encoding, making every already-stored document undecodable. + assert_rejected( + platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":32,"position":0}), + platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":64,"position":0}), + ); + } + + #[test] + fn rejects_removing_max_items_from_fixed_byte_array() { + // Removing `maxItems` turns a fixed (raw, no length prefix) byte array + // into a variable (varint length-prefixed) one, so it must be rejected. + assert_rejected( + platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":32,"position":0}), + platform_value!({"type":"array","byteArray":true,"minItems":32,"position":0}), + ); + } + + #[test] + fn rejects_changing_fixed_byte_array_size() { + // The byte-array check runs before validate_schema, so a fixed-size + // change is caught here as an encoding change. + assert_rejected( + platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":32,"position":0}), + platform_value!({"type":"array","byteArray":true,"minItems":64,"maxItems":64,"position":0}), + ); + } + + #[test] + fn rejects_tightening_variable_to_fixed_byte_array() { + // The reverse flip: a variable (varint length-prefixed) byte array + // narrowed to fixed (raw) also changes the on-disk layout -- old docs + // carry a length prefix the new fixed type would misread. + assert_rejected( + platform_value!({"type":"array","byteArray":true,"minItems":1,"maxItems":32,"position":0}), + platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":32,"position":0}), + ); + } + + #[test] + fn accepts_unchanged_fixed_byte_array() { + assert_accepted( + platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":32,"position":0}), + platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":32,"position":0}), + ); + } + + #[test] + fn accepts_widening_already_variable_byte_array() { + // Variable-length on both sides: the on-disk encoding does not change, + // so widening the bound stays allowed. + assert_accepted( + platform_value!({"type":"array","byteArray":true,"minItems":1,"maxItems":32,"position":0}), + platform_value!({"type":"array","byteArray":true,"minItems":1,"maxItems":64,"position":0}), + ); + } + + #[test] + fn accepts_max_items_change_when_min_items_is_omitted() { + // With `minItems` omitted (None) the encoder always uses the variable + // (varint length-prefixed) path regardless of `maxItems` -- the raw + // path requires BOTH bounds present and equal. So changing `maxItems` + // does not change the on-disk encoding and must stay allowed. An + // implicit `minItems: 0` is NOT fixed-length (mirrors the encoder). + assert_accepted( + platform_value!({"type":"array","byteArray":true,"maxItems":0,"position":0}), + platform_value!({"type":"array","byteArray":true,"maxItems":1,"position":0}), + ); + } + } } diff --git a/packages/rs-dpp/src/data_contract/document_type/property/byte_array_encoding_flip_tests.rs b/packages/rs-dpp/src/data_contract/document_type/property/byte_array_encoding_flip_tests.rs new file mode 100644 index 00000000000..832ba61c864 --- /dev/null +++ b/packages/rs-dpp/src/data_contract/document_type/property/byte_array_encoding_flip_tests.rs @@ -0,0 +1,299 @@ +//! Security regression tests for the byteArray on-disk encoding-flip chain-halt +//! vulnerability (Dash Platform v4.0.0-rc.1). +//! +//! BACKGROUND +//! ---------- +//! A `byteArray` document property is serialized differently depending on whether +//! its `minItems == maxItems`: +//! +//! * `min == max` -> stored as RAW bytes, with NO length prefix. The decoder +//! (`DocumentPropertyType::read_optionally_from`) reads exactly `min` bytes. +//! * `min != max` (or `maxItems` removed) -> stored as a VARINT length prefix +//! followed by the bytes. The decoder calls `read_varint_value`, which reads +//! a varint length then `read_exact`s that many bytes. +//! +//! A `DataContractUpdate` that *widens* `maxItems` (e.g. 32 -> 64) is accepted by +//! schema-compatibility validation, but it FLIPS the on-disk encoding of the +//! property. Documents that were already stored under the old fixed (raw) +//! encoding then become un-decodable under the new variable (varint-prefixed) +//! type: the first stored data byte is reinterpreted as a varint length. If that +//! byte is a varint continuation byte (>= 0x80, e.g. 0xFF) the decoded length is +//! enormous, `read_exact` overruns the buffer, and decoding returns +//! `DataContractError::CorruptedSerialization` => `Err`. +//! +//! These tests PROVE the bug exists today. They are written to PASS by asserting +//! that the failure (`Err`) happens. After the bug is fixed, the asserted `Err` +//! should become `Ok`, so the assertions can be flipped to lock in the fix as a +//! regression test. + +use super::*; +use crate::data_contract::accessors::v0::DataContractV0Getters; +use crate::data_contract::config::DataContractConfig; +use crate::data_contract::serialized_version::v0::DataContractInSerializationFormatV0; +use crate::data_contract::v0::DataContractV0; +use crate::data_contract::DataContract; +use crate::document::serialization_traits::DocumentPlatformConversionMethodsV0; +use crate::document::{Document, DocumentV0}; +use platform_value::{platform_value, Identifier, Value}; +use platform_version::version::PlatformVersion; +use std::collections::BTreeMap; +use std::io::BufReader; + +const BYTE_ARRAY_FIELD: &str = "data"; + +/// Builds a single-document-type [`DataContract`] whose `data` property is a +/// `byteArray` with the given `minItems`/`maxItems`. When `max_items` is `None` +/// the `maxItems` keyword is omitted entirely (the "removal" variant of the +/// attack). +fn build_contract_with_byte_array( + min_items: u32, + max_items: Option, + platform_version: &PlatformVersion, +) -> DataContract { + let config = DataContractConfig::default_for_version(platform_version).expect("default config"); + + let mut byte_array_schema = platform_value!({ + "type": "array", + "byteArray": true, + "minItems": min_items, + "position": 0_u32, + }); + + if let Some(max) = max_items { + if let Value::Map(map) = &mut byte_array_schema { + map.push((Value::Text("maxItems".to_string()), Value::U32(max))); + } + } + + let document_schema = platform_value!({ + "type": "object", + "properties": { + BYTE_ARRAY_FIELD: byte_array_schema, + }, + "additionalProperties": false, + }); + + let serialization_format = DataContractInSerializationFormatV0 { + id: Identifier::new([7; 32]), + config, + version: 1, + owner_id: Identifier::new([8; 32]), + schema_defs: None, + document_schemas: BTreeMap::from([("doc".to_string(), document_schema)]), + }; + + DataContractV0::try_from_platform_versioned( + serialization_format.into(), + true, + &mut vec![], + platform_version, + ) + .expect("should build contract") + .into() +} + +/// Builds a document whose byteArray `data` field is a 32-byte value whose FIRST +/// byte is `0xFF` (a varint continuation byte). +fn build_document_with_ff_prefixed_bytes(_contract: &DataContract) -> Document { + let mut bytes = [0u8; 32]; + bytes[0] = 0xFF; + + let mut properties = BTreeMap::new(); + // A 32-byte byteArray is canonically represented (and re-loaded) as + // `Value::Bytes32` by the decoder, so we store it that way to keep the + // happy-path round-trip comparison exact. + properties.insert(BYTE_ARRAY_FIELD.to_string(), Value::Bytes32(bytes)); + + DocumentV0 { + id: Identifier::new([1; 32]), + owner_id: Identifier::new([2; 32]), + properties, + revision: Some(1), + created_at: None, + updated_at: None, + transferred_at: None, + created_at_block_height: None, + updated_at_block_height: None, + transferred_at_block_height: None, + created_at_core_block_height: None, + updated_at_core_block_height: None, + transferred_at_core_block_height: None, + creator_id: None, + } + .into() +} + +// ---------------------------------------------------------------------------- +// TEST 1: byteArray encoding flip makes old bytes undecodable +// ---------------------------------------------------------------------------- + +/// PROPERTY-LEVEL PROOF. +/// +/// Encode a 32-byte value (first byte 0xFF) under a fixed `ByteArray{32,32}` +/// type, then attempt to decode it under a widened `ByteArray{32,64}` type. +/// +/// * Under the FIXED type the bytes are raw (no length prefix). +/// * Under the WIDENED type the decoder treats the first byte (0xFF) as a varint +/// length, reads a huge length, and `read_exact` overruns -> `Err`. +#[test] +fn byte_array_encoding_flip_property_level_makes_old_bytes_undecodable() { + let fixed = DocumentPropertyType::ByteArray(ByteArrayPropertySizes { + min_size: Some(32), + max_size: Some(32), + }); + let widened = DocumentPropertyType::ByteArray(ByteArrayPropertySizes { + min_size: Some(32), + max_size: Some(64), + }); + + let mut value_bytes = [0u8; 32]; + value_bytes[0] = 0xFF; + let value = Value::Bytes(value_bytes.to_vec()); + + // Encoded under the fixed (raw, no length prefix) type. + let encoded = fixed + .encode_value_ref_with_size(&value, true) + .expect("encode under fixed type should succeed"); + + // The fixed encoding is exactly the raw 32 bytes (no prefix). + assert_eq!( + encoded.len(), + 32, + "fixed byteArray must be stored raw with no length prefix" + ); + assert_eq!(encoded[0], 0xFF, "first stored byte is the 0xFF data byte"); + + // HAPPY PATH: decoding under the SAME fixed type round-trips perfectly. + { + let mut buf = BufReader::new(encoded.as_slice()); + let (decoded, _finished) = fixed + .read_optionally_from(&mut buf, true) + .expect("decode under fixed type should succeed"); + let decoded = decoded.expect("value present"); + let decoded_bytes = decoded.into_binary_bytes().expect("decoded value is bytes"); + assert_eq!( + decoded_bytes, value_bytes, + "round-trip under the unchanged fixed type must preserve the bytes" + ); + } + + // BUG: decoding the SAME stored bytes under the WIDENED type fails today, + // because the 0xFF first byte is misread as a varint length. + let mut buf = BufReader::new(encoded.as_slice()); + let result = widened.read_optionally_from(&mut buf, true); + + assert!( + result.is_err(), + "VULNERABILITY: widening maxItems flips the encoding so old raw bytes \ + become undecodable; expected Err today but got Ok: {:?}. \ + If this assertion now fails because the result is Ok, the bug has been \ + fixed -- flip this assertion to lock in the fix.", + result + ); + + let err = result.unwrap_err(); + // It is a serialization/corruption-class error coming out of read_varint_value. + assert!( + matches!( + err, + DataContractError::CorruptedSerialization(_) + | DataContractError::DecodingContractError(_) + ), + "expected a corrupted/decoding serialization error, got: {:?}", + err + ); +} + +/// DOCUMENT-LEVEL PROOF (the form closest to the on-chain path). +/// +/// Serialize a full [`Document`] against a contract whose `data` byteArray is +/// fixed `{minItems:32, maxItems:32}`, then call `Document::from_bytes` using a +/// contract whose `data` byteArray was widened to `{minItems:32, maxItems:64}`. +/// This mirrors exactly what happens on chain after a malicious +/// `DataContractUpdate`: the stored (old-encoding) document bytes are decoded +/// against the CURRENT (widened) document type. +#[test] +fn byte_array_encoding_flip_document_level_makes_old_bytes_undecodable() { + let platform_version = PlatformVersion::latest(); + + let fixed_contract = build_contract_with_byte_array(32, Some(32), platform_version); + let widened_contract = build_contract_with_byte_array(32, Some(64), platform_version); + + let fixed_type = fixed_contract + .document_type_for_name("doc") + .expect("fixed doc type"); + let widened_type = widened_contract + .document_type_for_name("doc") + .expect("widened doc type"); + + let document = build_document_with_ff_prefixed_bytes(&fixed_contract); + + // Stored on chain under the OLD (fixed 32/32) encoding. + // Fully-qualified call to disambiguate from serde's `Serialize::serialize`. + let serialized = DocumentPlatformConversionMethodsV0::serialize( + &document, + fixed_type, + &fixed_contract, + platform_version, + ) + .expect("serialize under fixed type should succeed"); + + // HAPPY PATH: round-trip under the unchanged fixed type works. + let round_tripped = Document::from_bytes(&serialized, fixed_type, platform_version) + .expect("round-trip under fixed type should succeed"); + assert_eq!( + round_tripped, document, + "round-trip under the unchanged fixed type must preserve the document" + ); + + // BUG: decoding the OLD stored bytes against the WIDENED current type fails. + let result = Document::from_bytes(&serialized, widened_type, platform_version); + + assert!( + result.is_err(), + "VULNERABILITY: a committed DataContractUpdate that widens maxItems from \ + 32 to 64 makes previously-stored documents undecodable. Expected Err \ + today but got Ok. If this is now Ok, the bug is fixed -- flip the \ + assertion." + ); +} + +/// Control: widening from `{32,32}` to `{32,64}` is the cause. A document whose +/// first stored byte is a NON-continuation byte (< 0x80) may decode to a +/// *different* (wrong) value rather than erroring, which is itself silent data +/// corruption -- but the 0xFF case above is the one that produces the hard +/// `Err` that halts the chain. Here we additionally show that removing +/// `maxItems` entirely triggers the same hard failure. +#[test] +fn byte_array_encoding_flip_via_maxitems_removal_makes_old_bytes_undecodable() { + let platform_version = PlatformVersion::latest(); + + let fixed_contract = build_contract_with_byte_array(32, Some(32), platform_version); + // maxItems removed entirely -> variable-length (varint-prefixed) encoding. + let removed_contract = build_contract_with_byte_array(32, None, platform_version); + + let fixed_type = fixed_contract + .document_type_for_name("doc") + .expect("fixed doc type"); + let removed_type = removed_contract + .document_type_for_name("doc") + .expect("maxItems-removed doc type"); + + let document = build_document_with_ff_prefixed_bytes(&fixed_contract); + + let serialized = DocumentPlatformConversionMethodsV0::serialize( + &document, + fixed_type, + &fixed_contract, + platform_version, + ) + .expect("serialize under fixed type should succeed"); + + let result = Document::from_bytes(&serialized, removed_type, platform_version); + + assert!( + result.is_err(), + "VULNERABILITY: removing maxItems also flips the encoding and makes old \ + bytes undecodable. Expected Err today but got Ok." + ); +} diff --git a/packages/rs-dpp/src/data_contract/document_type/property/mod.rs b/packages/rs-dpp/src/data_contract/document_type/property/mod.rs index d5877eafa06..cf8401be797 100644 --- a/packages/rs-dpp/src/data_contract/document_type/property/mod.rs +++ b/packages/rs-dpp/src/data_contract/document_type/property/mod.rs @@ -26,6 +26,9 @@ use serde::Serialize; pub mod array; +#[cfg(test)] +mod byte_array_encoding_flip_tests; + // This struct will be changed in future to support more validation logic and serialization // It will become versioned and it will be introduced by a new document type version // @append_only diff --git a/packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/byte_array_widen_accepted_tests.rs b/packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/byte_array_widen_accepted_tests.rs new file mode 100644 index 00000000000..35684b19b4b --- /dev/null +++ b/packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/byte_array_widen_accepted_tests.rs @@ -0,0 +1,140 @@ +//! Security regression test documenting the validation gap that enables the +//! byteArray encoding-flip chain-halt (Dash Platform v4.0.0-rc.1). +//! +//! `DataContractUpdate` runs each document type's new schema through +//! [`validate_schema_compatibility`] (the same version-aware production entry +//! point exercised here). Widening a `byteArray` property's `maxItems` +//! (e.g. 32 -> 64), or removing `maxItems` entirely, is classified as a +//! BACKWARD-COMPATIBLE change and is therefore ACCEPTED. +//! +//! That acceptance is exactly what makes the encoding flip reachable on chain: +//! validation lets the update through, but the on-disk encoding of the property +//! flips (raw fixed-size -> varint-length-prefixed), which corrupts the decode +//! of already-stored documents (see `byte_array_encoding_flip_tests` in the +//! `property` module). +//! +//! This test PASSES today by asserting that the widening is reported as +//! compatible. It documents the gap; it is expected to keep passing even after +//! the decode-side fix, unless the fix is implemented by rejecting the update at +//! validation time (in which case these assertions should be updated). + +use super::validate_schema_compatibility; +use platform_version::version::PlatformVersion; +use serde_json::json; + +/// A fixed `byteArray` schema with `minItems == maxItems == 32`. +fn fixed_32_schema() -> serde_json::Value { + json!({ + "type": "object", + "properties": { + "data": { + "type": "array", + "byteArray": true, + "minItems": 32, + "maxItems": 32, + "position": 0 + } + }, + "additionalProperties": false + }) +} + +#[test] +fn update_validation_accepts_widening_max_items_32_to_64() { + let platform_version = PlatformVersion::latest(); + + let original = fixed_32_schema(); + let widened = json!({ + "type": "object", + "properties": { + "data": { + "type": "array", + "byteArray": true, + "minItems": 32, + "maxItems": 64, + "position": 0 + } + }, + "additionalProperties": false + }); + + let result = validate_schema_compatibility(&original, &widened, platform_version) + .expect("compatibility validation must not error"); + + assert!( + result.is_valid(), + "VULNERABILITY (validation gap): widening byteArray maxItems 32 -> 64 is \ + accepted as a compatible DataContractUpdate, even though it flips the \ + on-disk encoding and corrupts existing documents. Incompatibilities \ + reported: {:?}", + result.errors + ); + assert!( + result.errors.is_empty(), + "expected no incompatibilities for the maxItems widen, got: {:?}", + result.errors + ); +} + +#[test] +fn update_validation_accepts_removing_max_items() { + let platform_version = PlatformVersion::latest(); + + let original = fixed_32_schema(); + // maxItems removed entirely -> property becomes variable-length on disk. + let removed = json!({ + "type": "object", + "properties": { + "data": { + "type": "array", + "byteArray": true, + "minItems": 32, + "position": 0 + } + }, + "additionalProperties": false + }); + + let result = validate_schema_compatibility(&original, &removed, platform_version) + .expect("compatibility validation must not error"); + + assert!( + result.is_valid(), + "VULNERABILITY (validation gap): removing byteArray maxItems is accepted \ + as a compatible DataContractUpdate, even though it flips the on-disk \ + encoding. Incompatibilities reported: {:?}", + result.errors + ); +} + +/// Sanity control: SHRINKING maxItems (64 -> 32) is correctly rejected, proving +/// the validator is actually inspecting `maxItems` (so the acceptance of the +/// WIDENING above is a deliberate rule, not a path that ignores the keyword). +#[test] +fn update_validation_rejects_shrinking_max_items_control() { + let platform_version = PlatformVersion::latest(); + + let original = json!({ + "type": "object", + "properties": { + "data": { + "type": "array", + "byteArray": true, + "minItems": 32, + "maxItems": 64, + "position": 0 + } + }, + "additionalProperties": false + }); + let shrunk = fixed_32_schema(); + + let result = validate_schema_compatibility(&original, &shrunk, platform_version) + .expect("compatibility validation must not error"); + + assert!( + !result.is_valid(), + "shrinking maxItems 64 -> 32 should be rejected as incompatible; if this \ + is accepted the validator is not inspecting maxItems at all" + ); +} diff --git a/packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/mod.rs b/packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/mod.rs index 182f8082581..b55f51a3385 100644 --- a/packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/mod.rs +++ b/packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/mod.rs @@ -4,6 +4,9 @@ use platform_version::version::PlatformVersion; mod v0; +#[cfg(test)] +mod byte_array_widen_accepted_tests; + use crate::validation::SimpleValidationResult; #[derive(Debug, Clone)] diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/mod.rs index 6d430d375aa..c0cc7df3325 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/mod.rs @@ -3116,4 +3116,421 @@ pub(in crate::execution) mod tests { ); } } + + /// End-to-end regression test for the byteArray on-disk encoding-flip + /// chain-halt vulnerability (Dash Platform v4.0.0-rc.1). + /// + /// This drives the full per-block DAO vote-poll resolver path so that + /// `Platform::check_for_ended_vote_polls` returns `Err` today (pre-fix). On + /// chain that `Err` propagates through `run_dao_platform_events` -> + /// `run_block_proposal` (a per-block event handler with a bare `?`), so it is + /// NOT caught into a per-state-transition result -- it halts the chain on + /// every validator at the block where the contested vote poll ends. + mod byte_array_encoding_flip_chain_halt { + use super::*; + use crate::test::helpers::fast_forward_to_block::fast_forward_to_block; + use dpp::data_contract::document_type::schema::validate_schema_compatibility; + + const CUSTOM_CONTESTED_CONTRACT: &str = + "tests/supporting_files/contract/dpns/dpns-contract-contested-unique-index.json"; + // Identical to the contract above except `domain.preorderSalt` byteArray + // has its `maxItems` widened from 32 to 64 -- the malicious update. + const CUSTOM_CONTESTED_CONTRACT_WIDENED: &str = + "tests/supporting_files/contract/dpns/dpns-contract-contested-unique-index-byte-array-widened.json"; + + /// Builds one preorder + one domain (contender) document for `identity`, + /// with `preorderSalt` forced to `[0xFF; 32]` so that, after the + /// byteArray encoding flips to varint-length-prefixed, the first stored + /// byte (0xFF, a varint continuation byte) decodes to an enormous length + /// and overruns the buffer. + #[allow(clippy::too_many_arguments)] + async fn build_preorder_and_domain( + contract: &DataContract, + identity: &Identity, + signer: &SimpleSigner, + key: &IdentityPublicKey, + name: &str, + // A per-contender distinguishing byte placed in the *tail* of the + // salt so the two contenders' `saltedDomainHash` differ (avoiding a + // unique-index collision on the preorder), while byte[0] stays 0xFF. + salt_discriminator: u8, + rng: &mut StdRng, + platform_version: &PlatformVersion, + ) -> (Vec, Vec) { + let preorder = contract + .document_type_for_name("preorder") + .expect("expected preorder document type"); + let domain = contract + .document_type_for_name("domain") + .expect("expected domain document type"); + + let entropy = Bytes32::random_with_rng(rng); + + let mut preorder_document = preorder + .random_document_with_identifier_and_entropy( + rng, + identity.id(), + entropy, + DocumentFieldFillType::FillIfNotRequired, + DocumentFieldFillSize::AnyDocumentFillSize, + platform_version, + ) + .expect("expected a random preorder document"); + + let mut domain_document = domain + .random_document_with_identifier_and_entropy( + rng, + identity.id(), + entropy, + DocumentFieldFillType::FillIfNotRequired, + DocumentFieldFillSize::AnyDocumentFillSize, + platform_version, + ) + .expect("expected a random domain document"); + + domain_document.set("parentDomainName", "dash".into()); + domain_document.set("normalizedParentDomainName", "dash".into()); + domain_document.set("label", name.into()); + domain_document.set( + "normalizedLabel", + convert_to_homograph_safe_chars(name).into(), + ); + domain_document.set("records.identity", domain_document.owner_id().into()); + domain_document.set("subdomainRules.allowSubdomains", false.into()); + + // The crux of the attack: a 32-byte salt whose FIRST byte is 0xFF + // (a varint continuation byte). The last byte distinguishes the two + // contenders so their preorder salted hashes do not collide. + let mut salt: [u8; 32] = [0xFF; 32]; + salt[31] = salt_discriminator; + + let mut salted_domain_buffer: Vec = vec![]; + salted_domain_buffer.extend(salt); + salted_domain_buffer + .extend((convert_to_homograph_safe_chars(name) + ".dash").as_bytes()); + let salted_domain_hash = hash_double(salted_domain_buffer); + + preorder_document.set("saltedDomainHash", salted_domain_hash.into()); + domain_document.set("preorderSalt", salt.into()); + + let preorder_transition = + BatchTransition::new_document_creation_transition_from_document( + preorder_document, + preorder, + entropy.0, + key, + 2, + 0, + None, + signer, + platform_version, + None, + ) + .await + .expect("expect to create preorder batch transition"); + + let domain_transition = + BatchTransition::new_document_creation_transition_from_document( + domain_document, + domain, + entropy.0, + key, + 3, + 0, + None, + signer, + platform_version, + None, + ) + .await + .expect("expect to create domain batch transition"); + + ( + preorder_transition + .serialize_to_bytes() + .expect("serialize preorder transition"), + domain_transition + .serialize_to_bytes() + .expect("serialize domain transition"), + ) + } + + /// Runs the full contest setup and returns the result of the per-block + /// vote-poll resolver. When `widen` is true the byteArray `maxItems` is + /// flipped (the attack); when false the original contract is left in + /// place (the control). + async fn run_contest_then_resolve(widen: bool) -> Result<(), crate::error::Error> { + let mut platform = TestPlatformBuilder::new() + .with_latest_protocol_version() + .build_with_mock_rpc() + .set_initial_state_structure(); + + let platform_version = PlatformVersion::latest(); + + let mut rng = StdRng::seed_from_u64(0xB17E_A77A); + + // Two contenders, each prefunded so they can pay the contested-index + // voting balance. + let identity_1_info = setup_identity(&mut platform, rng.gen(), dash_to_credits!(0.5)); + let identity_2_info = setup_identity(&mut platform, rng.gen(), dash_to_credits!(0.5)); + + let contract_owner = setup_identity(&mut platform, rng.gen(), dash_to_credits!(0.5)); + + // (1) Create the CUSTOM contested contract owned by `contract_owner`. + // It has a contested unique index on `normalizedLabel` and a fixed + // byteArray `preorderSalt` {minItems:32, maxItems:32}. + let contract = setup_contract( + &platform.drive, + CUSTOM_CONTESTED_CONTRACT, + None, + Some(contract_owner.0.id().to_buffer()), + None::, + None, + Some(platform_version), + ); + + let name = "quantum"; + + let platform_state = platform.state.load(); + + // (2) Create TWO contested documents (contenders) with the same + // contested-index values and a 0xFF-prefixed 32-byte byteArray. + let (preorder_tx_1, domain_tx_1) = build_preorder_and_domain( + &contract, + &identity_1_info.0, + &identity_1_info.1, + &identity_1_info.2, + name, + 0x01, + &mut rng, + platform_version, + ) + .await; + + let (preorder_tx_2, domain_tx_2) = build_preorder_and_domain( + &contract, + &identity_2_info.0, + &identity_2_info.1, + &identity_2_info.2, + name, + 0x02, + &mut rng, + platform_version, + ) + .await; + + // Submit the preorders. + let transaction = platform.drive.grove.start_transaction(); + let processing_result = platform + .platform + .process_raw_state_transitions( + &[preorder_tx_1, preorder_tx_2], + &platform_state, + &BlockInfo::default_with_time( + platform_state + .last_committed_block_time_ms() + .unwrap_or_default() + + 3000, + ), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process preorder state transitions"); + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("expected to commit transaction"); + assert_eq!( + processing_result.valid_count(), + 2, + "both preorders should be accepted" + ); + + // Submit the domains -> this opens the contested vote poll. + let transaction = platform.drive.grove.start_transaction(); + let processing_result = platform + .platform + .process_raw_state_transitions( + &[domain_tx_1, domain_tx_2], + &platform_state, + &BlockInfo::default_with_time( + platform_state + .last_committed_block_time_ms() + .unwrap_or_default() + + 3000, + ), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process domain state transitions"); + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("expected to commit transaction"); + assert_eq!( + processing_result.valid_count(), + 2, + "both contenders should be accepted, opening the contest" + ); + + // (3) Simulate the malicious DataContractUpdate that widens the + // byteArray `maxItems` from 32 to 64 (only in the attack scenario). + if widen { + // The JSON-schema compatibility layer still treats widening + // `maxItems` as a compatible change -- which is exactly why the + // dedicated byte-array-encoding check added to `validate_update` + // (the fix) is required to reject it. We pin that compatibility + // verdict here to document the gap the fix closes. + let original_domain_schema = serde_json::json!({ + "type": "object", + "properties": { + "preorderSalt": { + "type": "array", + "byteArray": true, + "minItems": 32, + "maxItems": 32, + "position": 4 + } + }, + "additionalProperties": false + }); + let widened_domain_schema = serde_json::json!({ + "type": "object", + "properties": { + "preorderSalt": { + "type": "array", + "byteArray": true, + "minItems": 32, + "maxItems": 64, + "position": 4 + } + }, + "additionalProperties": false + }); + let compatibility = validate_schema_compatibility( + &original_domain_schema, + &widened_domain_schema, + platform_version, + ) + .expect("schema compatibility validation must not error"); + assert!( + compatibility.is_valid(), + "the maxItems 32 -> 64 widening must be accepted by update \ + validation for this attack to be reachable on chain; \ + reported incompatibilities: {:?}", + compatibility.errors + ); + + // Apply the widened contract directly to grovedb to simulate the + // corrupt on-disk state that WOULD result if such an update were + // committed. On a real chain `validate_update` now rejects this + // update at the source (see the rs-dpp validate_byte_array_encoding + // tests), so the resolver is never reached with corrupt bytes; + // this reproduces the consequence the fix prevents. + // The derived `properties` for `preorderSalt` flip to the + // variable-length (varint-prefixed) decode path. + let widened_contract = setup_contract( + &platform.drive, + CUSTOM_CONTESTED_CONTRACT_WIDENED, + None, + Some(contract_owner.0.id().to_buffer()), + None::, + None, + Some(platform_version), + ); + assert_eq!( + widened_contract.id(), + contract.id(), + "the widened contract must replace the original at the same id" + ); + } + + // (4) Advance past the vote-poll end date and invoke the resolver. + let time_after_distribution_limit = platform_version + .dpp + .voting_versions + .default_vote_poll_time_duration_test_network_ms + + 10_000; + + fast_forward_to_block(&platform, time_after_distribution_limit, 900, 42, 0, false); + + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + + // This is the exact per-block call. In the attack scenario it returns + // Err because the resolver loads the OLD contender bytes and decodes + // them against the WIDENED `preorderSalt` type, misreading the 0xFF + // first byte as a varint length -> CorruptedSerialization. + platform.check_for_ended_vote_polls( + &platform_state, + &platform_state, + &BlockInfo { + time_ms: time_after_distribution_limit, + height: 900, + core_height: 42, + epoch: Default::default(), + }, + Some(&transaction), + platform_version, + ) + } + + /// THE CHAIN-HALT CONSEQUENCE. If a byteArray `maxItems` widening were + /// ever committed, the stored contender documents become undecodable and + /// the per-block vote-poll resolver returns `Err`, which propagates out of + /// the bare-`?` per-block event handler and halts every validator. + /// + /// The fix prevents that state at the source: `validate_update` now + /// rejects the widening (see the rs-dpp validate_byte_array_encoding + /// tests). This test deliberately writes the corrupt state directly to + /// grovedb (bypassing validation) to reproduce the consequence the + /// validation fix prevents. + #[tokio::test] + async fn widening_byte_array_max_items_halts_vote_poll_resolver() { + let result = run_contest_then_resolve(true).await; + + assert!( + result.is_err(), + "check_for_ended_vote_polls must return Err when stored contender \ + documents are decoded against a widened byteArray encoding -- the \ + chain-halt consequence the `validate_update` fix prevents by \ + rejecting the update at the source. Got Ok instead." + ); + + // Confirm it is the expected decode-failure variant from the encoding + // flip, not some unrelated error. Matching the variant (rather than the + // Debug string) keeps the test robust to message/format changes. + assert_matches!( + result.unwrap_err(), + crate::error::Error::Protocol(dpp::ProtocolError::DataContractError( + dpp::data_contract::errors::DataContractError::CorruptedSerialization(_) + | dpp::data_contract::errors::DataContractError::DecodingContractError(_) + )) + ); + } + + /// CONTROL (causation proof). The exact same contest, but WITHOUT the + /// byteArray widening, resolves successfully (`Ok`). Together with the + /// test above this proves the widening is what causes the halt -- the + /// only difference between the two runs is the `maxItems` flip. + #[tokio::test] + async fn contest_without_widening_resolves_successfully() { + let result = run_contest_then_resolve(false).await; + + assert!( + result.is_ok(), + "control: without the byteArray widening the vote-poll resolver \ + must succeed; got {:?}", + result + ); + } + } } diff --git a/packages/rs-drive-abci/tests/supporting_files/contract/dpns/dpns-contract-contested-unique-index-byte-array-widened.json b/packages/rs-drive-abci/tests/supporting_files/contract/dpns/dpns-contract-contested-unique-index-byte-array-widened.json new file mode 100644 index 00000000000..b6759424a3f --- /dev/null +++ b/packages/rs-drive-abci/tests/supporting_files/contract/dpns/dpns-contract-contested-unique-index-byte-array-widened.json @@ -0,0 +1,169 @@ +{ + "$formatVersion": "0", + "id": "DWBXe9EXFPHxvbArQgT45uQR5gMmi8dfMpLhR5KSbwnZ", + "ownerId": "2QjL594djCH2NyDsn45vd6yQjEDHupMKo7CEGVTHtQxU", + "version": 2, + "documentSchemas": { + "domain": { + "documentsMutable": false, + "canBeDeleted": true, + "transferable": 1, + "tradeMode": 1, + "type": "object", + "indices": [ + { + "name": "parentNameAndLabel", + "properties": [ + { + "normalizedParentDomainName": "asc" + }, + { + "normalizedLabel": "asc" + } + ], + "unique": true, + "contested": { + "fieldMatches": [ + { + "field": "normalizedLabel", + "regexPattern": "^[a-zA-Z01]{3,19}$" + } + ], + "resolution": 0, + "description": "If the normalized label part of this index is less than 20 characters (all alphabet a-z and 0 and 1) then this index is non unique while contest resolution takes place." + } + }, + { + "name": "identityId", + "nullSearchable": false, + "properties": [ + { + "records.identity": "asc" + } + ] + } + ], + "properties": { + "label": { + "type": "string", + "pattern": "^[a-zA-Z0-9][a-zA-Z0-9-]{0,61}[a-zA-Z0-9]$", + "minLength": 3, + "maxLength": 63, + "position": 0, + "description": "Domain label. e.g. 'Bob'." + }, + "normalizedLabel": { + "type": "string", + "pattern": "^[a-hj-km-np-z0-9][a-hj-km-np-z0-9-]{0,61}[a-hj-km-np-z0-9]$", + "maxLength": 63, + "position": 1, + "description": "Domain label converted to lowercase for case-insensitive uniqueness validation. \"o\", \"i\" and \"l\" replaced with \"0\" and \"1\" to mitigate homograph attack. e.g. 'b0b'", + "$comment": "Must be equal to the label in lowercase. \"o\", \"i\" and \"l\" must be replaced with \"0\" and \"1\"." + }, + "parentDomainName": { + "type": "string", + "pattern": "^$|^[a-zA-Z0-9][a-zA-Z0-9-]{0,61}[a-zA-Z0-9]$", + "minLength": 0, + "maxLength": 63, + "position": 2, + "description": "A full parent domain name. e.g. 'dash'." + }, + "normalizedParentDomainName": { + "type": "string", + "pattern": "^$|^[a-hj-km-np-z0-9][a-hj-km-np-z0-9-\\.]{0,61}[a-hj-km-np-z0-9]$", + "minLength": 0, + "maxLength": 63, + "position": 3, + "description": "A parent domain name in lowercase for case-insensitive uniqueness validation. \"o\", \"i\" and \"l\" replaced with \"0\" and \"1\" to mitigate homograph attack. e.g. 'dash'", + "$comment": "Must either be equal to an existing domain or empty to create a top level domain. \"o\", \"i\" and \"l\" must be replaced with \"0\" and \"1\". Only the data contract owner can create top level domains." + }, + "preorderSalt": { + "type": "array", + "byteArray": true, + "minItems": 32, + "maxItems": 64, + "position": 4, + "description": "Salt used in the preorder document. maxItems WIDENED from 32 to 64 (the attack)." + }, + "records": { + "type": "object", + "properties": { + "identity": { + "type": "array", + "byteArray": true, + "minItems": 32, + "maxItems": 32, + "position": 1, + "contentMediaType": "application/x.dash.dpp.identifier", + "description": "Identifier name record that refers to an Identity" + } + }, + "minProperties": 1, + "position": 5, + "additionalProperties": false + }, + "subdomainRules": { + "type": "object", + "properties": { + "allowSubdomains": { + "type": "boolean", + "description": "This option defines who can create subdomains: true - anyone; false - only the domain owner", + "$comment": "Only the domain owner is allowed to create subdomains for non top-level domains", + "position": 0 + } + }, + "position": 6, + "description": "Subdomain rules allow domain owners to define rules for subdomains", + "additionalProperties": false, + "required": [ + "allowSubdomains" + ] + } + }, + "required": [ + "$createdAt", + "$updatedAt", + "$transferredAt", + "label", + "normalizedLabel", + "normalizedParentDomainName", + "preorderSalt", + "records", + "subdomainRules" + ], + "additionalProperties": false, + "$comment": "In order to register a domain you need to create a preorder. The preorder step is needed to prevent man-in-the-middle attacks. normalizedLabel + '.' + normalizedParentDomain must not be longer than 253 chars length as defined by RFC 1035. Domain documents are immutable: modification and deletion are restricted" + }, + "preorder": { + "documentsMutable": false, + "canBeDeleted": true, + "type": "object", + "indices": [ + { + "name": "saltedHash", + "properties": [ + { + "saltedDomainHash": "asc" + } + ], + "unique": true + } + ], + "properties": { + "saltedDomainHash": { + "type": "array", + "byteArray": true, + "minItems": 32, + "maxItems": 32, + "position": 0, + "description": "Double sha-256 of the concatenation of a 32 byte random salt and a normalized domain name" + } + }, + "required": [ + "saltedDomainHash" + ], + "additionalProperties": false, + "$comment": "Preorder documents are immutable: modification and deletion are restricted" + } + } +}