From af7dd283dec0912e3942c409e0d6dac9778dddb6 Mon Sep 17 00:00:00 2001 From: LucaCappelletti94 Date: Tue, 27 Oct 2026 12:48:39 +0100 Subject: [PATCH 1/2] Rust: Do not treat the early return of ? as returning the unwrapped value --- .../2026-10-27-try-operator-early-return.md | 4 +++ .../codeql/rust/dataflow/internal/Node.qll | 8 +++++- .../dataflow/global/inline-flow.expected | 23 +++++++++++++++-- .../library-tests/dataflow/global/main.rs | 19 ++++++++++++++ .../dataflow/global/viableCallable.expected | 8 ++++++ .../CWE-312/CleartextLogging.expected | 6 +++++ .../security/CWE-312/test_logging.rs | 25 +++++++++++++++++++ 7 files changed, 90 insertions(+), 3 deletions(-) create mode 100644 rust/ql/lib/change-notes/2026-10-27-try-operator-early-return.md diff --git a/rust/ql/lib/change-notes/2026-10-27-try-operator-early-return.md b/rust/ql/lib/change-notes/2026-10-27-try-operator-early-return.md new file mode 100644 index 000000000000..d80859cc435d --- /dev/null +++ b/rust/ql/lib/change-notes/2026-10-27-try-operator-early-return.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The early return taken by the `?` operator no longer makes the unwrapped value of `x?` flow out of the enclosing function. This fixes false positive results from queries such as `rust/cleartext-logging`, where a sensitive value read with `?` was reported as part of an unrelated return value. diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/Node.qll b/rust/ql/lib/codeql/rust/dataflow/internal/Node.qll index b82c0f9221a1..be1b6fec2b27 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/Node.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/Node.qll @@ -473,7 +473,13 @@ abstract class ReturnNode extends Node { } final class ExprReturnNode extends ExprNode, ReturnNode { - ExprReturnNode() { n.getACfgNode().getASuccessor() instanceof AnnotatedExitCfgNode } + ExprReturnNode() { + exists(SuccessorType t | n.getACfgNode().getASuccessor(t) instanceof AnnotatedExitCfgNode | + // The early return of `x?` returns the `Err` or `None` held by `x`, which is + // a different value from the one `x?` evaluates to + not (n instanceof TryExpr and t instanceof ReturnSuccessor) + ) + } override ReturnKind getKind() { result = TNormalReturnKind() } } diff --git a/rust/ql/test/library-tests/dataflow/global/inline-flow.expected b/rust/ql/test/library-tests/dataflow/global/inline-flow.expected index 6fe331bb4d9b..8ed5ee8ac041 100644 --- a/rust/ql/test/library-tests/dataflow/global/inline-flow.expected +++ b/rust/ql/test/library-tests/dataflow/global/inline-flow.expected @@ -1,7 +1,8 @@ models | 1 | Summary: <& as core::ops::deref::Deref>::deref; Argument[self].Reference; ReturnValue; value | | 2 | Summary: ::mul; Argument[self,0]; ReturnValue; taint | -| 3 | Summary: futures_executor::local_pool::block_on; Argument[0]; ReturnValue; value | +| 3 | Summary: ::unwrap; Argument[self].Field[core::result::Result::Ok(0)]; ReturnValue; value | +| 4 | Summary: futures_executor::local_pool::block_on; Argument[0]; ReturnValue; value | edges | main.rs:12:28:14:1 | { ... } | main.rs:17:13:17:23 | get_data(...) | provenance | | | main.rs:13:5:13:13 | source(...) | main.rs:12:28:14:1 | { ... } | provenance | | @@ -201,7 +202,7 @@ edges | main.rs:348:9:348:9 | c | main.rs:350:10:350:16 | await b | provenance | | | main.rs:354:9:354:9 | a | main.rs:355:10:355:10 | a | provenance | | | main.rs:354:13:354:55 | ...::block_on(...) | main.rs:354:9:354:9 | a | provenance | | -| main.rs:354:41:354:54 | async_source(...) | main.rs:354:13:354:55 | ...::block_on(...) | provenance | MaD:3 | +| main.rs:354:41:354:54 | async_source(...) | main.rs:354:13:354:55 | ...::block_on(...) | provenance | MaD:4 | | main.rs:366:44:368:9 | { ... } | main.rs:403:18:403:38 | t.get_double_number() | provenance | | | main.rs:366:44:368:9 | { ... } | main.rs:407:18:407:50 | ...::get_double_number(...) | provenance | | | main.rs:367:13:367:29 | self.get_number() | main.rs:367:13:367:33 | ... * ... | provenance | MaD:2 | @@ -233,6 +234,14 @@ edges | main.rs:433:32:433:43 | STATIC_VALUE | main.rs:433:17:433:28 | static_value | provenance | | | main.rs:436:13:436:24 | STATIC_VALUE | main.rs:427:5:427:46 | static STATIC_VALUE | provenance | | | main.rs:436:28:436:37 | source(...) | main.rs:436:13:436:24 | STATIC_VALUE | provenance | | +| main.rs:466:55:469:5 | { ... } [Ok] | main.rs:473:14:473:39 | unwrapped_value_returned(...) [Ok] | provenance | | +| main.rs:467:13:467:13 | a [Ok, Ok] | main.rs:468:9:468:9 | a [Ok, Ok] | provenance | | +| main.rs:467:48:467:65 | Ok(...) [Ok, Ok] | main.rs:467:13:467:13 | a [Ok, Ok] | provenance | | +| main.rs:467:51:467:64 | Ok(...) [Ok] | main.rs:467:48:467:65 | Ok(...) [Ok, Ok] | provenance | | +| main.rs:467:54:467:63 | source(...) | main.rs:467:51:467:64 | Ok(...) [Ok] | provenance | | +| main.rs:468:9:468:9 | a [Ok, Ok] | main.rs:468:9:468:10 | TryExpr [Ok] | provenance | | +| main.rs:468:9:468:10 | TryExpr [Ok] | main.rs:466:55:469:5 | { ... } [Ok] | provenance | | +| main.rs:473:14:473:39 | unwrapped_value_returned(...) [Ok] | main.rs:473:14:473:48 | ... .unwrap() | provenance | MaD:3 | nodes | main.rs:12:28:14:1 | { ... } | semmle.label | { ... } | | main.rs:13:5:13:13 | source(...) | semmle.label | source(...) | @@ -488,6 +497,15 @@ nodes | main.rs:436:13:436:24 | STATIC_VALUE | semmle.label | STATIC_VALUE | | main.rs:436:28:436:37 | source(...) | semmle.label | source(...) | | main.rs:437:18:437:29 | STATIC_VALUE | semmle.label | STATIC_VALUE | +| main.rs:466:55:469:5 | { ... } [Ok] | semmle.label | { ... } [Ok] | +| main.rs:467:13:467:13 | a [Ok, Ok] | semmle.label | a [Ok, Ok] | +| main.rs:467:48:467:65 | Ok(...) [Ok, Ok] | semmle.label | Ok(...) [Ok, Ok] | +| main.rs:467:51:467:64 | Ok(...) [Ok] | semmle.label | Ok(...) [Ok] | +| main.rs:467:54:467:63 | source(...) | semmle.label | source(...) | +| main.rs:468:9:468:9 | a [Ok, Ok] | semmle.label | a [Ok, Ok] | +| main.rs:468:9:468:10 | TryExpr [Ok] | semmle.label | TryExpr [Ok] | +| main.rs:473:14:473:39 | unwrapped_value_returned(...) [Ok] | semmle.label | unwrapped_value_returned(...) [Ok] | +| main.rs:473:14:473:48 | ... .unwrap() | semmle.label | ... .unwrap() | subpaths | main.rs:38:16:38:24 | source(...) | main.rs:26:28:26:33 | ...: i64 | main.rs:26:17:26:25 | SelfParam [Return] [&ref, MyStruct] | main.rs:38:5:38:5 | [post] a [MyStruct] | | main.rs:39:10:39:10 | a [MyStruct] | main.rs:30:17:30:21 | SelfParam [&ref, MyStruct] | main.rs:30:31:32:5 | { ... } | main.rs:39:10:39:21 | a.get_data() | @@ -555,3 +573,4 @@ testFailures | main.rs:434:18:434:29 | static_value | main.rs:436:28:436:37 | source(...) | main.rs:434:18:434:29 | static_value | $@ | main.rs:436:28:436:37 | source(...) | source(...) | | main.rs:437:18:437:29 | STATIC_VALUE | main.rs:427:36:427:45 | source(...) | main.rs:437:18:437:29 | STATIC_VALUE | $@ | main.rs:427:36:427:45 | source(...) | source(...) | | main.rs:437:18:437:29 | STATIC_VALUE | main.rs:436:28:436:37 | source(...) | main.rs:437:18:437:29 | STATIC_VALUE | $@ | main.rs:436:28:436:37 | source(...) | source(...) | +| main.rs:473:14:473:48 | ... .unwrap() | main.rs:467:54:467:63 | source(...) | main.rs:473:14:473:48 | ... .unwrap() | $@ | main.rs:467:54:467:63 | source(...) | source(...) | diff --git a/rust/ql/test/library-tests/dataflow/global/main.rs b/rust/ql/test/library-tests/dataflow/global/main.rs index 2d8975e153a5..ba49a11b7783 100644 --- a/rust/ql/test/library-tests/dataflow/global/main.rs +++ b/rust/ql/test/library-tests/dataflow/global/main.rs @@ -454,3 +454,22 @@ fn main() { test_operator_overloading(); test_async_await(); } + +mod try_operator { + use super::{sink, source}; + + fn unwrapped_value_not_returned() -> Option { + let _a = Some(source(50))?; + Some(0) + } + + fn unwrapped_value_returned() -> Result { + let a: Result, i64> = Ok(Ok(source(51))); + a? + } + + fn test_try_operator() { + sink(unwrapped_value_not_returned().unwrap()); + sink(unwrapped_value_returned().unwrap()); // $ hasValueFlow=51 + } +} diff --git a/rust/ql/test/library-tests/dataflow/global/viableCallable.expected b/rust/ql/test/library-tests/dataflow/global/viableCallable.expected index 6f30416d54a7..cb6efb107ff5 100644 --- a/rust/ql/test/library-tests/dataflow/global/viableCallable.expected +++ b/rust/ql/test/library-tests/dataflow/global/viableCallable.expected @@ -145,3 +145,11 @@ | main.rs:452:5:452:25 | data_through_method(...) | main.rs:187:1:199:1 | fn data_through_method | | main.rs:454:5:454:31 | test_operator_overloading(...) | main.rs:256:1:298:1 | fn test_operator_overloading | | main.rs:455:5:455:22 | test_async_await(...) | main.rs:353:1:358:1 | fn test_async_await | +| main.rs:462:23:462:32 | source(...) | main.rs:1:1:3:1 | fn source | +| main.rs:467:54:467:63 | source(...) | main.rs:1:1:3:1 | fn source | +| main.rs:472:9:472:53 | sink(...) | main.rs:5:1:7:1 | fn sink | +| main.rs:472:14:472:43 | unwrapped_value_not_returned(...) | main.rs:461:5:464:5 | fn unwrapped_value_not_returned | +| main.rs:472:14:472:52 | ... .unwrap() | {EXTERNAL LOCATION} | [summarized] fn unwrap | +| main.rs:473:9:473:49 | sink(...) | main.rs:5:1:7:1 | fn sink | +| main.rs:473:14:473:39 | unwrapped_value_returned(...) | main.rs:466:5:469:5 | fn unwrapped_value_returned | +| main.rs:473:14:473:48 | ... .unwrap() | {EXTERNAL LOCATION} | [summarized] fn unwrap | diff --git a/rust/ql/test/query-tests/security/CWE-312/CleartextLogging.expected b/rust/ql/test/query-tests/security/CWE-312/CleartextLogging.expected index 9e83fc7fe9d6..b87bd75ca140 100644 --- a/rust/ql/test/query-tests/security/CWE-312/CleartextLogging.expected +++ b/rust/ql/test/query-tests/security/CWE-312/CleartextLogging.expected @@ -55,6 +55,7 @@ | test_logging.rs:245:20:245:65 | ... .as_bytes() | test_logging.rs:245:46:245:53 | password | test_logging.rs:245:20:245:65 | ... .as_bytes() | This operation writes $@ to a log file. | test_logging.rs:245:46:245:53 | password | password | | test_logging.rs:248:15:248:60 | ... .as_bytes() | test_logging.rs:248:41:248:48 | password | test_logging.rs:248:15:248:60 | ... .as_bytes() | This operation writes $@ to a log file. | test_logging.rs:248:41:248:48 | password | password | | test_logging.rs:251:15:251:60 | ... .as_bytes() | test_logging.rs:251:41:251:48 | password | test_logging.rs:251:15:251:60 | ... .as_bytes() | This operation writes $@ to a log file. | test_logging.rs:251:41:251:48 | password | password | +| test_logging.rs:290:11:290:31 | MacroExpr | test_logging.rs:289:17:289:39 | get_password_or_error(...) | test_logging.rs:290:11:290:31 | MacroExpr | This operation writes $@ to a log file. | test_logging.rs:289:17:289:39 | get_password_or_error(...) | get_password_or_error(...) | edges | test_logging.rs:42:28:42:35 | password | test_logging.rs:42:12:42:35 | MacroExpr | provenance | Sink:MaD:11 | | test_logging.rs:43:28:43:35 | password | test_logging.rs:43:12:43:35 | MacroExpr | provenance | Sink:MaD:11 | @@ -225,6 +226,8 @@ edges | test_logging.rs:251:23:251:48 | MacroExpr | test_logging.rs:251:23:251:48 | ...::format(...) | provenance | MaD:20 | | test_logging.rs:251:23:251:48 | { ... } | test_logging.rs:251:23:251:48 | ...::must_use(...) | provenance | MaD:21 | | test_logging.rs:251:41:251:48 | password | test_logging.rs:251:23:251:48 | MacroExpr | provenance | | +| test_logging.rs:289:9:289:13 | value | test_logging.rs:290:11:290:31 | MacroExpr | provenance | Sink:MaD:11 | +| test_logging.rs:289:17:289:39 | get_password_or_error(...) | test_logging.rs:289:9:289:13 | value | provenance | | models | 1 | Sink: ::log_expect; Argument[0]; log-injection | | 2 | Sink: ::expect; Argument[0]; log-injection | @@ -462,4 +465,7 @@ nodes | test_logging.rs:251:23:251:48 | MacroExpr | semmle.label | MacroExpr | | test_logging.rs:251:23:251:48 | { ... } | semmle.label | { ... } | | test_logging.rs:251:41:251:48 | password | semmle.label | password | +| test_logging.rs:289:9:289:13 | value | semmle.label | value | +| test_logging.rs:289:17:289:39 | get_password_or_error(...) | semmle.label | get_password_or_error(...) | +| test_logging.rs:290:11:290:31 | MacroExpr | semmle.label | MacroExpr | subpaths diff --git a/rust/ql/test/query-tests/security/CWE-312/test_logging.rs b/rust/ql/test/query-tests/security/CWE-312/test_logging.rs index 2936cb4f538b..50f0bd5ef7bb 100644 --- a/rust/ql/test/query-tests/security/CWE-312/test_logging.rs +++ b/rust/ql/test/query-tests/security/CWE-312/test_logging.rs @@ -271,6 +271,30 @@ fn test_collections(harmless: String) { password_queue.make_contiguous(); } +fn get_password_or_error() -> Result { + Ok("123456".to_string()) +} + +#[derive(Debug)] +struct Outcome { + count: usize, +} + +fn count_entries() -> Result { + let _p = get_password_or_error()?; + Ok(Outcome { count: 1 }) +} + +fn test_try_operator() -> Result<(), std::io::Error> { + let value = get_password_or_error()?; // $ Source[rust/cleartext-logging]=try + info!("message = {}", value); // $ Alert[rust/cleartext-logging]=try + + let outcome = count_entries()?; + info!("message = {:?}", outcome); // (the outcome does not contain the password) + + Ok(()) +} + fn main() { simple_logger::SimpleLogger::new().init().unwrap(); @@ -281,4 +305,5 @@ fn main() { ); test_std("123456".to_string(), 0, None); test_collections("harmless".to_string()); + _ = test_try_operator(); } From 6f89834a05b97cd32c4e460220e034acbddb9d1f Mon Sep 17 00:00:00 2001 From: LucaCappelletti94 Date: Tue, 27 Oct 2026 18:26:37 +0100 Subject: [PATCH 2/2] Rust: Share the returned-value check with callback return sinks --- .../rust/dataflow/internal/FlowSummaryImpl.qll | 4 +--- .../lib/codeql/rust/dataflow/internal/Node.qll | 17 ++++++++++------- .../test/library-tests/dataflow/models/main.rs | 15 +++++++++++++++ .../dataflow/models/models.ext.yml | 1 + 4 files changed, 27 insertions(+), 10 deletions(-) diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll index 58932fc2ae46..1294a108ef7e 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/FlowSummaryImpl.qll @@ -144,8 +144,6 @@ private Callable getCallable(Expr expr) { } module Input2 implements Impl::Private::InputSig2 { - private import codeql.rust.controlflow.CfgNodes - class SourceSinkReportingElement extends AstNode { DataFlowCallable getEnclosingCallable() { result.asCfgScope() = this.getEnclosingCfgScope() } @@ -158,7 +156,7 @@ module Input2 implements Impl::Private::InputSig2 { exists(Callable c | sc = Impl::Private::SummaryComponent::return(_) and c = getCallable(this) and - result.getACfgNode().getASuccessor() instanceof AnnotatedExitCfgNode and + isReturnedValue(result) and result.getEnclosingCfgScope() = c ) } diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/Node.qll b/rust/ql/lib/codeql/rust/dataflow/internal/Node.qll index be1b6fec2b27..654283a30f27 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/Node.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/Node.qll @@ -472,14 +472,17 @@ abstract class ReturnNode extends Node { abstract ReturnKind getKind(); } +/** Holds if the value of `n` is returned from its enclosing callable. */ +predicate isReturnedValue(AstNode n) { + exists(SuccessorType t | n.getACfgNode().getASuccessor(t) instanceof AnnotatedExitCfgNode | + // The early return of `x?` returns the `Err` or `None` held by `x`, which is + // a different value from the one `x?` evaluates to + not (n instanceof TryExpr and t instanceof ReturnSuccessor) + ) +} + final class ExprReturnNode extends ExprNode, ReturnNode { - ExprReturnNode() { - exists(SuccessorType t | n.getACfgNode().getASuccessor(t) instanceof AnnotatedExitCfgNode | - // The early return of `x?` returns the `Err` or `None` held by `x`, which is - // a different value from the one `x?` evaluates to - not (n instanceof TryExpr and t instanceof ReturnSuccessor) - ) - } + ExprReturnNode() { isReturnedValue(n) } override ReturnKind getKind() { result = TNormalReturnKind() } } diff --git a/rust/ql/test/library-tests/dataflow/models/main.rs b/rust/ql/test/library-tests/dataflow/models/main.rs index b005f11d546a..bb773c85e980 100644 --- a/rust/ql/test/library-tests/dataflow/models/main.rs +++ b/rust/ql/test/library-tests/dataflow/models/main.rs @@ -581,3 +581,18 @@ async fn main() { test_neutrals(); let dummy = Some(0); // ensure that the the `lang:core` crate is extracted } + +mod try_in_sink_callback { + use super::source; + + // has a sink model + fn pass_sink_option(f: impl FnOnce(()) -> Option) {} + + fn test_try_in_sink_callback() { + let a = |_a| { + let _s = Some(source(100))?; + Some(0) + }; + pass_sink_option(a); + } +} diff --git a/rust/ql/test/library-tests/dataflow/models/models.ext.yml b/rust/ql/test/library-tests/dataflow/models/models.ext.yml index 644d17f21d40..07edfd2fe612 100644 --- a/rust/ql/test/library-tests/dataflow/models/models.ext.yml +++ b/rust/ql/test/library-tests/dataflow/models/models.ext.yml @@ -26,6 +26,7 @@ extensions: - ["::sink", "Argument[self].Field[main::MyFieldEnum::D::field_d]", "test-sink", "manual"] - ["main::sink_out_of_function::pass_sink", "Argument[0].ReturnValue", "test-sink", "manual"] - ["main::sink_out_of_function::pass_sink_nested", "Argument[0].ReturnValue.Field[main::MyFieldEnum::E::field_e].Field[core::option::Option::Some(0)]", "test-sink", "manual"] + - ["main::try_in_sink_callback::pass_sink_option", "Argument[0].ReturnValue", "test-sink", "manual"] - ["main::external_file::generated_sink", "Argument[0]", "test-sink", "dfc-generated"] - ["main::external_file::neutral_generated_sink", "Argument[0]", "test-sink", "dfc-generated"] - ["main::external_file::neutral_manual_sink", "Argument[0]", "test-sink", "manual"]