From d6def48f216b2d6ee8ede40cfa587c6203851322 Mon Sep 17 00:00:00 2001 From: Jayson Grace Date: Fri, 15 May 2026 12:59:29 -0600 Subject: [PATCH 1/3] fix: reject malformed ntlm hashes and add tests for hash validation **Added:** - Validation to reject NTLM hashes that are not exactly 32 hex characters to prevent relay artifacts and partial captures from entering state - Tests to ensure malformed NTLM hashes are rejected and only valid hashes are accepted - Tests to verify non-NTLM hashes (e.g., AES256) of any length are accepted **Changed:** - Updated all NTLM hash test fixtures to use canonical 32-character values for consistency and to avoid confusion with validation logic --- .../state/publishing/credentials.rs | 68 ++++++++++++++++--- 1 file changed, 58 insertions(+), 10 deletions(-) diff --git a/ares-cli/src/orchestrator/state/publishing/credentials.rs b/ares-cli/src/orchestrator/state/publishing/credentials.rs index ec80b230..b86d883c 100644 --- a/ares-cli/src/orchestrator/state/publishing/credentials.rs +++ b/ares-cli/src/orchestrator/state/publishing/credentials.rs @@ -143,6 +143,23 @@ impl SharedState { // Mirrors the credential-side fix in `sanitize_credential`. hash.domain = hash.domain.to_lowercase(); + // Reject malformed NTLM hashes before they enter state. NTLM relay + // artifacts sometimes produce 33-char values (relay timestamp suffix + // or partial capture); sprayhound/secretsdump both hard-fail on + // non-32-char hex, so storing them only causes agent confusion. + if hash.hash_type.to_lowercase() == "ntlm" { + let v = &hash.hash_value; + if v.len() != 32 || !v.chars().all(|c| c.is_ascii_hexdigit()) { + tracing::warn!( + username = %hash.username, + domain = %hash.domain, + hash_len = v.len(), + "Dropping malformed NTLM hash (expected 32 hex chars)" + ); + return Ok(false); + } + } + let operation_id = { let state = self.inner.read().await; state.operation_id.clone() @@ -473,6 +490,9 @@ mod tests { } } + const NTLM_HASH_A: &str = "aad3b435b51404eeaad3b435b51404ee"; // pragma: allowlist secret + const NTLM_HASH_B: &str = "31d6cfe0d16ae931b73c59d7e0c089c0"; // pragma: allowlist secret + fn make_hash(username: &str, domain: &str, hash_type: &str, hash_value: &str) -> Hash { Hash { id: uuid::Uuid::new_v4().to_string(), @@ -740,7 +760,7 @@ mod tests { let state = SharedState::new("op-1".to_string()); let q = mock_queue(); - let hash = make_hash("admin", "contoso.local", "NTLM", "aabbccdd"); + let hash = make_hash("admin", "contoso.local", "NTLM", NTLM_HASH_A); let added = state.publish_hash(&q, hash).await.unwrap(); assert!(added); @@ -754,8 +774,8 @@ mod tests { let state = SharedState::new("op-1".to_string()); let q = mock_queue(); - let hash1 = make_hash("admin", "contoso.local", "NTLM", "aabbccdd"); - let hash2 = make_hash("admin", "contoso.local", "NTLM", "aabbccdd"); + let hash1 = make_hash("admin", "contoso.local", "NTLM", NTLM_HASH_A); + let hash2 = make_hash("admin", "contoso.local", "NTLM", NTLM_HASH_A); assert!(state.publish_hash(&q, hash1).await.unwrap()); assert!(!state.publish_hash(&q, hash2).await.unwrap()); } @@ -767,8 +787,8 @@ mod tests { let state = SharedState::new("op-1".to_string()); let q = mock_queue(); - let upper = make_hash("admin", "CONTOSO.LOCAL", "NTLM", "aabbccdd"); - let lower = make_hash("admin", "contoso.local", "NTLM", "aabbccdd"); + let upper = make_hash("admin", "CONTOSO.LOCAL", "NTLM", NTLM_HASH_A); + let lower = make_hash("admin", "contoso.local", "NTLM", NTLM_HASH_A); assert!(state.publish_hash(&q, upper).await.unwrap()); assert!(!state.publish_hash(&q, lower).await.unwrap()); @@ -788,7 +808,7 @@ mod tests { s.domains.push("contoso.local".to_string()); } - let hash = make_hash("krbtgt", "contoso.local", "NTLM", "aabbccdd11223344"); + let hash = make_hash("krbtgt", "contoso.local", "NTLM", NTLM_HASH_A); state.publish_hash(&q, hash).await.unwrap(); let s = state.inner.read().await; @@ -807,7 +827,7 @@ mod tests { s.domains.push("contoso.local".to_string()); } - let hash = make_hash("krbtgt", "contoso.local", "NTLM", "aabbccdd11223344"); + let hash = make_hash("krbtgt", "contoso.local", "NTLM", NTLM_HASH_A); state.publish_hash(&q, hash).await.unwrap(); let mut conn = q.connection(); @@ -826,7 +846,7 @@ mod tests { let state = SharedState::new("op-1".to_string()); let q = mock_queue(); - let hash = make_hash("krbtgt", "", "NTLM", "aabbccdd11223344"); + let hash = make_hash("krbtgt", "", "NTLM", NTLM_HASH_A); state.publish_hash(&q, hash).await.unwrap(); let s = state.inner.read().await; @@ -844,7 +864,7 @@ mod tests { let state = SharedState::new("op-1".to_string()); let q = mock_queue(); - let hash = make_hash("admin", "contoso.local", "NTLM", "aabbccdd"); + let hash = make_hash("admin", "contoso.local", "NTLM", NTLM_HASH_A); state.publish_hash(&q, hash).await.unwrap(); let updated = state @@ -916,7 +936,7 @@ mod tests { async fn publish_hash_emits_event_with_capturing_recorder() { let (state, recorder) = capturing_state("op-h"); let q = mock_queue(); - let hash = make_hash("admin", "contoso.local", "NTLM", "aabbccdd"); + let hash = make_hash("admin", "contoso.local", "NTLM", NTLM_HASH_A); assert!(state.publish_hash(&q, hash).await.unwrap()); let evs = recorder.captured().await; @@ -937,6 +957,34 @@ mod tests { } } + #[tokio::test] + async fn publish_hash_rejects_malformed_ntlm() { + let state = SharedState::new("op-1".to_string()); + let q = mock_queue(); + + // 33 chars — relay artifact + let bad = make_hash("robb.stark", "north.sevenkingdoms.local", "NTLM", "aad3b435b51404eeaad3b435b51404ee0"); // pragma: allowlist secret + assert!(!state.publish_hash(&q, bad).await.unwrap()); + + // 8 chars — truncated capture + let short = make_hash("robb.stark", "north.sevenkingdoms.local", "NTLM", "aabbccdd"); + assert!(!state.publish_hash(&q, short).await.unwrap()); + + let s = state.inner.read().await; + assert!(s.hashes.is_empty(), "malformed hashes must not enter state"); + } + + #[tokio::test] + async fn publish_hash_accepts_non_ntlm_any_length() { + // AES256 keys are 64 hex chars; we must not reject them. + let state = SharedState::new("op-1".to_string()); + let q = mock_queue(); + let aes = make_hash("krbtgt", "contoso.local", "AES256", "aabbccdd11223344aabbccdd11223344aabbccdd11223344aabbccdd11223344"); + assert!(state.publish_hash(&q, aes).await.unwrap()); + let s = state.inner.read().await; + assert_eq!(s.hashes.len(), 1); + } + #[tokio::test] async fn disabled_recorder_emits_nothing() { // SharedState::new() defaults to OpStateRecorder::Disabled. From 9517867d83ede6dfa0e601904fb06dd7c2e065cb Mon Sep 17 00:00:00 2001 From: Jayson Grace Date: Fri, 15 May 2026 14:23:54 -0600 Subject: [PATCH 2/3] refactor: harden domain admin detection and simplify test formatting **Changed:** - Domain admin indicator detection now only trusts tool-output-backed evidence, specifically a krbtgt hash in the payload; agent self-reporting via `has_domain_admin: true` is ignored to prevent false positives from LLM or agent hallucination (`parsing.rs`, related tests updated) - `resolve_da_path` always returns a fixed path "secretsdump -> krbtgt hash", ignoring agent-provided path fields for safety and consistency; related tests updated to match new logic (`admin_checks.rs`, related tests updated) - Test formatting improved for clarity and consistency, including more compact `.push` and function call lines, and use of multi-line arguments for long values in test helpers (`laps.rs`, `credentials.rs`, `domains.rs`) - Removed unused test constant for NTLM hash B in credentials tests **Removed:** - Unused `NTLM_HASH_B` constant from credentials test module --- ares-cli/src/orchestrator/automation/laps.rs | 9 +++---- .../result_processing/admin_checks.rs | 26 +++++++------------ .../orchestrator/result_processing/parsing.rs | 11 ++++---- .../orchestrator/result_processing/tests.rs | 17 +++++++----- .../state/publishing/credentials.rs | 22 +++++++++++++--- .../orchestrator/state/publishing/domains.rs | 7 +---- 6 files changed, 49 insertions(+), 43 deletions(-) diff --git a/ares-cli/src/orchestrator/automation/laps.rs b/ares-cli/src/orchestrator/automation/laps.rs index 190515ee..ca0fcb6b 100644 --- a/ares-cli/src/orchestrator/automation/laps.rs +++ b/ares-cli/src/orchestrator/automation/laps.rs @@ -441,8 +441,7 @@ mod tests { #[test] fn laps_hash_sweep_emits_work_item_for_valid_ntlm_hash() { let mut s = state_with_dc("contoso.local", "192.168.58.10"); - s.hashes - .push(ntlm_hash("alice", "contoso.local", HASH_A)); + s.hashes.push(ntlm_hash("alice", "contoso.local", HASH_A)); let work = collect_laps_hash_sweep_work(&s); assert_eq!(work.len(), 1); @@ -565,8 +564,7 @@ mod tests { // dedup key go through `.to_lowercase()` — the work item is still // emitted. let mut s = state_with_dc("contoso.local", "192.168.58.10"); - s.hashes - .push(ntlm_hash("Alice", "CONTOSO.LOCAL", HASH_A)); + s.hashes.push(ntlm_hash("Alice", "CONTOSO.LOCAL", HASH_A)); let work = collect_laps_hash_sweep_work(&s); assert_eq!(work.len(), 1); assert_eq!(work[0].dedup_key, "laps_extract:sweep:contoso.local:alice"); @@ -575,8 +573,7 @@ mod tests { #[test] fn laps_hash_sweep_emits_one_item_per_eligible_hash() { let mut s = state_with_dc("contoso.local", "192.168.58.10"); - s.hashes - .push(ntlm_hash("alice", "contoso.local", HASH_A)); + s.hashes.push(ntlm_hash("alice", "contoso.local", HASH_A)); s.hashes.push(ntlm_hash("bob", "contoso.local", HASH_B)); let work = collect_laps_hash_sweep_work(&s); assert_eq!(work.len(), 2); diff --git a/ares-cli/src/orchestrator/result_processing/admin_checks.rs b/ares-cli/src/orchestrator/result_processing/admin_checks.rs index 797fa9e5..3c19a26f 100644 --- a/ares-cli/src/orchestrator/result_processing/admin_checks.rs +++ b/ares-cli/src/orchestrator/result_processing/admin_checks.rs @@ -80,18 +80,8 @@ fn is_valid_domain_fqdn(s: &str) -> bool { } /// Determine the domain admin path from a payload. -/// -/// If `has_domain_admin` is explicitly `true`, returns the `domain_admin_path` -/// string (if present). Otherwise falls back to the secretsdump path. -pub(crate) fn resolve_da_path(payload: &Value) -> Option { - if payload.get("has_domain_admin").and_then(|v| v.as_bool()) == Some(true) { - payload - .get("domain_admin_path") - .and_then(|v| v.as_str()) - .map(|s| s.to_string()) - } else { - Some("secretsdump -> krbtgt hash".to_string()) - } +pub(crate) fn resolve_da_path(_payload: &Value) -> Option { + Some("secretsdump -> krbtgt hash".to_string()) } /// Check if text indicates a golden ticket was saved. @@ -651,21 +641,25 @@ mod tests { // -- resolve_da_path ---------------------------------------------------- #[test] - fn resolve_da_path_explicit_true_with_path() { + fn resolve_da_path_always_secretsdump() { + // Agent-provided path fields are ignored; path is always fixed. let payload = json!({ "has_domain_admin": true, "domain_admin_path": "spray → secretsdump → krbtgt" }); assert_eq!( resolve_da_path(&payload).as_deref(), - Some("spray → secretsdump → krbtgt") + Some("secretsdump -> krbtgt hash") ); } #[test] - fn resolve_da_path_explicit_true_no_path() { + fn resolve_da_path_no_fields() { let payload = json!({ "has_domain_admin": true }); - assert_eq!(resolve_da_path(&payload), None); + assert_eq!( + resolve_da_path(&payload).as_deref(), + Some("secretsdump -> krbtgt hash") + ); } #[test] diff --git a/ares-cli/src/orchestrator/result_processing/parsing.rs b/ares-cli/src/orchestrator/result_processing/parsing.rs index ed5b3a82..1486e093 100644 --- a/ares-cli/src/orchestrator/result_processing/parsing.rs +++ b/ares-cli/src/orchestrator/result_processing/parsing.rs @@ -150,9 +150,9 @@ pub(crate) fn parse_discoveries(payload: &Value) -> ParsedDiscoveries { /// Check if a payload contains domain admin indicators. Pure function. pub(crate) fn has_domain_admin_indicator(payload: &Value) -> bool { - if payload.get("has_domain_admin").and_then(|v| v.as_bool()) == Some(true) { - return true; - } + // Only trust tool-output-backed evidence: a krbtgt hash in the payload. + // Agent self-reporting via `has_domain_admin: true` is not accepted — + // LLMs hallucinate domain admin routinely and it derails post-DA automation. if let Some(hashes) = payload.get("hashes").and_then(|v| v.as_array()) { for hash_val in hashes { if let Some(username) = hash_val.get("username").and_then(|v| v.as_str()) { @@ -173,9 +173,10 @@ mod tests { // ── has_domain_admin_indicator ── #[test] - fn domain_admin_flag_true() { + fn domain_admin_flag_true_ignored() { + // Agent self-reporting is not accepted — must have a krbtgt hash. let payload = json!({"has_domain_admin": true}); - assert!(has_domain_admin_indicator(&payload)); + assert!(!has_domain_admin_indicator(&payload)); } #[test] diff --git a/ares-cli/src/orchestrator/result_processing/tests.rs b/ares-cli/src/orchestrator/result_processing/tests.rs index 5933f0cf..2a5f542b 100644 --- a/ares-cli/src/orchestrator/result_processing/tests.rs +++ b/ares-cli/src/orchestrator/result_processing/tests.rs @@ -251,8 +251,9 @@ fn parse_mixed_payload() { } #[test] -fn da_indicator_explicit_flag() { - assert!(has_domain_admin_indicator( +fn da_indicator_explicit_flag_ignored() { + // Agent self-report is not accepted without a krbtgt hash. + assert!(!has_domain_admin_indicator( &json!({"has_domain_admin": true}) )); } @@ -906,21 +907,25 @@ fn golden_ticket_indicator_both_present_not_adjacent() { // --- resolve_da_path tests --- #[test] -fn da_path_explicit_flag_with_path() { +fn da_path_always_krbtgt() { + // Agent-provided path fields are ignored. let payload = json!({ "has_domain_admin": true, "domain_admin_path": "secretsdump -> Administrator" }); assert_eq!( resolve_da_path(&payload), - Some("secretsdump -> Administrator".to_string()) + Some("secretsdump -> krbtgt hash".to_string()) ); } #[test] -fn da_path_explicit_flag_without_path() { +fn da_path_no_fields_defaults_to_krbtgt() { let payload = json!({"has_domain_admin": true}); - assert_eq!(resolve_da_path(&payload), None); + assert_eq!( + resolve_da_path(&payload), + Some("secretsdump -> krbtgt hash".to_string()) + ); } #[test] diff --git a/ares-cli/src/orchestrator/state/publishing/credentials.rs b/ares-cli/src/orchestrator/state/publishing/credentials.rs index b86d883c..4c87ae2c 100644 --- a/ares-cli/src/orchestrator/state/publishing/credentials.rs +++ b/ares-cli/src/orchestrator/state/publishing/credentials.rs @@ -491,7 +491,6 @@ mod tests { } const NTLM_HASH_A: &str = "aad3b435b51404eeaad3b435b51404ee"; // pragma: allowlist secret - const NTLM_HASH_B: &str = "31d6cfe0d16ae931b73c59d7e0c089c0"; // pragma: allowlist secret fn make_hash(username: &str, domain: &str, hash_type: &str, hash_value: &str) -> Hash { Hash { @@ -963,11 +962,21 @@ mod tests { let q = mock_queue(); // 33 chars — relay artifact - let bad = make_hash("robb.stark", "north.sevenkingdoms.local", "NTLM", "aad3b435b51404eeaad3b435b51404ee0"); // pragma: allowlist secret + let bad = make_hash( + "robb.stark", + "north.sevenkingdoms.local", + "NTLM", + "aad3b435b51404eeaad3b435b51404ee0", + ); // pragma: allowlist secret assert!(!state.publish_hash(&q, bad).await.unwrap()); // 8 chars — truncated capture - let short = make_hash("robb.stark", "north.sevenkingdoms.local", "NTLM", "aabbccdd"); + let short = make_hash( + "robb.stark", + "north.sevenkingdoms.local", + "NTLM", + "aabbccdd", + ); assert!(!state.publish_hash(&q, short).await.unwrap()); let s = state.inner.read().await; @@ -979,7 +988,12 @@ mod tests { // AES256 keys are 64 hex chars; we must not reject them. let state = SharedState::new("op-1".to_string()); let q = mock_queue(); - let aes = make_hash("krbtgt", "contoso.local", "AES256", "aabbccdd11223344aabbccdd11223344aabbccdd11223344aabbccdd11223344"); + let aes = make_hash( + "krbtgt", + "contoso.local", + "AES256", + "aabbccdd11223344aabbccdd11223344aabbccdd11223344aabbccdd11223344", + ); assert!(state.publish_hash(&q, aes).await.unwrap()); let s = state.inner.read().await; assert_eq!(s.hashes.len(), 1); diff --git a/ares-cli/src/orchestrator/state/publishing/domains.rs b/ares-cli/src/orchestrator/state/publishing/domains.rs index 764e3382..a1d0f48a 100644 --- a/ares-cli/src/orchestrator/state/publishing/domains.rs +++ b/ares-cli/src/orchestrator/state/publishing/domains.rs @@ -373,12 +373,7 @@ mod tests { s.domains.push("contoso.local".into()); } let outcome = state - .publish_candidate_domain( - &q, - "evil.local", - DomainEvidence::HostnameInference, - None, - ) + .publish_candidate_domain(&q, "evil.local", DomainEvidence::HostnameInference, None) .await .unwrap(); assert_eq!(outcome, DomainPublishOutcome::Held); From 3754110a47865e3672309a8ad4369cebd0d88499 Mon Sep 17 00:00:00 2001 From: Jayson Grace Date: Fri, 15 May 2026 14:47:06 -0600 Subject: [PATCH 3/3] fix: export S3_BUCKET in test script and update comment wording **Changed:** - Export S3_BUCKET in the test.sh script to ensure it is available to child processes, improving environment setup reliability - Reword comment in Rust parsing module for clarity ("derails" to "breaks") --- ares-cli/src/orchestrator/result_processing/parsing.rs | 2 +- test.sh | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/ares-cli/src/orchestrator/result_processing/parsing.rs b/ares-cli/src/orchestrator/result_processing/parsing.rs index 1486e093..2a4b0ce0 100644 --- a/ares-cli/src/orchestrator/result_processing/parsing.rs +++ b/ares-cli/src/orchestrator/result_processing/parsing.rs @@ -152,7 +152,7 @@ pub(crate) fn parse_discoveries(payload: &Value) -> ParsedDiscoveries { pub(crate) fn has_domain_admin_indicator(payload: &Value) -> bool { // Only trust tool-output-backed evidence: a krbtgt hash in the payload. // Agent self-reporting via `has_domain_admin: true` is not accepted — - // LLMs hallucinate domain admin routinely and it derails post-DA automation. + // LLMs hallucinate domain admin routinely and it breaks post-DA automation. if let Some(hashes) = payload.get("hashes").and_then(|v| v.as_array()) { for hash_val in hashes { if let Some(username) = hash_val.get("username").and_then(|v| v.as_str()) { diff --git a/test.sh b/test.sh index d32762fd..1fe791ac 100755 --- a/test.sh +++ b/test.sh @@ -10,8 +10,7 @@ ulimit -n 65536 || ulimit -n 10240 || true EC2_NAME="${EC2_NAME:-kali-ares}" TARGET="${TARGET:-dreadgoad}" BLUE_ENABLED="${BLUE_ENABLED:-1}" -S3_BUCKET=dread-infra-alpha-operator-range-staging-us-west-1 - +export S3_BUCKET=dread-infra-alpha-operator-range-staging-us-west-1 echo "=== Stopping workers + any running operation ===" task ec2:stop EC2_NAME="${EC2_NAME}" 2>/dev/null || true