From 482ebfe1e41c508535a5bdf1babc68256e5e364a Mon Sep 17 00:00:00 2001 From: Michael Mera Date: Fri, 22 Mar 2024 09:45:12 +0100 Subject: [PATCH] fix CI deadlock when testing known vulnerabilities (#306) * test(tlspuffin): add timeout argument to forked execution * test(tlspuffin): retry several times when testing known vulnerabilities --- puffin/src/cli.rs | 52 ++-- puffin/src/execution.rs | 83 ++++++ puffin/src/lib.rs | 1 + tlspuffin/src/integration_tests/mutations.rs | 277 ++++++++++--------- tlspuffin/src/test_utils.rs | 105 ++++--- tlspuffin/src/tls/vulnerabilities.rs | 43 ++- 6 files changed, 345 insertions(+), 216 deletions(-) create mode 100644 puffin/src/execution.rs diff --git a/puffin/src/cli.rs b/puffin/src/cli.rs index 659bb63e3..bad5e6d5f 100644 --- a/puffin/src/cli.rs +++ b/puffin/src/cli.rs @@ -15,6 +15,7 @@ use log::{error, info, LevelFilter}; use crate::{ algebra::set_deserialize_signature, codec::Codec, + execution::forked_execution, experiment::*, fuzzer::{ harness::{default_put_options, set_default_put_options}, @@ -389,33 +390,11 @@ fn seed( Ok(()) } -use nix::{ - sys::wait::{waitpid, WaitPidFlag}, - unistd::{fork, ForkResult}, -}; - use crate::{ agent::AgentName, put::{PutDescriptor, PutName}, }; -pub fn expect_crash(func: R) -where - R: FnOnce(), -{ - match unsafe { fork() } { - Ok(ForkResult::Parent { child, .. }) => { - let status = waitpid(child, Option::from(WaitPidFlag::empty())).unwrap(); - info!("Finished executing: {:?}", status); - } - Ok(ForkResult::Child) => { - func(); - std::process::exit(0); - } - Err(_) => panic!("Fork failed"), - } -} - fn execute>(input: P, put_registry: &'static PutRegistry) { let trace = match Trace::::from_file(input.as_ref()) { Ok(t) => t, @@ -429,16 +408,25 @@ fn execute>(input: P, put_registry: &'stati // When generating coverage a crash means that no coverage is stored // By executing in a fork, even when that process crashes, the other executed code will still yield coverage - expect_crash(move || { - let mut ctx = TraceContext::new(put_registry, default_put_options().clone()); - if let Err(err) = trace.execute(&mut ctx) { - error!( - "Failed to execute trace {}: {:?}", - input.as_ref().display(), - err - ); - } - }); + let status = forked_execution( + move || { + let mut ctx = TraceContext::new(put_registry, default_put_options().clone()); + if let Err(err) = trace.execute(&mut ctx) { + error!( + "Failed to execute trace {}: {:?}", + input.as_ref().display(), + err + ); + std::process::exit(1); + } + }, + None, + ); + + match status { + Ok(s) => info!("execution finished with status {s:?}"), + Err(reason) => panic!("failed to execute trace: {reason}"), + } } fn binary_attack( diff --git a/puffin/src/execution.rs b/puffin/src/execution.rs new file mode 100644 index 000000000..d8d307374 --- /dev/null +++ b/puffin/src/execution.rs @@ -0,0 +1,83 @@ +use std::{thread, time::Duration}; + +use nix::{ + sys::{ + signal::{kill, Signal}, + wait::{ + waitpid, WaitPidFlag, + WaitStatus::{Exited, Signaled}, + }, + }, + unistd::{fork, ForkResult, Pid}, +}; + +pub fn forked_execution(func: R, timeout: Option) -> Result +where + R: FnOnce(), +{ + match unsafe { fork() } { + Ok(ForkResult::Parent { child, .. }) => { + let status = waitpid(child, Option::from(WaitPidFlag::empty())).unwrap(); + + if let Signaled(_, signal, _) = status { + match signal { + Signal::SIGSEGV | Signal::SIGABRT => return Ok(ExecutionStatus::Crashed), + Signal::SIGUSR2 if timeout.is_some() => return Ok(ExecutionStatus::Timeout), + _ => { + return Err(format!( + "execution process finished with unexpected signal {}", + signal + )) + } + } + } else if let Exited(_, code) = status { + if code == 0 { + return Ok(ExecutionStatus::Success); + } else { + return Ok(ExecutionStatus::Failure(code)); + } + } + + Err(format!( + "execution process finished with unexpected status {:?}", + status + )) + } + Ok(ForkResult::Child) => { + if let Some(t) = timeout { + thread::spawn(move || { + thread::sleep(t); + kill(Pid::this(), Signal::SIGUSR2).ok(); + }); + } + + func(); + std::process::exit(0); + } + Err(e) => Err(format!("fork failed: {}", e)), + } +} + +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub enum ExecutionStatus { + Timeout, + Crashed, + Success, + Failure(i32), +} + +pub trait AssertExecution { + fn expect_crash(self); +} + +impl AssertExecution for Result { + fn expect_crash(self) { + use ExecutionStatus as S; + match self { + Ok(S::Failure(_)) | Ok(S::Crashed) => (), + Ok(S::Timeout) => panic!("trace execution timed out"), + Ok(S::Success) => panic!("expected trace execution to crash, but succeeded"), + Err(reason) => panic!("trace execution error: {reason}"), + } + } +} diff --git a/puffin/src/lib.rs b/puffin/src/lib.rs index 378db3723..18d87c44c 100644 --- a/puffin/src/lib.rs +++ b/puffin/src/lib.rs @@ -6,6 +6,7 @@ pub mod claims; pub mod cli; pub mod codec; pub mod error; +pub mod execution; pub mod experiment; pub mod fuzzer; pub mod graphviz; diff --git a/tlspuffin/src/integration_tests/mutations.rs b/tlspuffin/src/integration_tests/mutations.rs index 487a1fb53..7dacbc3be 100644 --- a/tlspuffin/src/integration_tests/mutations.rs +++ b/tlspuffin/src/integration_tests/mutations.rs @@ -1,6 +1,7 @@ use puffin::{ agent::AgentName, algebra::{dynamic_function::DescribableFunction, Term}, + execution::{forked_execution, AssertExecution}, fuzzer::mutations::{ util::TermConstraints, RemoveAndLiftMutator, RepeatMutator, ReplaceMatchMutator, ReplaceReuseMutator, @@ -18,7 +19,6 @@ use puffin::{ use crate::{ put_registry::TLS_PUT_REGISTRY, query::TlsQueryMatcher, - test_utils::expect_crash, tls::{ fn_impl::{ fn_client_hello, fn_encrypt12, fn_seq_1, fn_sign_transcript, @@ -46,133 +46,167 @@ fn test_mutate_seed_cve_2021_3449() { let mut state = create_state(); let _server = AgentName::first(); - expect_crash(move || { - for _i in 0..5 { - let mut attempts = 0; + forked_execution( + move || { + for _i in 0..5 { + let mut attempts = 0; - let (mut trace, _) = _seed_client_attacker12(AgentName::first()); + let (mut trace, _) = _seed_client_attacker12(AgentName::first()); - // Check if we can append another encrypted message + // Check if we can append another encrypted message - let mut mutator = RepeatMutator::new(15); + let mut mutator = RepeatMutator::new(15); - fn check_is_encrypt12(step: &Step) -> bool { - if let Action::Input(input) = &step.action { - if input.recipe.name() == fn_encrypt12.name() { - return true; + fn check_is_encrypt12(step: &Step) -> bool { + if let Action::Input(input) = &step.action { + if input.recipe.name() == fn_encrypt12.name() { + return true; + } } + false } - false - } - loop { - attempts += 1; - let mut mutate = trace.clone(); - mutator.mutate(&mut state, &mut mutate, 0).unwrap(); - - let length = mutate.steps.len(); - if let Some(last) = mutate.steps.get(length - 1) { - if check_is_encrypt12(last) { - if let Some(step) = mutate.steps.get(length - 2) { - if check_is_encrypt12(step) { - trace = mutate; - break; + loop { + attempts += 1; + let mut mutate = trace.clone(); + mutator.mutate(&mut state, &mut mutate, 0).unwrap(); + + let length = mutate.steps.len(); + if let Some(last) = mutate.steps.get(length - 1) { + if check_is_encrypt12(last) { + if let Some(step) = mutate.steps.get(length - 2) { + if check_is_encrypt12(step) { + trace = mutate; + break; + } } } } } - } - println!("attempts 1: {}", attempts); - attempts = 0; - - // Check if we have a client hello in last encrypted one - - let constraints = TermConstraints { - min_term_size: 0, - max_term_size: 300, - }; - let mut mutator = ReplaceReuseMutator::new(constraints); - - loop { - attempts += 1; - let mut mutate = trace.clone(); - mutator.mutate(&mut state, &mut mutate, 0).unwrap(); - - if let Some(last) = mutate.steps.iter().last() { - match &last.action { - Action::Input(input) => match &input.recipe { - Term::Variable(_) => {} - Term::Application(_, subterms) => { - if let Some(first_subterm) = subterms.iter().next() { - if first_subterm.name() == fn_client_hello.name() { - trace = mutate; - break; + println!("attempts 1: {}", attempts); + attempts = 0; + + // Check if we have a client hello in last encrypted one + + let constraints = TermConstraints { + min_term_size: 0, + max_term_size: 300, + }; + let mut mutator = ReplaceReuseMutator::new(constraints); + + loop { + attempts += 1; + let mut mutate = trace.clone(); + mutator.mutate(&mut state, &mut mutate, 0).unwrap(); + + if let Some(last) = mutate.steps.iter().last() { + match &last.action { + Action::Input(input) => match &input.recipe { + Term::Variable(_) => {} + Term::Application(_, subterms) => { + if let Some(first_subterm) = subterms.iter().next() { + if first_subterm.name() == fn_client_hello.name() { + trace = mutate; + break; + } } } - } - }, - Action::Output(_) => {} + }, + Action::Output(_) => {} + } } } - } - println!("attempts 2: {}", attempts); - attempts = 0; - - // Test if we can replace the sequence number - - let mut mutator = ReplaceMatchMutator::new(constraints, &TLS_SIGNATURE); - - loop { - attempts += 1; - let mut mutate = trace.clone(); - mutator.mutate(&mut state, &mut mutate, 0).unwrap(); - - if let Some(last) = mutate.steps.iter().last() { - match &last.action { - Action::Input(input) => match &input.recipe { - Term::Variable(_) => {} - Term::Application(_, subterms) => { - if let Some(last_subterm) = subterms.iter().last() { - if last_subterm.name() == fn_seq_1.name() { - trace = mutate; - break; + println!("attempts 2: {}", attempts); + attempts = 0; + + // Test if we can replace the sequence number + + let mut mutator = ReplaceMatchMutator::new(constraints, &TLS_SIGNATURE); + + loop { + attempts += 1; + let mut mutate = trace.clone(); + mutator.mutate(&mut state, &mut mutate, 0).unwrap(); + + if let Some(last) = mutate.steps.iter().last() { + match &last.action { + Action::Input(input) => match &input.recipe { + Term::Variable(_) => {} + Term::Application(_, subterms) => { + if let Some(last_subterm) = subterms.iter().last() { + if last_subterm.name() == fn_seq_1.name() { + trace = mutate; + break; + } } } + }, + Action::Output(_) => {} + } + } + } + println!("attempts 3: {}", attempts); + attempts = 0; + + // Remove sig algo + + let mut mutator = RemoveAndLiftMutator::new(constraints); + + loop { + attempts += 1; + let mut mutate = trace.clone(); + let result = mutator.mutate(&mut state, &mut mutate, 0).unwrap(); + + if let MutationResult::Mutated = result { + if let Some(last) = mutate.steps.iter().last() { + match &last.action { + Action::Input(input) => match &input.recipe { + Term::Variable(_) => {} + Term::Application(_, subterms) => { + if let Some(first_subterm) = subterms.iter().next() { + let sig_alg_extensions = first_subterm + .count_functions_by_name( + fn_signature_algorithm_extension.name(), + ); + let support_groups_extensions = first_subterm + .count_functions_by_name( + fn_support_group_extension.name(), + ); + if sig_alg_extensions == 0 + && support_groups_extensions == 1 + { + trace = mutate; + break; + } + } + } + }, + Action::Output(_) => {} } - }, - Action::Output(_) => {} + } } } - } - println!("attempts 3: {}", attempts); - attempts = 0; + println!("attempts 4: {}", attempts); + attempts = 0; - // Remove sig algo + // Sucessfully renegotiate - let mut mutator = RemoveAndLiftMutator::new(constraints); + let mut mutator = ReplaceReuseMutator::new(constraints); - loop { - attempts += 1; - let mut mutate = trace.clone(); - let result = mutator.mutate(&mut state, &mut mutate, 0).unwrap(); + loop { + attempts += 1; + let mut mutate = trace.clone(); + mutator.mutate(&mut state, &mut mutate, 0).unwrap(); - if let MutationResult::Mutated = result { if let Some(last) = mutate.steps.iter().last() { match &last.action { Action::Input(input) => match &input.recipe { Term::Variable(_) => {} Term::Application(_, subterms) => { if let Some(first_subterm) = subterms.iter().next() { - let sig_alg_extensions = first_subterm - .count_functions_by_name( - fn_signature_algorithm_extension.name(), - ); - let support_groups_extensions = first_subterm - .count_functions_by_name( - fn_support_group_extension.name(), - ); - if sig_alg_extensions == 0 && support_groups_extensions == 1 - { + let signatures = first_subterm + .count_functions_by_name(fn_sign_transcript.name()); + if signatures == 1 { trace = mutate; break; } @@ -183,43 +217,14 @@ fn test_mutate_seed_cve_2021_3449() { } } } - } - println!("attempts 4: {}", attempts); - attempts = 0; - - // Sucessfully renegotiate - - let mut mutator = ReplaceReuseMutator::new(constraints); - - loop { - attempts += 1; - let mut mutate = trace.clone(); - mutator.mutate(&mut state, &mut mutate, 0).unwrap(); - - if let Some(last) = mutate.steps.iter().last() { - match &last.action { - Action::Input(input) => match &input.recipe { - Term::Variable(_) => {} - Term::Application(_, subterms) => { - if let Some(first_subterm) = subterms.iter().next() { - let signatures = first_subterm - .count_functions_by_name(fn_sign_transcript.name()); - if signatures == 1 { - trace = mutate; - break; - } - } - } - }, - Action::Output(_) => {} - } - } - } - println!("attempts 5: {}", attempts); + println!("attempts 5: {}", attempts); - let mut context = TraceContext::new(&TLS_PUT_REGISTRY, PutOptions::default()); - let _ = trace.execute(&mut context); - println!("try"); - } - }); + let mut context = TraceContext::new(&TLS_PUT_REGISTRY, PutOptions::default()); + let _ = trace.execute(&mut context); + println!("try"); + } + }, + Some(std::time::Duration::from_secs(30)), + ) + .expect_crash(); } diff --git a/tlspuffin/src/test_utils.rs b/tlspuffin/src/test_utils.rs index 92e5268f2..a44fef036 100644 --- a/tlspuffin/src/test_utils.rs +++ b/tlspuffin/src/test_utils.rs @@ -1,49 +1,70 @@ -use nix::{ - sys::{ - signal::Signal, - wait::{ - waitpid, WaitPidFlag, - WaitStatus::{Exited, Signaled}, - }, - }, - unistd::{fork, ForkResult}, +use std::time::Duration; + +use log::info; +use puffin::{ + execution::{forked_execution, ExecutionStatus}, + put::PutOptions, + trace::Trace, }; -use puffin::{put::PutOptions, trace::Trace}; use crate::{put_registry::TLS_PUT_REGISTRY, query::TlsQueryMatcher}; +// TODO refactor forked execution into a build pattern +// +// Because we now have several optional arguments to execute a trace and +// several more in [`forked_execution()`], the API is difficult to read at +// call site. +// +// It would make sense to group everything into a builder pattern for +// creating an trace execution. This would give something like: +// +// Execution::builder(trace, options) +// .timeout(Duration::from_secs(10)) +// .retry(5) +// .expect_crash() #[allow(dead_code)] -pub fn expect_trace_crash(trace: Trace, default_put_options: PutOptions) { - expect_crash(move || { - // Ignore Rust errors - let _ = trace.execute_deterministic(&TLS_PUT_REGISTRY, default_put_options); - }); -} - -pub fn expect_crash(func: R) -where - R: FnOnce(), -{ - match unsafe { fork() } { - Ok(ForkResult::Parent { child, .. }) => { - let status = waitpid(child, Option::from(WaitPidFlag::empty())).unwrap(); +pub fn expect_trace_crash( + trace: Trace, + default_put_options: PutOptions, + timeout: Option, + retry: Option, +) { + let nb_retry = retry.unwrap_or(1); - if let Signaled(_, signal, _) = status { - if signal != Signal::SIGSEGV && signal != Signal::SIGABRT { - panic!("Trace did not crash with SIGSEGV/SIGABRT!") - } - } else if let Exited(_, code) = status { - if code == 0 { - panic!("Trace did not crash exit with non-zero code (AddressSanitizer)!") - } - } else { - panic!("Trace did not signal!") - } - } - Ok(ForkResult::Child) => { - func(); - std::process::exit(0); - } - Err(_) => panic!("Fork failed"), - } + let _ = std::iter::repeat(()) + .take(nb_retry) + .map(|_| { + forked_execution( + || { + // Ignore Rust errors + let _ = trace + .clone() + .execute_deterministic(&TLS_PUT_REGISTRY, default_put_options.clone()); + }, + timeout, + ) + }) + .map(|status| { + use ExecutionStatus as S; + match &status { + Ok(S::Failure(_)) | Ok(S::Crashed) => info!("trace execution crashed"), + Ok(S::Timeout) => info!("trace execution timed out"), + Ok(S::Success) => info!("expected trace execution to crash, but succeeded"), + Err(reason) => info!("trace execution error: {reason}"), + }; + status + }) + .take_while(|status| { + matches!( + status, + Ok(ExecutionStatus::Failure(_)) | Ok(ExecutionStatus::Crashed) + ) + }) + .next() + .unwrap_or_else(|| { + panic!( + "expected trace execution to crash (retried {} times)", + nb_retry + ) + }); } diff --git a/tlspuffin/src/tls/vulnerabilities.rs b/tlspuffin/src/tls/vulnerabilities.rs index d6415537f..dbab0d94a 100644 --- a/tlspuffin/src/tls/vulnerabilities.rs +++ b/tlspuffin/src/tls/vulnerabilities.rs @@ -1107,13 +1107,18 @@ pub mod tests { #[cfg(all(feature = "openssl101-binding", feature = "asan"))] #[cfg(feature = "tls12")] #[test] - #[ignore] // We can not check for this vulnerability right now + #[ignore] // We cannot check for this vulnerability right now fn test_seed_freak() { use puffin::put::PutOptions; use crate::test_utils::expect_trace_crash; - expect_trace_crash(seed_freak.build_trace(), PutOptions::default()); + expect_trace_crash( + seed_freak.build_trace(), + PutOptions::default(), + Some(std::time::Duration::from_secs(5)), + Some(5), + ); } #[cfg(all(feature = "openssl101-binding", feature = "asan"))] @@ -1124,7 +1129,12 @@ pub mod tests { use crate::test_utils::expect_trace_crash; - expect_trace_crash(seed_heartbleed.build_trace(), PutOptions::default()); + expect_trace_crash( + seed_heartbleed.build_trace(), + PutOptions::default(), + Some(std::time::Duration::from_secs(5)), + Some(5), + ); } #[test] @@ -1135,7 +1145,12 @@ pub mod tests { use crate::test_utils::expect_trace_crash; - expect_trace_crash(seed_cve_2021_3449.build_trace(), PutOptions::default()); + expect_trace_crash( + seed_cve_2021_3449.build_trace(), + PutOptions::default(), + Some(std::time::Duration::from_secs(5)), + Some(5), + ); } #[test] @@ -1189,6 +1204,8 @@ pub mod tests { expect_trace_crash( seed_session_resumption_dhe_full.build_trace(), PutOptions::from_slice_vec(vec![("use_clear", &true.to_string())]), + Some(std::time::Duration::from_secs(5)), + Some(5), ); } @@ -1205,7 +1222,12 @@ pub mod tests { crate::tls::seeds::seed_successful12_with_tickets.execute_trace(); } - expect_trace_crash(seed_cve_2022_38153.build_trace(), PutOptions::default()); + expect_trace_crash( + seed_cve_2022_38153.build_trace(), + PutOptions::default(), + Some(std::time::Duration::from_secs(5)), + Some(5), + ); } #[cfg(all(feature = "tls13", feature = "tls13-session-resumption"))] @@ -1220,7 +1242,12 @@ pub mod tests { use crate::test_utils::expect_trace_crash; - expect_trace_crash(seed_cve_2022_39173.build_trace(), PutOptions::default()); + expect_trace_crash( + seed_cve_2022_39173.build_trace(), + PutOptions::default(), + Some(std::time::Duration::from_secs(5)), + Some(5), + ); } #[cfg(all(feature = "tls13", feature = "tls13-session-resumption"))] @@ -1238,6 +1265,8 @@ pub mod tests { expect_trace_crash( seed_cve_2022_39173_full.build_trace(), PutOptions::default(), + Some(std::time::Duration::from_secs(5)), + Some(5), ); } @@ -1256,6 +1285,8 @@ pub mod tests { expect_trace_crash( seed_cve_2022_39173_minimized.build_trace(), PutOptions::default(), + Some(std::time::Duration::from_secs(5)), + Some(5), ); }