From ef440dfe2846ab83f34b4c337790d6ce45b7b5e6 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 12 Jun 2026 12:36:02 +0200 Subject: [PATCH 1/6] fix(dpp): enforce byte array encoding stability in data contract updates A byte array document property with `minItems == maxItems` is serialized as raw fixed-length bytes with no length prefix; any other size bounds use a variable-length (varint) prefix. Schema-compatibility validation treats widening or removing `maxItems` as compatible, so a contract update could cross that boundary and silently change the on-disk layout of existing documents, leaving them unreadable when re-decoded against the updated type. Reject, in document-type update validation, any byte array size-bound change that alters its on-disk encoding (crossing the fixed/variable boundary or changing the fixed length). Safe changes, such as widening an already variable-length byte array, remain allowed. Adds rs-dpp unit tests and an rs-drive-abci regression. Thanks to Daniel Derefaka (https://github.com/DanielDerefaka) for the report. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../methods/validate_update/v0/mod.rs | 169 +++++++ .../byte_array_encoding_flip_tests.rs | 300 +++++++++++++ .../document_type/property/mod.rs | 3 + .../byte_array_widen_accepted_tests.rs | 140 ++++++ .../validate_schema_compatibility/mod.rs | 3 + .../state_transition/state_transitions/mod.rs | 420 ++++++++++++++++++ ...ested-unique-index-byte-array-widened.json | 169 +++++++ 7 files changed, 1204 insertions(+) create mode 100644 packages/rs-dpp/src/data_contract/document_type/property/byte_array_encoding_flip_tests.rs create mode 100644 packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/byte_array_widen_accepted_tests.rs create mode 100644 packages/rs-drive-abci/tests/supporting_files/contract/dpns/dpns-contract-contested-unique-index-byte-array-widened.json 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..c3111406526 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,79 @@ 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. Failing it where stored documents are re-decoded + /// by a per-block handler would otherwise be unrecoverable. + fn validate_byte_array_encoding_stability( + &self, + new_document_type: DocumentTypeRef, + ) -> SimpleConsensusValidationResult { + // `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 +1395,103 @@ 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") + } + + fn assert_rejected(old_ba: platform_value::Value, new_ba: platform_value::Value) { + 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); + let result = old + .as_ref() + .validate_update_v0(new.as_ref(), platform_version) + .expect("validate_update should not error"); + 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 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); + let result = old + .as_ref() + .validate_update_v0(new.as_ref(), platform_version) + .expect("validate_update should not error"); + 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_changing_fixed_byte_array_size() { + 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 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}), + ); + } + } } 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..f5f285c4ca4 --- /dev/null +++ b/packages/rs-dpp/src/data_contract/document_type/property/byte_array_encoding_flip_tests.rs @@ -0,0 +1,300 @@ +//! 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)); + + let _ = contract; // keep signature symmetric / future-proof + 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..84fdeb6b196 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,424 @@ 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 `validate_byte_array_encoding` + // unit tests in rs-dpp), 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 `validate_byte_array_encoding` unit tests + /// in rs-dpp). 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 a serialization/corruption-class failure, i.e. the + // encoding-flip decode error and not some unrelated error. + let err = result.unwrap_err(); + let err_msg = format!("{:?}", err); + assert!( + err_msg.contains("Corrupted") + || err_msg.contains("orrupted") + || err_msg.contains("erializ") + || err_msg.contains("ecod"), + "expected a serialization/corruption/decoding error from the \ + encoding flip, got: {}", + err_msg + ); + } + + /// 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" + } + } +} From 180fa0edf7016737c355df41caf66a5e1ed63438 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 12 Jun 2026 12:48:02 +0200 Subject: [PATCH 2/6] test(dpp): cover byteArray maxItems removal and tidy unused test param Address review feedback on #3868: - add a validate_update test asserting that removing maxItems from a fixed byte array (fixed -> unbounded variable) is rejected - drop the unused `contract` parameter usage in a test helper Co-Authored-By: Claude Opus 4.8 (1M context) --- .../document_type/methods/validate_update/v0/mod.rs | 10 ++++++++++ .../property/byte_array_encoding_flip_tests.rs | 3 +-- 2 files changed, 11 insertions(+), 2 deletions(-) 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 c3111406526..c8f9cf5138b 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 @@ -1476,6 +1476,16 @@ mod tests { ); } + #[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 accepts_unchanged_fixed_byte_array() { assert_accepted( 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 index f5f285c4ca4..832ba61c864 100644 --- 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 @@ -94,7 +94,7 @@ fn build_contract_with_byte_array( /// 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 { +fn build_document_with_ff_prefixed_bytes(_contract: &DataContract) -> Document { let mut bytes = [0u8; 32]; bytes[0] = 0xFF; @@ -104,7 +104,6 @@ fn build_document_with_ff_prefixed_bytes(contract: &DataContract) -> Document { // happy-path round-trip comparison exact. properties.insert(BYTE_ARRAY_FIELD.to_string(), Value::Bytes32(bytes)); - let _ = contract; // keep signature symmetric / future-proof DocumentV0 { id: Identifier::new([1; 32]), owner_id: Identifier::new([2; 32]), From f71cb817f3e61dab4902372cb3838909cc912a48 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 12 Jun 2026 13:18:21 +0200 Subject: [PATCH 3/6] fix(dpp)!: version-gate the byte array encoding-stability check (protocol 12) Address review: a consensus tightening must not change behavior for already released protocol versions. validate_update is dispatched on a feature version that protocol versions 1-11 pin to 0, so adding the check in validate_update_v0 would alter consensus for deployed versions and could split upgraded vs unupgraded validators during a rolling rollout. - move the check into a new validate_update_v1 (= v0 checks + byte array encoding stability); validate_update_v0 is restored bit-exact - dispatcher: add arm 1 => validate_update_v1 - DPP_VALIDATION_VERSIONS_V3.document_type.validate_update: 0 -> 1 (V3 is used only by protocol 12 / release 3.1.0), so the rule activates atomically at that protocol upgrade rather than on binary rollout - tests now exercise the public validate_update dispatcher (latest selects v1) plus a pre-activation version (protocol 11 still accepts the flip), so a missing arm or stale feature-version constant fails a test - e2e: assert the decode-failure error variant instead of substring matching Co-Authored-By: Claude Opus 4.8 (1M context) --- .../methods/validate_update/mod.rs | 4 +- .../methods/validate_update/v0/mod.rs | 179 ------------ .../methods/validate_update/v1/mod.rs | 256 ++++++++++++++++++ .../state_transition/state_transitions/mod.rs | 39 ++- .../dpp_validation_versions/v3.rs | 2 +- 5 files changed, 279 insertions(+), 201 deletions(-) create mode 100644 packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs diff --git a/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/mod.rs b/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/mod.rs index 8accc4522fe..2fdaf02d702 100644 --- a/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/mod.rs +++ b/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/mod.rs @@ -4,6 +4,7 @@ use crate::ProtocolError; use platform_version::version::PlatformVersion; mod v0; +mod v1; impl DocumentTypeRef<'_> { /// Verify that the update to the document type is valid. @@ -20,9 +21,10 @@ impl DocumentTypeRef<'_> { .validate_update { 0 => self.validate_update_v0(new_document_type, platform_version), + 1 => self.validate_update_v1(new_document_type, platform_version), version => Err(ProtocolError::UnknownVersionMismatch { method: "validate_update".to_string(), - known_versions: vec![0], + known_versions: vec![0, 1], received: version, }), } 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 c8f9cf5138b..ddb72c9a71f 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,7 +3,6 @@ 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; @@ -34,79 +33,10 @@ 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. Failing it where stored documents are re-decoded - /// by a per-block handler would otherwise be unrecoverable. - fn validate_byte_array_encoding_stability( - &self, - new_document_type: DocumentTypeRef, - ) -> SimpleConsensusValidationResult { - // `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, @@ -1395,113 +1325,4 @@ 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") - } - - fn assert_rejected(old_ba: platform_value::Value, new_ba: platform_value::Value) { - 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); - let result = old - .as_ref() - .validate_update_v0(new.as_ref(), platform_version) - .expect("validate_update should not error"); - 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 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); - let result = old - .as_ref() - .validate_update_v0(new.as_ref(), platform_version) - .expect("validate_update should not error"); - 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_changing_fixed_byte_array_size() { - 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_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 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}), - ); - } - } } diff --git a/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs b/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs new file mode 100644 index 00000000000..fd3bb184184 --- /dev/null +++ b/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs @@ -0,0 +1,256 @@ +use crate::consensus::state::data_contract::document_type_update_error::DocumentTypeUpdateError; +use crate::data_contract::document_type::accessors::DocumentTypeV0Getters; +use crate::data_contract::document_type::property::{ByteArrayPropertySizes, DocumentPropertyType}; +use crate::data_contract::document_type::DocumentTypeRef; +use crate::validation::SimpleConsensusValidationResult; +use crate::ProtocolError; +use platform_version::version::PlatformVersion; + +impl DocumentTypeRef<'_> { + /// `validate_update` feature version 1. + /// + /// Identical to v0 (config + index + schema-compatibility) plus an extra + /// check that no byte array property changes its on-disk encoding. v0 is left + /// bit-exact for already-released protocol versions; this tighter rule is + /// selected only by the protocol version that pins `validate_update` to 1, so + /// it activates atomically at that protocol upgrade rather than on binary + /// rollout. + #[inline(always)] + pub(super) fn validate_update_v1( + &self, + new_document_type: DocumentTypeRef, + platform_version: &PlatformVersion, + ) -> Result { + let result = self.validate_update_v0(new_document_type, platform_version)?; + + if !result.is_valid() { + return Ok(result); + } + + Ok(self.validate_byte_array_encoding_stability(new_document_type)) + } + + /// 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. + fn validate_byte_array_encoding_stability( + &self, + new_document_type: DocumentTypeRef, + ) -> SimpleConsensusValidationResult { + // `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() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::consensus::state::state_error::StateError; + use crate::consensus::ConsensusError; + use crate::data_contract::config::DataContractConfig; + use crate::data_contract::document_type::DocumentType; + use assert_matches::assert_matches; + use platform_value::{platform_value, Identifier}; + 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 so a missing match arm or + /// stale `validate_update` feature-version constant fails the test. The latest + /// protocol version selects feature version 1 (the byte-array check). + 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 byte_array_check_rejects_fixed_size_change() { + // A fixed-size change (32 -> 64) also alters the on-disk layout and is + // rejected by the byte-array check. Through the public dispatcher this + // case is additionally caught earlier by JSON-schema compatibility (a + // minItems widening is itself incompatible), so we exercise the byte-array + // check in isolation here to cover its fixed-size branch directly. + let platform_version = PlatformVersion::latest(); + let old = document_type_with_byte_array( + platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":32,"position":0}), + platform_version, + ); + let new = document_type_with_byte_array( + platform_value!({"type":"array","byteArray":true,"minItems":64,"maxItems":64,"position":0}), + platform_version, + ); + let result = old + .as_ref() + .validate_byte_array_encoding_stability(new.as_ref()); + assert_matches!( + result.errors.as_slice(), + [ConsensusError::StateError(StateError::DocumentTypeUpdateError(e))] + if e.additional_message().contains("byte array encoding") + ); + } + + #[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 pre_activation_protocol_version_still_accepts_the_flip() { + // Gating proof: under a protocol version that pins `validate_update` to + // feature version 0 (no byte-array check), the same encoding flip is + // accepted. This is why the rule must activate at a protocol version + // boundary rather than on binary rollout. + let platform_version = PlatformVersion::get(11).expect("protocol version 11 should exist"); + let old = document_type_with_byte_array( + platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":32,"position":0}), + platform_version, + ); + let new = document_type_with_byte_array( + platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":64,"position":0}), + platform_version, + ); + let result = old + .as_ref() + .validate_update(new.as_ref(), platform_version) + .expect("validate_update should not error"); + assert!( + result.is_valid(), + "pre-activation protocol version must NOT reject the flip, got {:?}", + result.errors + ); + } +} 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 84fdeb6b196..c7e34cc7faa 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 @@ -3431,10 +3431,11 @@ pub(in crate::execution) mod tests { // 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 `validate_byte_array_encoding` - // unit tests in rs-dpp), so the resolver is never reached with - // corrupt bytes; this reproduces the consequence the fix prevents. + // committed. On a real chain `validate_update` (feature version 1, + // active from protocol 12) now rejects this update at the source + // (see the rs-dpp validate_update::v1 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( @@ -3488,10 +3489,11 @@ pub(in crate::execution) mod tests { /// 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 `validate_byte_array_encoding` unit tests - /// in rs-dpp). This test deliberately writes the corrupt state directly to - /// grovedb (bypassing validation) to reproduce the consequence the + /// The fix prevents that state at the source: `validate_update` (feature + /// version 1, active from protocol 12) now rejects the widening (see the + /// rs-dpp validate_update::v1 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() { @@ -3505,18 +3507,15 @@ pub(in crate::execution) mod tests { rejecting the update at the source. Got Ok instead." ); - // Confirm it is a serialization/corruption-class failure, i.e. the - // encoding-flip decode error and not some unrelated error. - let err = result.unwrap_err(); - let err_msg = format!("{:?}", err); - assert!( - err_msg.contains("Corrupted") - || err_msg.contains("orrupted") - || err_msg.contains("erializ") - || err_msg.contains("ecod"), - "expected a serialization/corruption/decoding error from the \ - encoding flip, got: {}", - err_msg + // 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(_) + )) ); } diff --git a/packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v3.rs b/packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v3.rs index d2d4795d6fe..676ac9c16a8 100644 --- a/packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v3.rs +++ b/packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v3.rs @@ -23,7 +23,7 @@ pub const DPP_VALIDATION_VERSIONS_V3: DPPValidationVersions = DPPValidationVersi validate_localizations: 0, }, document_type: DocumentTypeValidationVersions { - validate_update: 0, + validate_update: 1, contested_index_limit: 1, unique_index_limit: 10, }, From 18775d81259969dea9ec8c1344ddeb524542b156 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 12 Jun 2026 13:28:39 +0200 Subject: [PATCH 4/6] test(dpp): pin implicit-minItems byte array classification Address review: clarify that `fixed_length` deliberately mirrors the encoder (`encode_value_ref_with_size`), which uses the raw, no-length-prefix path ONLY when both `minItems` and `maxItems` are present and equal. An omitted `minItems` (`None`) is varint length-prefixed, so it is correctly NOT treated as fixed. Adds a test that `{maxItems: 0} -> {maxItems: 1}` (both variable) stays allowed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../methods/validate_update/v1/mod.rs | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs b/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs index fd3bb184184..a830e7dbc74 100644 --- a/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs +++ b/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs @@ -42,8 +42,13 @@ impl DocumentTypeRef<'_> { &self, new_document_type: DocumentTypeRef, ) -> SimpleConsensusValidationResult { - // `Some(n)` => fixed raw encoding of length `n`; `None` => variable - // (varint length-prefixed) encoding. + // 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), @@ -218,6 +223,19 @@ mod tests { ); } + #[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 (it mirrors encode_value_ref_with_size). + assert_accepted( + platform_value!({"type":"array","byteArray":true,"maxItems":0,"position":0}), + platform_value!({"type":"array","byteArray":true,"maxItems":1,"position":0}), + ); + } + #[test] fn accepts_widening_already_variable_byte_array() { // Variable-length on both sides: the on-disk encoding does not change, so From 7de917886b19a9352602a3b1f7ee86ee19adf181 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 12 Jun 2026 18:40:21 +0200 Subject: [PATCH 5/6] refactor(dpp): apply byte array encoding-stability check in place (match #3865) Per maintainer decision, drop the version-gating and add the byte-array encoding-stability check directly to validate_update_v0, consistent with the sibling chain-halt fix #3865 which added its rejection rule to validate_structure_v0 in place. The check runs before validate_schema so it cannot be bypassed by a JSON-schema-compatible widening. - revert validate_update_v1, the dispatcher arm, and the DPP_VALIDATION_VERSIONS_V3 feature-version bump - check + tests now live in validate_update/v0 (tests still exercise the public validate_update dispatcher); drops the pre-activation gating test - e2e error assertion keeps assert_matches! on the decode-failure variant Co-Authored-By: Claude Opus 4.8 (1M context) --- .../methods/validate_update/mod.rs | 4 +- .../methods/validate_update/v0/mod.rs | 201 +++++++++++++ .../methods/validate_update/v1/mod.rs | 274 ------------------ .../state_transition/state_transitions/mod.rs | 18 +- .../dpp_validation_versions/v3.rs | 2 +- 5 files changed, 211 insertions(+), 288 deletions(-) delete mode 100644 packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs diff --git a/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/mod.rs b/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/mod.rs index 2fdaf02d702..8accc4522fe 100644 --- a/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/mod.rs +++ b/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/mod.rs @@ -4,7 +4,6 @@ use crate::ProtocolError; use platform_version::version::PlatformVersion; mod v0; -mod v1; impl DocumentTypeRef<'_> { /// Verify that the update to the document type is valid. @@ -21,10 +20,9 @@ impl DocumentTypeRef<'_> { .validate_update { 0 => self.validate_update_v0(new_document_type, platform_version), - 1 => self.validate_update_v1(new_document_type, platform_version), version => Err(ProtocolError::UnknownVersionMismatch { method: "validate_update".to_string(), - known_versions: vec![0, 1], + known_versions: vec![0], received: version, }), } 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..43bba000956 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,130 @@ 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 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/methods/validate_update/v1/mod.rs b/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs deleted file mode 100644 index a830e7dbc74..00000000000 --- a/packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs +++ /dev/null @@ -1,274 +0,0 @@ -use crate::consensus::state::data_contract::document_type_update_error::DocumentTypeUpdateError; -use crate::data_contract::document_type::accessors::DocumentTypeV0Getters; -use crate::data_contract::document_type::property::{ByteArrayPropertySizes, DocumentPropertyType}; -use crate::data_contract::document_type::DocumentTypeRef; -use crate::validation::SimpleConsensusValidationResult; -use crate::ProtocolError; -use platform_version::version::PlatformVersion; - -impl DocumentTypeRef<'_> { - /// `validate_update` feature version 1. - /// - /// Identical to v0 (config + index + schema-compatibility) plus an extra - /// check that no byte array property changes its on-disk encoding. v0 is left - /// bit-exact for already-released protocol versions; this tighter rule is - /// selected only by the protocol version that pins `validate_update` to 1, so - /// it activates atomically at that protocol upgrade rather than on binary - /// rollout. - #[inline(always)] - pub(super) fn validate_update_v1( - &self, - new_document_type: DocumentTypeRef, - platform_version: &PlatformVersion, - ) -> Result { - let result = self.validate_update_v0(new_document_type, platform_version)?; - - if !result.is_valid() { - return Ok(result); - } - - Ok(self.validate_byte_array_encoding_stability(new_document_type)) - } - - /// 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. - 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() - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::consensus::state::state_error::StateError; - use crate::consensus::ConsensusError; - use crate::data_contract::config::DataContractConfig; - use crate::data_contract::document_type::DocumentType; - use assert_matches::assert_matches; - use platform_value::{platform_value, Identifier}; - 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 so a missing match arm or - /// stale `validate_update` feature-version constant fails the test. The latest - /// protocol version selects feature version 1 (the byte-array check). - 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 byte_array_check_rejects_fixed_size_change() { - // A fixed-size change (32 -> 64) also alters the on-disk layout and is - // rejected by the byte-array check. Through the public dispatcher this - // case is additionally caught earlier by JSON-schema compatibility (a - // minItems widening is itself incompatible), so we exercise the byte-array - // check in isolation here to cover its fixed-size branch directly. - let platform_version = PlatformVersion::latest(); - let old = document_type_with_byte_array( - platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":32,"position":0}), - platform_version, - ); - let new = document_type_with_byte_array( - platform_value!({"type":"array","byteArray":true,"minItems":64,"maxItems":64,"position":0}), - platform_version, - ); - let result = old - .as_ref() - .validate_byte_array_encoding_stability(new.as_ref()); - assert_matches!( - result.errors.as_slice(), - [ConsensusError::StateError(StateError::DocumentTypeUpdateError(e))] - if e.additional_message().contains("byte array encoding") - ); - } - - #[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_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 (it mirrors encode_value_ref_with_size). - assert_accepted( - platform_value!({"type":"array","byteArray":true,"maxItems":0,"position":0}), - platform_value!({"type":"array","byteArray":true,"maxItems":1,"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 pre_activation_protocol_version_still_accepts_the_flip() { - // Gating proof: under a protocol version that pins `validate_update` to - // feature version 0 (no byte-array check), the same encoding flip is - // accepted. This is why the rule must activate at a protocol version - // boundary rather than on binary rollout. - let platform_version = PlatformVersion::get(11).expect("protocol version 11 should exist"); - let old = document_type_with_byte_array( - platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":32,"position":0}), - platform_version, - ); - let new = document_type_with_byte_array( - platform_value!({"type":"array","byteArray":true,"minItems":32,"maxItems":64,"position":0}), - platform_version, - ); - let result = old - .as_ref() - .validate_update(new.as_ref(), platform_version) - .expect("validate_update should not error"); - assert!( - result.is_valid(), - "pre-activation protocol version must NOT reject the flip, got {:?}", - result.errors - ); - } -} 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 c7e34cc7faa..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 @@ -3431,11 +3431,10 @@ pub(in crate::execution) mod tests { // 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` (feature version 1, - // active from protocol 12) now rejects this update at the source - // (see the rs-dpp validate_update::v1 tests), so the resolver is - // never reached with corrupt bytes; this reproduces the - // consequence the fix prevents. + // 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( @@ -3489,11 +3488,10 @@ pub(in crate::execution) mod tests { /// 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` (feature - /// version 1, active from protocol 12) now rejects the widening (see the - /// rs-dpp validate_update::v1 tests). This test deliberately writes the - /// corrupt state directly to grovedb (bypassing validation) to reproduce - /// the consequence the + /// 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() { diff --git a/packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v3.rs b/packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v3.rs index 676ac9c16a8..d2d4795d6fe 100644 --- a/packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v3.rs +++ b/packages/rs-platform-version/src/version/dpp_versions/dpp_validation_versions/v3.rs @@ -23,7 +23,7 @@ pub const DPP_VALIDATION_VERSIONS_V3: DPPValidationVersions = DPPValidationVersi validate_localizations: 0, }, document_type: DocumentTypeValidationVersions { - validate_update: 1, + validate_update: 0, contested_index_limit: 1, unique_index_limit: 10, }, From 14ba9484d888697a4c95c34835b8644c5a032bad Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 12 Jun 2026 18:48:13 +0200 Subject: [PATCH 6/6] test(dpp): cover variable->fixed byteArray encoding flip Address review: add an explicit test that narrowing a variable (varint length-prefixed) byte array to fixed (raw) is rejected, documenting the reverse of the already-tested fixed->variable cases. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../document_type/methods/validate_update/v0/mod.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) 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 43bba000956..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 @@ -1495,6 +1495,17 @@ mod tests { ); } + #[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(