diff --git a/CHANGELOG.md b/CHANGELOG.md index 880e18b..24bf920 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,45 @@ # Changelog +## [1.3.2] - 2025-12-03 + +### Security Fixes (Bugbot Review) + +This release addresses critical security issues identified by Cursor Bugbot code review. + +### Fixed + +#### C# Library +- **SorobanHelper: Data truncation vulnerability** - `EncodeBytesAsScVal` and `EncodeStringAsScVal` now validate input length and throw `ArgumentException` for data exceeding 255 bytes, preventing silent data corruption +- **SorobanRpcClient: XDR boolean decode false positives** - Replaced unsafe `xdrBytes.Any(b => b == 0x01)` heuristic with proper SCVal format parsing using type discriminant validation + +#### Rust Smart Contract +- **Balance comparison logic bug** - `verify_balance_proof` now uses proper numeric comparison via `parse_decimal_to_scaled()` instead of incorrect byte length comparison (`balance_data.len() >= required_amount_data.len()`) +- **Malformed input vulnerability** - `parse_decimal_to_scaled()` now returns `None` for malformed inputs like "-", ".", or empty bytes instead of `Some(0)`, preventing invalid balance data from being treated as zero +- **Test algorithm mismatch** - All tests now use `compute_test_hmac()` with proper HMAC-SHA256 (RFC 2104 with ipad/opad) instead of plain SHA256, matching production contract behavior + +### Added +- `SorobanHelper.MaxBytesLength` constant (255) for explicit length limit documentation +- `parse_decimal_to_scaled()` function in Rust contract for accurate decimal number parsing with `has_digits` validation +- `compute_test_hmac()` helper in Rust tests for consistent HMAC computation +- `test_verify_balance_proof_insufficient()` test case for balance < required scenario +- `test_verify_balance_proof_malformed_input()` test case for malformed inputs like "-", "." + +### Changed +- Balance verification now correctly handles edge cases like "99.0" vs "100.0" +- XDR boolean decoding now validates SCValType discriminant before extracting value + +--- + +## [1.3.1] - 2025-12-03 + +### Fixed +- Added `SorobanHelper` class with SCVal encoding/decoding utilities +- Fixed balance parsing with `CultureInfo.InvariantCulture` for consistent decimal handling +- Added `using StellarDotnetSdk.Accounts` for `KeyPair` class access in tests +- Improved test coverage for Stellar integration + +--- + ## [1.3.0] - 2025-12-02 ### Major Release: Production-Ready Stellar Integration diff --git a/ZkpSharp/Integration/Stellar/SorobanHelper.cs b/ZkpSharp/Integration/Stellar/SorobanHelper.cs index 357f2f1..417cd95 100644 --- a/ZkpSharp/Integration/Stellar/SorobanHelper.cs +++ b/ZkpSharp/Integration/Stellar/SorobanHelper.cs @@ -13,6 +13,12 @@ public static class SorobanHelper /// /// The bytes to encode. /// Base64-encoded SCVal representation. + /// + /// Maximum length for byte data in simplified SCVal encoding. + /// For larger data, use proper XDR encoding with Stellar SDK. + /// + public const int MaxBytesLength = 255; + public static string EncodeBytesAsScVal(byte[] bytes) { if (bytes == null || bytes.Length == 0) @@ -20,6 +26,14 @@ public static string EncodeBytesAsScVal(byte[] bytes) throw new ArgumentException("Bytes cannot be null or empty.", nameof(bytes)); } + if (bytes.Length > MaxBytesLength) + { + throw new ArgumentException( + $"Bytes length ({bytes.Length}) exceeds maximum allowed ({MaxBytesLength}). " + + "For larger data, use proper XDR encoding with Stellar SDK.", + nameof(bytes)); + } + // For SCVal bytes type, we prepend a type indicator and encode // Type 14 (0x0E) is SCValType::SCV_BYTES in Soroban var scVal = new byte[bytes.Length + 4]; @@ -73,6 +87,14 @@ public static string EncodeStringAsScVal(string value) var bytes = Encoding.UTF8.GetBytes(value); + if (bytes.Length > MaxBytesLength) + { + throw new ArgumentException( + $"String byte length ({bytes.Length}) exceeds maximum allowed ({MaxBytesLength}). " + + "For larger strings, use proper XDR encoding with Stellar SDK.", + nameof(value)); + } + // Type 14 (0x0E) is also used for strings in Soroban (as bytes) var scVal = new byte[bytes.Length + 4]; scVal[0] = 0x0E; // SCValType::SCV_STRING (represented as bytes) diff --git a/ZkpSharp/Integration/Stellar/SorobanRpcClient.cs b/ZkpSharp/Integration/Stellar/SorobanRpcClient.cs index 4373f98..ac274ae 100644 --- a/ZkpSharp/Integration/Stellar/SorobanRpcClient.cs +++ b/ZkpSharp/Integration/Stellar/SorobanRpcClient.cs @@ -229,14 +229,42 @@ private bool DecodeBooleanFromXdr(string xdrBase64) // Decode XDR base64 string var xdrBytes = Convert.FromBase64String(xdrBase64); - // Simplified parsing - proper XDR decoding requires Soroban SDK - // This is a best-effort attempt - if (xdrBytes.Length >= 2) + // Soroban SCVal boolean format: + // - First 4 bytes: type discriminant (0x00000000 for SCV_BOOL) + // - Next 4 bytes: value (0x00000000 for false, 0x00000001 for true) + // Minimum 8 bytes required for a valid SCVal boolean + + if (xdrBytes.Length < 4) + { + return false; + } + + // Check for SCVal boolean type discriminant + // SCV_BOOL = 0, SCV_TRUE = 1, SCV_FALSE = 0 (in older format) + // In XDR big-endian format: + // - Type 0 (SCV_BOOL) with value in next bytes + // - Or Type 1 (SCV_VOID which we treat as false) + + // Check the type discriminant (first 4 bytes, big-endian) + int typeDiscriminant = (xdrBytes[0] << 24) | (xdrBytes[1] << 16) | + (xdrBytes[2] << 8) | xdrBytes[3]; + + // SCValType::SCV_BOOL = 0 + if (typeDiscriminant == 0 && xdrBytes.Length >= 8) + { + // For SCV_BOOL, the value is in the next 4 bytes + int value = (xdrBytes[4] << 24) | (xdrBytes[5] << 16) | + (xdrBytes[6] << 8) | xdrBytes[7]; + return value != 0; + } + + // SCValType::SCV_TRUE = 1 (alternative representation) + if (typeDiscriminant == 1) { - // Basic heuristic: look for boolean indicators - return xdrBytes.Any(b => b == 0x01); + return true; } + // Default to false for unknown formats return false; } catch (FormatException ex) diff --git a/ZkpSharp/ZkpSharp.csproj b/ZkpSharp/ZkpSharp.csproj index bae7d9f..256b700 100644 --- a/ZkpSharp/ZkpSharp.csproj +++ b/ZkpSharp/ZkpSharp.csproj @@ -4,7 +4,7 @@ net8.0 enable enable - 1.3.1 + 1.3.2 README.md https://github.com/asagynbaev https://github.com/asagynbaev/ZkpSharp @@ -13,7 +13,7 @@ Azimbek Sagynbaev .NET library for implementing Zero-Knowledge Proofs (ZKP) with production-ready Stellar Soroban integration. Supports age, balance, membership, range, and time condition proofs with on-chain verification. zkp;zero-knowledge-proof;cryptography;privacy;blockchain;stellar;soroban;smart-contracts;ton;solana;hmac - v1.3.1: Bug fixes - added SorobanHelper class, fixed balance parsing with InvariantCulture, improved test coverage. + v1.3.2: Security fixes - SorobanHelper validates max 255 bytes to prevent truncation, XDR boolean decode uses proper SCVal format instead of heuristic, Rust contract uses numeric comparison for balance verification, tests use HMAC-SHA256 matching production. diff --git a/contracts/stellar/contracts/proof-balance/src/lib.rs b/contracts/stellar/contracts/proof-balance/src/lib.rs index 9dd3eb3..c7c8daa 100644 --- a/contracts/stellar/contracts/proof-balance/src/lib.rs +++ b/contracts/stellar/contracts/proof-balance/src/lib.rs @@ -100,8 +100,8 @@ impl ZkpVerifier { /// # Arguments /// * `env` - The Soroban environment /// * `proof` - The proof hash to verify - /// * `balance_data` - The balance value as bytes - /// * `required_amount_data` - The required amount as bytes + /// * `balance_data` - The balance value as bytes (decimal string, e.g., "1000.50") + /// * `required_amount_data` - The required amount as bytes (decimal string, e.g., "500.25") /// * `salt` - The cryptographic salt /// * `hmac_key` - The HMAC secret key /// @@ -128,9 +128,20 @@ impl ZkpVerifier { return false; } - // Additional validation: ensure balance >= required_amount - // This is a simplified check - in production, you'd parse the actual numeric values - let balance_sufficient = balance_data.len() >= required_amount_data.len(); + // Parse and compare numeric values + let balance = Self::parse_decimal_to_scaled(&balance_data); + let required = Self::parse_decimal_to_scaled(&required_amount_data); + + let balance_sufficient = match (balance, required) { + (Some(b), Some(r)) => b >= r, + _ => { + env.events().publish( + (Symbol::new(&env, "error"),), + VerificationError::InvalidInput as u32, + ); + false + } + }; env.events().publish( (Symbol::new(&env, "balance_check"),), @@ -140,6 +151,78 @@ impl ZkpVerifier { proof_valid && balance_sufficient } + /// Parses a decimal string (e.g., "1234.56") to a scaled integer for comparison. + /// Returns None if parsing fails or if no digits are present. + /// The result is scaled by 10^8 to handle up to 8 decimal places. + fn parse_decimal_to_scaled(data: &Bytes) -> Option { + // Return None for empty input + if data.len() == 0 { + return None; + } + + let mut result: i128 = 0; + let mut decimal_places: u32 = 0; + let mut found_decimal = false; + let mut is_negative = false; + let mut has_digits = false; // Track if at least one digit was parsed + + for i in 0..data.len() { + let byte = data.get(i)?; + + // Handle negative sign (only at the start, before any digits) + if byte == b'-' && !has_digits && !found_decimal { + is_negative = true; + continue; + } + + // Handle decimal point + if byte == b'.' { + if found_decimal { + return None; // Multiple decimal points + } + found_decimal = true; + continue; + } + + // Handle digits + if byte >= b'0' && byte <= b'9' { + has_digits = true; + let digit = (byte - b'0') as i128; + result = result.checked_mul(10)?.checked_add(digit)?; + + if found_decimal { + decimal_places += 1; + if decimal_places > 8 { + // Too many decimal places, stop processing + break; + } + } + } else if byte != b' ' { + // Invalid character (allow spaces to be ignored) + return None; + } + } + + // Must have at least one digit to be valid + if !has_digits { + return None; + } + + // Scale to 8 decimal places for consistent comparison + const SCALE_FACTOR: u32 = 8; + let scale_needed = SCALE_FACTOR.saturating_sub(decimal_places); + + for _ in 0..scale_needed { + result = result.checked_mul(10)?; + } + + if is_negative { + result = -result; + } + + Some(result) + } + /// Batch verification of multiple proofs for efficiency. /// /// # Arguments diff --git a/contracts/stellar/contracts/proof-balance/src/test.rs b/contracts/stellar/contracts/proof-balance/src/test.rs index 2ea04b2..8906115 100644 --- a/contracts/stellar/contracts/proof-balance/src/test.rs +++ b/contracts/stellar/contracts/proof-balance/src/test.rs @@ -24,18 +24,50 @@ mod test { salt } - /// Helper to manually compute HMAC for testing - fn compute_expected_hmac(env: &Env, data: &Bytes, salt: &Bytes, key: &BytesN<32>) -> BytesN<32> { - let contract = ZkpVerifierClient::new(env, &env.register_contract(None, ZkpVerifier)); + /// Computes HMAC-SHA256 for testing - matches the contract's compute_hmac implementation. + /// This is the same algorithm used in the contract to ensure tests match production behavior. + fn compute_test_hmac(env: &Env, message: &Bytes, key: &BytesN<32>) -> BytesN<32> { + const IPAD: u8 = 0x36; + const OPAD: u8 = 0x5c; + const BLOCK_SIZE: u32 = 64; + + // Create padded key (64 bytes) + let mut key_padded = Bytes::new(env); + for i in 0..32 { + key_padded.push_back(key.get(i).unwrap()); + } + for _ in 32..BLOCK_SIZE { + key_padded.push_back(0); + } + + // Compute inner hash: H((K ⊕ ipad) || m) + let mut inner_data = Bytes::new(env); + for i in 0..BLOCK_SIZE { + inner_data.push_back(key_padded.get(i).unwrap() ^ IPAD); + } + inner_data.append(message); - // Concatenate data and salt + let inner_hash = env.crypto().sha256(&inner_data); + + // Compute outer hash: H((K ⊕ opad) || inner_hash) + let mut outer_data = Bytes::new(env); + for i in 0..BLOCK_SIZE { + outer_data.push_back(key_padded.get(i).unwrap() ^ OPAD); + } + outer_data.append(&inner_hash.to_bytes()); + + env.crypto().sha256(&outer_data) + } + + /// Helper to compute expected HMAC proof for test data + fn compute_expected_proof(env: &Env, data: &Bytes, salt: &Bytes, key: &BytesN<32>) -> BytesN<32> { + // Concatenate data and salt (same as contract does) let mut message = Bytes::new(env); message.append(data); message.append(salt); - // For testing, we'll use the contract's internal HMAC computation - // In real scenario, this would match the C# library's HMAC output - env.crypto().sha256(&message) // Simplified for testing + // Compute HMAC-SHA256 (matching contract's algorithm) + compute_test_hmac(env, &message, key) } #[test] @@ -53,14 +85,8 @@ mod test { let mut data = Bytes::new(&env); data.extend_from_array(&[1, 2, 3, 4, 5]); - // Concatenate data and salt for HMAC - let mut message = Bytes::new(&env); - message.append(&data); - message.append(&salt); - - // For this test, we compute the expected proof using the same logic - // In production, this would come from the C# library - let proof = env.crypto().sha256(&message); + // Compute proof using HMAC-SHA256 (same algorithm as contract) + let proof = compute_expected_proof(&env, &data, &salt, &key); // Verify the proof let result = client.verify_proof(&proof, &data, &salt, &key); @@ -138,11 +164,8 @@ mod test { let mut required_data = Bytes::new(&env); required_data.extend_from_array(b"500.0"); - // Compute proof for the balance - let mut message = Bytes::new(&env); - message.append(&balance_data); - message.append(&salt); - let proof = env.crypto().sha256(&message); + // Compute proof using HMAC-SHA256 + let proof = compute_expected_proof(&env, &balance_data, &salt, &key); // Verify balance proof let result = client.verify_balance_proof( @@ -156,6 +179,89 @@ mod test { assert!(result, "Valid balance proof should be verified"); } + #[test] + fn test_verify_balance_proof_insufficient() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register_contract(None, ZkpVerifier); + let client = ZkpVerifierClient::new(&env, &contract_id); + + let key = create_test_key(&env); + let salt = create_test_salt(&env); + + // Balance data - smaller than required + let mut balance_data = Bytes::new(&env); + balance_data.extend_from_array(b"99.0"); + + // Required amount - larger than balance + let mut required_data = Bytes::new(&env); + required_data.extend_from_array(b"100.0"); + + // Compute valid proof for the balance + let proof = compute_expected_proof(&env, &balance_data, &salt, &key); + + // Verify balance proof - should fail because balance < required + let result = client.verify_balance_proof( + &proof, + &balance_data, + &required_data, + &salt, + &key, + ); + + assert!(!result, "Balance proof should fail when balance < required"); + } + + #[test] + fn test_verify_balance_proof_malformed_input() { + let env = Env::default(); + env.mock_all_auths(); + + let contract_id = env.register_contract(None, ZkpVerifier); + let client = ZkpVerifierClient::new(&env, &contract_id); + + let key = create_test_key(&env); + let salt = create_test_salt(&env); + + // Test with malformed balance data (just "-") + let mut malformed_balance = Bytes::new(&env); + malformed_balance.extend_from_array(b"-"); + + let mut required_data = Bytes::new(&env); + required_data.extend_from_array(b"100.0"); + + // Compute proof for the malformed data + let proof = compute_expected_proof(&env, &malformed_balance, &salt, &key); + + // Should fail because "-" is not a valid number + let result = client.verify_balance_proof( + &proof, + &malformed_balance, + &required_data, + &salt, + &key, + ); + + assert!(!result, "Malformed balance '-' should fail verification"); + + // Test with just decimal point "." + let mut dot_only = Bytes::new(&env); + dot_only.extend_from_array(b"."); + + let proof2 = compute_expected_proof(&env, &dot_only, &salt, &key); + + let result2 = client.verify_balance_proof( + &proof2, + &dot_only, + &required_data, + &salt, + &key, + ); + + assert!(!result2, "Malformed balance '.' should fail verification"); + } + #[test] fn test_batch_verification_all_valid() { let env = Env::default(); @@ -177,10 +283,8 @@ mod test { let mut data = Bytes::new(&env); data.extend_from_array(&[i, i + 1, i + 2]); - let mut message = Bytes::new(&env); - message.append(&data); - message.append(&salt); - let proof = env.crypto().sha256(&message); + // Compute proof using HMAC-SHA256 + let proof = compute_expected_proof(&env, &data, &salt, &key); proofs.push_back(proof); data_items.push_back(data); @@ -207,20 +311,17 @@ mod test { let mut data_items = Vec::new(&env); let mut salts = Vec::new(&env); - // First proof - valid + // First proof - valid (using HMAC-SHA256) let salt1 = create_test_salt(&env); let mut data1 = Bytes::new(&env); data1.extend_from_array(&[1, 2, 3]); - let mut message1 = Bytes::new(&env); - message1.append(&data1); - message1.append(&salt1); - let proof1 = env.crypto().sha256(&message1); + let proof1 = compute_expected_proof(&env, &data1, &salt1, &key); proofs.push_back(proof1); data_items.push_back(data1); salts.push_back(salt1); - // Second proof - INVALID + // Second proof - INVALID (wrong hash, all zeros) let salt2 = create_test_salt(&env); let mut data2 = Bytes::new(&env); data2.extend_from_array(&[4, 5, 6]);