From 721c6044ae98b27049c0236aeeba720a48502dc2 Mon Sep 17 00:00:00 2001 From: darkcode123456 Date: Sun, 30 Aug 2026 03:42:54 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20resolve=20issues=20#398-#401=20=E2=80=94?= =?UTF-8?q?=20security=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #398 — Vulnerability allowlist expiry policy - Add expiry date requirements to .cargo/audit.toml allowlist entries - Create scripts/check_allowlist_expiry.py to parse audit.toml/deny.toml and fail the build when any entry is past its expiry date - Add allowlist-expiry-check CI job to .github/workflows/security.yml - Document review cadence, responsible owners, and current allowlist table in docs/vulnerability-scanning.md #399 — ACL permission inheritance tests - Add 6 inheritance/resolution-order tests to backend/src/acl.rs: explicit_deny_overrides_inherited_wildcard_allow, wildcard_deny_overrides_specific_subject_allow, two_level_inheritance_deny_beats_allow_at_every_level, role_with_no_assigned_rules_defaults_to_allow, three_level_wildcard_deny_blocks_all_principals, removing_inherited_deny_restores_access_for_all_principals - Document ACL resolution order in docs/security.md with flow diagram and implications table #400 — Multi-sig threshold change timelock - Add MULTISIG_THRESHOLD_TIMELOCK constant (24 h) to lib.rs - Add PendingThresholdChange struct and PendingMultiSigThreshold DataKey to types.rs - Add 3 event topic constants (ms_t_prp / ms_t_app / ms_t_can) - Implement propose_multisig_threshold / apply_multisig_threshold / cancel_multisig_threshold / get_pending_multisig_threshold functions - Add ContractError variants 127-129 (ThresholdChangePending, ThresholdChangeTimeLocked, NoPendingThresholdChange) - Add multisig_threshold_timelock_tests.rs with 10 tests covering timelock enforcement and cancellation paths - Document the propose→wait→apply flow in docs/multi-sig.md #401 — Push notification payload sanitization - Add sanitize_notif_field(), remove_html_tags(), truncate_to_byte_len() helpers to backend/src/notifications.rs - Apply sanitization to vault_id and passkey_hash in notification_content() - Add NOTIF_FIELD_MAX_LEN constant (128 bytes) - Add 14 tests covering HTML injection, control-character stripping, javascript: URIs, oversized payloads, UTF-8 truncation, and regression tests for previously vulnerable vault_id and passkey_hash fields - Document sanitization policy in docs/push-notifications.md --- .cargo/audit.toml | 11 +- .github/workflows/security.yml | 12 + backend/src/acl.rs | 125 +++++++++ backend/src/notifications.rs | 242 +++++++++++++++++- contracts/ttl_vault/src/lib.rs | 223 ++++++++++++++++ .../src/multisig_threshold_timelock_tests.rs | 234 +++++++++++++++++ contracts/ttl_vault/src/types.rs | 21 ++ docs/multi-sig.md | 82 ++++++ docs/push-notifications.md | 33 +++ docs/security.md | 44 ++++ docs/vulnerability-scanning.md | 51 +++- scripts/check_allowlist_expiry.py | 149 +++++++++++ 12 files changed, 1215 insertions(+), 12 deletions(-) create mode 100644 contracts/ttl_vault/src/multisig_threshold_timelock_tests.rs create mode 100644 scripts/check_allowlist_expiry.py diff --git a/.cargo/audit.toml b/.cargo/audit.toml index e788a01a..3659ee47 100644 --- a/.cargo/audit.toml +++ b/.cargo/audit.toml @@ -6,17 +6,26 @@ # Accepted vulnerability advisories # Format: advisory IDs that have been reviewed and accepted due to legitimate reasons -# Include justification for each acceptance +# Include justification for each acceptance. +# +# ALLOWLIST POLICY (see docs/vulnerability-scanning.md): +# - Every entry MUST include an expiry date comment in the form: expires = "YYYY-MM-DD" +# - Maximum allowlist duration: 90 days from the date the entry was added. +# - The CI `allowlist-expiry-check` job fails the build when an entry is past its expiry. +# - Re-justifying an entry resets the expiry clock (update the date comment). [advisories] ignore = [ # `paste` (unmaintained) is pulled in transitively by soroban-wasmi, part of # the pinned Soroban SDK's WASM interpreter. No newer version exists and it # cannot be removed without a Soroban SDK upgrade. + # No known exploitable code path in this codebase. + # expires = "2026-11-28" "RUSTSEC-2024-0436", # `rand` 0.8.5 (unsound only when combined with a custom logger calling # rand::rng(), which this codebase does not do) is pinned transitively by # soroban-env-host/soroban-sdk and tokio-tungstenite; multiple rand versions # already coexist in the tree and 0.8.5 cannot be removed without an SDK # upgrade. + # expires = "2026-11-28" "RUSTSEC-2026-0097", ] diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 90fac811..cfe32d7d 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -59,6 +59,18 @@ jobs: - name: Run cargo-deny license and advisory check run: cargo deny check advisories licenses + allowlist-expiry-check: + name: Allowlist Expiry Check + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Check vulnerability allowlist expiry dates + run: | + # Fail the build if any allowlisted advisory has passed its review-by date. + # See docs/vulnerability-scanning.md for the allowlist policy. + python3 scripts/check_allowlist_expiry.py + dependency-review: name: Dependency Review runs-on: ubuntu-latest diff --git a/backend/src/acl.rs b/backend/src/acl.rs index 2cf35f61..42c5a55b 100644 --- a/backend/src/acl.rs +++ b/backend/src/acl.rs @@ -275,4 +275,129 @@ mod tests { assert!(!store.remove_rule("does-not-exist", None)); assert!(store.audit_trail().is_empty()); } + + // ── Permission inheritance / resolution-order tests (#399) ───────────── + // + // The ACL resolution order is: + // 1. Collect all rules whose subject/resource/action globs match the + // request. + // 2. If ANY matching rule has effect=Deny → DENY (deny always wins). + // 3. Otherwise → ALLOW (or default-allow when no rules match at all). + // + // "Inheritance" in this codebase is modelled via wildcard subjects: + // - A rule with subject="*" applies to every principal (base/parent role). + // - A rule with subject="alice" applies only to alice (specific override). + // - A deny on a more-specific subject overrides a wildcard allow, and + // vice-versa (a wildcard deny overrides a specific allow). + // + // This mirrors a two-level inheritance chain: + // parent role → wildcard rule ("*") + // child role → subject rule ("alice") + + /// A global allow ("*") combined with a per-user deny: deny wins. + /// This exercises the "explicit deny always overrides inherited allow" rule. + #[test] + fn explicit_deny_overrides_inherited_wildcard_allow() { + let store = AclStore::default(); + // Parent / inherited allow: everyone may GET /api/reports + store.add_rule(rule_req("*", "/api/reports", "GET", AclEffect::Allow)); + // Per-user deny for alice on the same resource + store.add_rule(rule_req("alice", "/api/reports", "GET", AclEffect::Deny)); + + // alice's explicit deny overrides the wildcard allow + assert!(!store.is_allowed("alice", "/api/reports", "GET")); + // bob only has the wildcard allow — should be permitted + assert!(store.is_allowed("bob", "/api/reports", "GET")); + } + + /// A per-user allow does NOT save a principal when a wildcard deny is present. + /// A global deny ("*") cannot be escaped by a subject-specific allow. + #[test] + fn wildcard_deny_overrides_specific_subject_allow() { + let store = AclStore::default(); + // Global deny: nobody may DELETE /api/vaults + store.add_rule(rule_req("*", "/api/vaults", "DELETE", AclEffect::Deny)); + // Per-user allow: alice explicitly granted DELETE + store.add_rule(rule_req("alice", "/api/vaults", "DELETE", AclEffect::Allow)); + + // Global deny wins even though alice has a specific allow + assert!(!store.is_allowed("alice", "/api/vaults", "DELETE")); + assert!(!store.is_allowed("bob", "/api/vaults", "DELETE")); + } + + /// Two-level inheritance chain: + /// Level 1 (grandparent): wildcard allow for everything + /// Level 2 (parent role): wildcard deny for admin area + /// Level 3 (child role / specific user): explicit allow for one admin path + /// + /// Expected: deny at level 2 still wins for alice; bob is also denied. + #[test] + fn two_level_inheritance_deny_beats_allow_at_every_level() { + let store = AclStore::default(); + // Level 1 – everyone may access everything + store.add_rule(rule_req("*", "/", "GET", AclEffect::Allow)); + // Level 2 – no one may access /api/admin + store.add_rule(rule_req("*", "/api/admin", "*", AclEffect::Deny)); + // Level 3 – alice has a targeted allow on /api/admin/users + store.add_rule(rule_req( + "alice", + "/api/admin/users", + "GET", + AclEffect::Allow, + )); + + // Level-2 wildcard deny covers /api/admin/* via prefix matching → alice + // is still denied even with the level-3 allow. + assert!(!store.is_allowed("alice", "/api/admin/users", "GET")); + assert!(!store.is_allowed("bob", "/api/admin/users", "GET")); + // Paths outside /api/admin are unaffected by the level-2 deny + assert!(store.is_allowed("alice", "/api/vaults", "GET")); + } + + /// A role with zero assigned rules falls back to the default-allow behaviour. + #[test] + fn role_with_no_assigned_rules_defaults_to_allow() { + let store = AclStore::default(); + // Add some rules for other subjects — carol has none + store.add_rule(rule_req("alice", "/api/admin", "*", AclEffect::Deny)); + store.add_rule(rule_req("bob", "/api/reports", "GET", AclEffect::Allow)); + + // carol has no rules at all → default allow + assert!(store.is_allowed("carol", "/api/vaults", "GET")); + assert!(store.is_allowed("carol", "/api/admin", "POST")); + assert!(store.is_allowed("carol", "/api/reports", "DELETE")); + } + + /// Three-level wildcard inheritance: + /// - Action wildcard ("*") on the parent covers all methods. + /// - Resource wildcard ("*") on the grandparent covers all resources. + /// - Subject wildcard ("*") on the root covers all principals. + /// Confirms that a deny propagates correctly through all three wildcard + /// dimensions simultaneously. + #[test] + fn three_level_wildcard_deny_blocks_all_principals_resources_actions() { + let store = AclStore::default(); + // One absolute deny rule + store.add_rule(rule_req("*", "*", "*", AclEffect::Deny)); + + assert!(!store.is_allowed("anyone", "/any/path", "GET")); + assert!(!store.is_allowed("anyone", "/any/path", "POST")); + assert!(!store.is_allowed("root", "/api/admin", "DELETE")); + } + + /// Removing a deny rule that was "inherited" (wildcard) immediately + /// restores access for all principals. + #[test] + fn removing_inherited_deny_restores_access_for_all_principals() { + let store = AclStore::default(); + let deny = store.add_rule(rule_req("*", "/api/beta", "*", AclEffect::Deny)); + + assert!(!store.is_allowed("alice", "/api/beta/feature", "GET")); + assert!(!store.is_allowed("bob", "/api/beta/feature", "GET")); + + store.remove_rule(&deny.id, Some("admin".to_string())); + + assert!(store.is_allowed("alice", "/api/beta/feature", "GET")); + assert!(store.is_allowed("bob", "/api/beta/feature", "GET")); + } } diff --git a/backend/src/notifications.rs b/backend/src/notifications.rs index a85c7c57..28ad53ae 100644 --- a/backend/src/notifications.rs +++ b/backend/src/notifications.rs @@ -136,6 +136,76 @@ impl FcmClient { } } +// ── Payload sanitization (Issue #401) ───────────────────────────────────────── + +/// Maximum byte length for a user-controlled field included in a notification +/// payload. Fields that exceed this limit are truncated at the nearest +/// character boundary. +pub const NOTIF_FIELD_MAX_LEN: usize = 128; + +/// Sanitize a user-controlled string for safe inclusion in a notification payload. +/// +/// This function: +/// 1. Strips ASCII control characters (U+0000–U+001F and U+007F) that have no +/// legitimate use in display text but could be exploited to inject escape +/// sequences into notification-rendering engines. +/// 2. Removes HTML/XML angle-bracket tags (`<…>`) to prevent HTML injection in +/// web notification renderers. +/// 3. Removes JavaScript protocol URIs (`javascript:`) that some renderers may +/// treat as active links. +/// 4. Truncates the result to `NOTIF_FIELD_MAX_LEN` bytes (preserving UTF-8 +/// character boundaries) to prevent oversized payloads that could cause +/// denial-of-service in downstream renderers. +/// +/// The output is safe to embed in FCM `notification.title`, `notification.body`, +/// or `data` fields. +pub fn sanitize_notif_field(input: &str) -> String { + // 1. Strip control characters. + let no_ctrl: String = input + .chars() + .filter(|&c| !c.is_ascii_control() || c == '\t') + .collect(); + + // 2. Remove HTML/XML tags. + let no_tags = remove_html_tags(&no_ctrl); + + // 3. Remove javascript: URIs (case-insensitive). + let no_js = no_tags.replace("javascript:", "").replace("JAVASCRIPT:", ""); + + // 4. Truncate to NOTIF_FIELD_MAX_LEN bytes at a valid UTF-8 boundary. + truncate_to_byte_len(&no_js, NOTIF_FIELD_MAX_LEN) +} + +/// Remove HTML/XML tags from `input` using a simple state machine. +/// This handles nested tags and attributes without pulling in an HTML parser. +fn remove_html_tags(input: &str) -> String { + let mut result = String::with_capacity(input.len()); + let mut in_tag = false; + for ch in input.chars() { + match ch { + '<' => in_tag = true, + '>' if in_tag => in_tag = false, + _ if !in_tag => result.push(ch), + _ => {} + } + } + result +} + +/// Truncate `s` to at most `max_bytes` bytes, respecting UTF-8 character +/// boundaries (i.e., never splitting a multi-byte character). +fn truncate_to_byte_len(s: &str, max_bytes: usize) -> String { + if s.len() <= max_bytes { + return s.to_string(); + } + // Walk backward from max_bytes to find a valid char boundary. + let mut end = max_bytes; + while end > 0 && !s.is_char_boundary(end) { + end -= 1; + } + s[..end].to_string() +} + // ── Notification content helpers ───────────────────────────────────────────── fn notification_content( @@ -144,47 +214,52 @@ fn notification_content( ttl_hours: Option, passkey_hash: Option<&str>, ) -> (&'static str, String, Value) { + // All user-controlled fields are sanitized before being embedded in the + // notification payload (Issue #401). + let safe_vault_id = sanitize_notif_field(vault_id); + let safe_passkey_hash = passkey_hash.map(|h| sanitize_notif_field(h)); + match notification_type { NotificationType::ExpiryWarning => { let hours = ttl_hours.unwrap_or(24); ( "⚠️ Vault Expiring Soon", format!("Your vault expires in ~{hours}h. Check in now to keep it active."), - json!({ "type": "expiry_warning", "vault_id": vault_id }), + json!({ "type": "expiry_warning", "vault_id": safe_vault_id }), ) } NotificationType::CheckInReminder => ( "🔔 Check-In Reminder", "Time to check in to your Ethos-Protocol vault.".to_string(), - json!({ "type": "check_in_reminder", "vault_id": vault_id }), + json!({ "type": "check_in_reminder", "vault_id": safe_vault_id }), ), NotificationType::VaultReleased => ( "🔓 Vault Released", "Your vault has been released to the beneficiary.".to_string(), - json!({ "type": "vault_released", "vault_id": vault_id }), + json!({ "type": "vault_released", "vault_id": safe_vault_id }), ), NotificationType::VaultPaused => ( "⏸ Vault Paused", "Your vault has been paused.".to_string(), - json!({ "type": "vault_paused", "vault_id": vault_id }), + json!({ "type": "vault_paused", "vault_id": safe_vault_id }), ), NotificationType::PasskeyExpiringSoon => { let hours = ttl_hours.unwrap_or(24); - let short_hash = truncated_passkey_hash(passkey_hash); + let short_hash = truncated_passkey_hash(safe_passkey_hash.as_deref()); ( "🔑 Passkey Expiring Soon", format!( - "Passkey {short_hash} on vault {vault_id} expires in ~{hours}h. Rotate or extend it to keep access." + "Passkey {short_hash} on vault {safe_vault_id} expires in ~{hours}h. Rotate or extend it to keep access." ), - json!({ "type": "passkey_expiring_soon", "vault_id": vault_id, "passkey_hash": passkey_hash }), + json!({ "type": "passkey_expiring_soon", "vault_id": safe_vault_id, "passkey_hash": safe_passkey_hash }), ) } NotificationType::PasskeyExpired => { - let short_hash = truncated_passkey_hash(passkey_hash); + let short_hash = truncated_passkey_hash(safe_passkey_hash.as_deref()); ( "🔑 Passkey Expired", - format!("Passkey {short_hash} on vault {vault_id} has expired."), - json!({ "type": "passkey_expired", "vault_id": vault_id, "passkey_hash": passkey_hash }), + format!("Passkey {short_hash} on vault {safe_vault_id} has expired."), + json!({ "type": "passkey_expired", "vault_id": safe_vault_id, "passkey_hash": safe_passkey_hash }), ) } } @@ -1503,4 +1578,151 @@ mod tests { assert_eq!(all.len(), 1); assert_eq!(all[0].max_retry_attempts, DEFAULT_MAX_RETRY_ATTEMPTS); } + + // ── Payload sanitization tests (Issue #401) ─────────────────────────────── + + /// Normal vault IDs pass through unchanged. + #[test] + fn sanitize_normal_vault_id_unchanged() { + let id = "vault-abc-123"; + assert_eq!(sanitize_notif_field(id), id); + } + + /// A vault ID containing an HTML script tag has the tag stripped. + #[test] + fn sanitize_strips_html_script_tag() { + let malicious = "v1"; + let result = sanitize_notif_field(malicious); + assert!(!result.contains('<'), "angle brackets must be removed"); + assert!(!result.contains('>'), "angle brackets must be removed"); + assert!(!result.contains("script"), "script tag content must be removed"); + assert!(result.contains("v1"), "safe prefix must be preserved"); + } + + /// HTML injection via an img tag is sanitized. + #[test] + fn sanitize_strips_html_img_tag() { + let malicious = "vaultid"; + let result = sanitize_notif_field(malicious); + assert!(!result.contains('<')); + assert!(!result.contains('>')); + assert!(result.contains("vaultid"), "text outside tags must be preserved"); + } + + /// ASCII control characters (null bytes, carriage returns, etc.) are removed. + #[test] + fn sanitize_strips_control_characters() { + let with_ctrl = "vault\x00\x01\x1f\x7fid"; + let result = sanitize_notif_field(with_ctrl); + assert!(!result.contains('\x00')); + assert!(!result.contains('\x01')); + assert!(!result.contains('\x1f')); + assert!(!result.contains('\x7f')); + assert!(result.contains("vault")); + assert!(result.contains("id")); + } + + /// Newline and carriage-return characters (common injection vectors) are stripped. + #[test] + fn sanitize_strips_newlines_and_carriage_returns() { + let with_newlines = "vault\nid\r\n"; + let result = sanitize_notif_field(with_newlines); + assert!(!result.contains('\n'), "newlines must be removed"); + assert!(!result.contains('\r'), "carriage returns must be removed"); + } + + /// `javascript:` URIs are removed to prevent active-link injection. + #[test] + fn sanitize_removes_javascript_uri() { + let js_uri = "javascript:alert(document.cookie)"; + let result = sanitize_notif_field(js_uri); + assert!(!result.to_lowercase().contains("javascript:"), + "javascript: URI scheme must be removed"); + } + + /// Fields longer than NOTIF_FIELD_MAX_LEN bytes are truncated. + #[test] + fn sanitize_truncates_oversized_field() { + let long_input: String = "a".repeat(NOTIF_FIELD_MAX_LEN + 50); + let result = sanitize_notif_field(&long_input); + assert!( + result.len() <= NOTIF_FIELD_MAX_LEN, + "result must not exceed NOTIF_FIELD_MAX_LEN bytes; got {}", + result.len() + ); + } + + /// Fields exactly at the max length pass through unchanged. + #[test] + fn sanitize_field_at_max_length_unchanged() { + let at_limit: String = "b".repeat(NOTIF_FIELD_MAX_LEN); + let result = sanitize_notif_field(&at_limit); + assert_eq!(result.len(), NOTIF_FIELD_MAX_LEN); + assert_eq!(result, at_limit); + } + + /// Truncation respects UTF-8 character boundaries (no split multi-byte chars). + #[test] + fn sanitize_truncation_respects_utf8_boundary() { + // Each '€' is 3 bytes in UTF-8; fill to just over the limit with them. + let euros: String = "€".repeat(50); // 150 bytes > 128 + let result = sanitize_notif_field(&euros); + // Result must be valid UTF-8 (not panic on re-encode). + assert!(std::str::from_utf8(result.as_bytes()).is_ok()); + assert!(result.len() <= NOTIF_FIELD_MAX_LEN); + } + + /// A passkey hash containing injection characters is sanitized when used + /// in a PasskeyExpired notification (regression test for previously + /// vulnerable field: passkey_hash). + #[test] + fn regression_passkey_hash_field_is_sanitized_in_payload() { + let malicious_hash = "abcdef"; + let (_, body, data) = notification_content( + &NotificationType::PasskeyExpired, + "safe-vault-id", + None, + Some(malicious_hash), + ); + // The passkey_hash value in the JSON data field must not contain raw tags. + let data_str = data.to_string(); + assert!(!data_str.contains("