From f584d22b534801e54c6b4137042165bf60ed151c Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Wed, 30 Apr 2025 10:47:11 +0200 Subject: [PATCH 1/2] Rust: Use type inference to insert implicit borrows and derefs --- .../rust/dataflow/internal/DataFlowImpl.qll | 22 ++---------- .../codeql/rust/internal/TypeInference.qll | 26 ++++++++++++-- .../dataflow/local/DataFlowStep.expected | 35 ------------------- .../security/CWE-089/SqlInjection.expected | 12 ------- .../CaptureSummaryModels.expected | 8 ----- 5 files changed, 27 insertions(+), 76 deletions(-) diff --git a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll index 4376df7caf8d..b29506224406 100644 --- a/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll +++ b/rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll @@ -11,6 +11,7 @@ private import rust private import SsaImpl as SsaImpl private import codeql.rust.controlflow.internal.Scope as Scope private import codeql.rust.internal.PathResolution +private import codeql.rust.internal.TypeInference as TypeInference private import codeql.rust.controlflow.ControlFlowGraph private import codeql.rust.controlflow.CfgNodes private import codeql.rust.dataflow.Ssa @@ -321,23 +322,6 @@ predicate lambdaCallExpr(CallExprCfgNode call, LambdaCallKind kind, ExprCfgNode exists(kind) } -/** Holds if `mc` implicitly borrows its receiver. */ -private predicate implicitBorrow(MethodCallExpr mc) { - // Determining whether an implicit borrow happens depends on the type of the - // receiever as well as the target. As a heuristic we simply check if the - // target takes `self` as a borrow and limit the approximation to cases where - // the receiver is a simple variable. - mc.getReceiver() instanceof VariableAccess and - mc.getStaticTarget().getParamList().getSelfParam().isRef() -} - -/** Holds if `mc` implicitly dereferences its receiver. */ -private predicate implicitDeref(MethodCallExpr mc) { - // Similarly to `implicitBorrow` this is an approximation. - mc.getReceiver() instanceof VariableAccess and - not mc.getStaticTarget().getParamList().getSelfParam().isRef() -} - // Defines a set of aliases needed for the `RustDataFlow` module private module Aliases { class DataFlowCallableAlias = DataFlowCallable; @@ -520,15 +504,15 @@ module RustDataFlow implements InputSig { pragma[nomagic] private predicate implicitDerefToReceiver(Node node1, ReceiverNode node2, ReferenceContent c) { + TypeInference::receiverHasImplicitDeref(node1.asExpr().getExpr()) and node1.asExpr() = node2.getReceiver() and - implicitDeref(node2.getMethodCall().getMethodCallExpr()) and exists(c) } pragma[nomagic] private predicate implicitBorrowToReceiver(Node node1, ReceiverNode node2, ReferenceContent c) { + TypeInference::receiverHasImplicitBorrow(node1.asExpr().getExpr()) and node1.asExpr() = node2.getReceiver() and - implicitBorrow(node2.getMethodCall().getMethodCallExpr()) and exists(c) } diff --git a/rust/ql/lib/codeql/rust/internal/TypeInference.qll b/rust/ql/lib/codeql/rust/internal/TypeInference.qll index 2fae9ef1f5b8..b999b72240bf 100644 --- a/rust/ql/lib/codeql/rust/internal/TypeInference.qll +++ b/rust/ql/lib/codeql/rust/internal/TypeInference.qll @@ -690,7 +690,7 @@ private Type inferCallExprBaseType(AstNode n, TypePath path) { | if apos.isSelf() then - exists(Type receiverType | receiverType = CallExprBaseMatchingInput::inferReceiverType(n) | + exists(Type receiverType | receiverType = inferType(n) | if receiverType = TRefType() then path = path0 and @@ -840,7 +840,7 @@ private Type inferFieldExprType(AstNode n, TypePath path) { | if apos.isSelf() then - exists(Type receiverType | receiverType = FieldExprMatchingInput::inferReceiverType(n) | + exists(Type receiverType | receiverType = inferType(n) | if receiverType = TRefType() then // adjust for implicit deref @@ -895,6 +895,28 @@ cached private module Cached { private import codeql.rust.internal.CachedStages + /** Holds if `receiver` is the receiver of a method call with an implicit dereference. */ + cached + predicate receiverHasImplicitDeref(AstNode receiver) { + exists(CallExprBaseMatchingInput::Access a, CallExprBaseMatchingInput::AccessPosition apos | + apos.isSelf() and + receiver = a.getNodeAt(apos) and + inferType(receiver) = TRefType() and + CallExprBaseMatching::inferAccessType(a, apos, TypePath::nil()) != TRefType() + ) + } + + /** Holds if `receiver` is the receiver of a method call with an implicit borrow. */ + cached + predicate receiverHasImplicitBorrow(AstNode receiver) { + exists(CallExprBaseMatchingInput::Access a, CallExprBaseMatchingInput::AccessPosition apos | + apos.isSelf() and + receiver = a.getNodeAt(apos) and + CallExprBaseMatching::inferAccessType(a, apos, TypePath::nil()) = TRefType() and + inferType(receiver) != TRefType() + ) + } + pragma[inline] private Type getLookupType(AstNode n) { exists(Type t | diff --git a/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected b/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected index d9f17dbf4c4d..6c2a7c2ba85b 100644 --- a/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected +++ b/rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected @@ -1931,33 +1931,16 @@ readStep | main.rs:221:9:221:23 | ...::Some(...) | Some | main.rs:221:22:221:22 | n | | main.rs:230:9:230:15 | Some(...) | Some | main.rs:230:14:230:14 | n | | main.rs:234:9:234:15 | Some(...) | Some | main.rs:234:14:234:14 | n | -| main.rs:241:10:241:11 | s1 | &ref | main.rs:241:10:241:11 | receiver for s1 | -| main.rs:246:10:246:11 | s1 | &ref | main.rs:246:10:246:11 | receiver for s1 | -| main.rs:249:10:249:11 | s2 | &ref | main.rs:249:10:249:11 | receiver for s2 | -| main.rs:254:10:254:11 | s1 | &ref | main.rs:254:10:254:11 | receiver for s1 | -| main.rs:257:10:257:11 | s2 | &ref | main.rs:257:10:257:11 | receiver for s2 | | main.rs:263:14:263:15 | s1 | Ok | main.rs:263:14:263:16 | TryExpr | | main.rs:263:14:263:15 | s1 | Some | main.rs:263:14:263:16 | TryExpr | | main.rs:265:10:265:11 | s2 | Ok | main.rs:265:10:265:12 | TryExpr | | main.rs:265:10:265:11 | s2 | Some | main.rs:265:10:265:12 | TryExpr | -| main.rs:271:29:271:30 | r1 | &ref | main.rs:271:29:271:30 | receiver for r1 | -| main.rs:272:29:272:30 | r1 | &ref | main.rs:272:29:272:30 | receiver for r1 | -| main.rs:273:10:273:12 | o1a | &ref | main.rs:273:10:273:12 | receiver for o1a | -| main.rs:274:10:274:12 | o1b | &ref | main.rs:274:10:274:12 | receiver for o1b | -| main.rs:277:29:277:30 | r2 | &ref | main.rs:277:29:277:30 | receiver for r2 | -| main.rs:278:29:278:30 | r2 | &ref | main.rs:278:29:278:30 | receiver for r2 | -| main.rs:279:10:279:12 | o2a | &ref | main.rs:279:10:279:12 | receiver for o2a | -| main.rs:280:10:280:12 | o2b | &ref | main.rs:280:10:280:12 | receiver for o2b | | main.rs:287:14:287:15 | s1 | Ok | main.rs:287:14:287:16 | TryExpr | | main.rs:287:14:287:15 | s1 | Some | main.rs:287:14:287:16 | TryExpr | | main.rs:288:14:288:15 | s2 | Ok | main.rs:288:14:288:16 | TryExpr | | main.rs:288:14:288:15 | s2 | Some | main.rs:288:14:288:16 | TryExpr | | main.rs:291:14:291:15 | s3 | Ok | main.rs:291:14:291:16 | TryExpr | | main.rs:291:14:291:15 | s3 | Some | main.rs:291:14:291:16 | TryExpr | -| main.rs:298:10:298:11 | s1 | &ref | main.rs:298:10:298:11 | receiver for s1 | -| main.rs:299:10:299:11 | s1 | &ref | main.rs:299:10:299:11 | receiver for s1 | -| main.rs:302:10:302:11 | s2 | &ref | main.rs:302:10:302:11 | receiver for s2 | -| main.rs:303:10:303:11 | s2 | &ref | main.rs:303:10:303:11 | receiver for s2 | | main.rs:315:9:315:25 | ...::A(...) | A | main.rs:315:24:315:24 | n | | main.rs:316:9:316:25 | ...::B(...) | B | main.rs:316:24:316:24 | n | | main.rs:319:9:319:25 | ...::A(...) | A | main.rs:319:24:319:24 | n | @@ -1997,40 +1980,22 @@ readStep | main.rs:442:9:442:20 | TuplePat | tuple.0 | main.rs:442:10:442:13 | cond | | main.rs:442:9:442:20 | TuplePat | tuple.1 | main.rs:442:16:442:19 | name | | main.rs:442:25:442:29 | names | element | main.rs:442:9:442:20 | TuplePat | -| main.rs:444:21:444:24 | name | &ref | main.rs:444:21:444:24 | receiver for name | | main.rs:444:41:444:67 | [post] \|...\| ... | captured default_name | main.rs:444:41:444:67 | [post] default_name | -| main.rs:444:44:444:55 | default_name | &ref | main.rs:444:44:444:55 | receiver for default_name | | main.rs:444:44:444:55 | this | captured default_name | main.rs:444:44:444:55 | default_name | -| main.rs:445:18:445:18 | n | &ref | main.rs:445:18:445:18 | receiver for n | -| main.rs:468:13:468:13 | a | &ref | main.rs:468:13:468:13 | receiver for a | -| main.rs:469:13:469:13 | b | &ref | main.rs:469:13:469:13 | receiver for b | -| main.rs:470:19:470:19 | b | &ref | main.rs:470:19:470:19 | receiver for b | | main.rs:481:10:481:11 | vs | element | main.rs:481:10:481:14 | vs[0] | -| main.rs:482:11:482:12 | vs | &ref | main.rs:482:11:482:12 | receiver for vs | | main.rs:482:11:482:35 | ... .unwrap() | &ref | main.rs:482:10:482:35 | * ... | -| main.rs:483:11:483:12 | vs | &ref | main.rs:483:11:483:12 | receiver for vs | | main.rs:483:11:483:35 | ... .unwrap() | &ref | main.rs:483:10:483:35 | * ... | | main.rs:485:14:485:15 | vs | element | main.rs:485:9:485:9 | v | | main.rs:488:9:488:10 | &... | &ref | main.rs:488:10:488:10 | v | -| main.rs:488:15:488:16 | vs | &ref | main.rs:488:15:488:16 | receiver for vs | | main.rs:488:15:488:23 | vs.iter() | element | main.rs:488:9:488:10 | &... | -| main.rs:492:27:492:28 | vs | &ref | main.rs:492:27:492:28 | receiver for vs | | main.rs:493:9:493:10 | &... | &ref | main.rs:493:10:493:10 | v | | main.rs:493:15:493:17 | vs2 | element | main.rs:493:9:493:10 | &... | -| main.rs:497:5:497:6 | vs | &ref | main.rs:497:5:497:6 | receiver for vs | | main.rs:497:29:497:29 | x | &ref | main.rs:497:28:497:29 | * ... | -| main.rs:498:5:498:6 | vs | &ref | main.rs:498:5:498:6 | receiver for vs | | main.rs:498:34:498:34 | x | &ref | main.rs:498:33:498:34 | * ... | -| main.rs:500:14:500:15 | vs | &ref | main.rs:500:14:500:15 | receiver for vs | | main.rs:500:14:500:27 | vs.into_iter() | element | main.rs:500:9:500:9 | v | | main.rs:506:10:506:15 | vs_mut | element | main.rs:506:10:506:18 | vs_mut[0] | -| main.rs:507:11:507:16 | vs_mut | &ref | main.rs:507:11:507:16 | receiver for vs_mut | | main.rs:507:11:507:39 | ... .unwrap() | &ref | main.rs:507:10:507:39 | * ... | -| main.rs:508:11:508:16 | vs_mut | &ref | main.rs:508:11:508:16 | receiver for vs_mut | | main.rs:508:11:508:39 | ... .unwrap() | &ref | main.rs:508:10:508:39 | * ... | | main.rs:510:9:510:14 | &mut ... | &ref | main.rs:510:14:510:14 | v | -| main.rs:510:19:510:24 | vs_mut | &ref | main.rs:510:19:510:24 | receiver for vs_mut | | main.rs:510:19:510:35 | vs_mut.iter_mut() | element | main.rs:510:9:510:14 | &mut ... | | main.rs:524:11:524:15 | c_ref | &ref | main.rs:524:10:524:15 | * ... | -| main.rs:531:10:531:10 | a | &ref | main.rs:531:10:531:10 | receiver for a | -| main.rs:537:10:537:10 | b | &ref | main.rs:537:10:537:10 | receiver for b | diff --git a/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected b/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected index b5aa4a386f29..7c3c14194747 100644 --- a/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected +++ b/rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected @@ -31,15 +31,11 @@ edges | sqlx.rs:52:32:52:87 | ...::must_use(...) | sqlx.rs:52:9:52:20 | safe_query_3 | provenance | | | sqlx.rs:52:32:52:87 | MacroExpr | sqlx.rs:52:32:52:87 | ...::format(...) | provenance | MaD:4 | | sqlx.rs:52:32:52:87 | { ... } | sqlx.rs:52:32:52:87 | ...::must_use(...) | provenance | MaD:9 | -| sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | sqlx.rs:63:26:63:39 | unsafe_query_1 [&ref] | provenance | | | sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | sqlx.rs:63:26:63:48 | unsafe_query_1.as_str() | provenance | MaD:3 | -| sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | sqlx.rs:74:25:74:38 | unsafe_query_1 [&ref] | provenance | | | sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | sqlx.rs:74:25:74:47 | unsafe_query_1.as_str() | provenance | MaD:3 | | sqlx.rs:53:26:53:36 | &arg_string [&ref] | sqlx.rs:53:9:53:22 | unsafe_query_1 [&ref] | provenance | | | sqlx.rs:53:27:53:36 | arg_string | sqlx.rs:53:26:53:36 | &arg_string [&ref] | provenance | | -| sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | sqlx.rs:65:30:65:43 | unsafe_query_2 [&ref] | provenance | | | sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | sqlx.rs:65:30:65:52 | unsafe_query_2.as_str() | provenance | MaD:3 | -| sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | sqlx.rs:76:29:76:42 | unsafe_query_2 [&ref] | provenance | | | sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | sqlx.rs:76:29:76:51 | unsafe_query_2.as_str() | provenance | MaD:3 | | sqlx.rs:54:26:54:39 | &remote_string [&ref] | sqlx.rs:54:9:54:22 | unsafe_query_2 [&ref] | provenance | | | sqlx.rs:54:27:54:39 | remote_string | sqlx.rs:54:26:54:39 | &remote_string [&ref] | provenance | | @@ -50,10 +46,6 @@ edges | sqlx.rs:56:34:56:89 | ...::must_use(...) | sqlx.rs:56:9:56:22 | unsafe_query_4 | provenance | | | sqlx.rs:56:34:56:89 | MacroExpr | sqlx.rs:56:34:56:89 | ...::format(...) | provenance | MaD:4 | | sqlx.rs:56:34:56:89 | { ... } | sqlx.rs:56:34:56:89 | ...::must_use(...) | provenance | MaD:9 | -| sqlx.rs:63:26:63:39 | unsafe_query_1 [&ref] | sqlx.rs:63:26:63:48 | unsafe_query_1.as_str() | provenance | MaD:3 | -| sqlx.rs:65:30:65:43 | unsafe_query_2 [&ref] | sqlx.rs:65:30:65:52 | unsafe_query_2.as_str() | provenance | MaD:3 | -| sqlx.rs:74:25:74:38 | unsafe_query_1 [&ref] | sqlx.rs:74:25:74:47 | unsafe_query_1.as_str() | provenance | MaD:3 | -| sqlx.rs:76:29:76:42 | unsafe_query_2 [&ref] | sqlx.rs:76:29:76:51 | unsafe_query_2.as_str() | provenance | MaD:3 | models | 1 | Source: lang:std; crate::env::args; commandargs; ReturnValue.Element | | 2 | Source: repo:https://github.com/seanmonstar/reqwest:reqwest; crate::blocking::get; remote; ReturnValue.Field[crate::result::Result::Ok(0)] | @@ -100,15 +92,11 @@ nodes | sqlx.rs:56:34:56:89 | MacroExpr | semmle.label | MacroExpr | | sqlx.rs:56:34:56:89 | { ... } | semmle.label | { ... } | | sqlx.rs:62:26:62:46 | safe_query_3.as_str() | semmle.label | safe_query_3.as_str() | -| sqlx.rs:63:26:63:39 | unsafe_query_1 [&ref] | semmle.label | unsafe_query_1 [&ref] | | sqlx.rs:63:26:63:48 | unsafe_query_1.as_str() | semmle.label | unsafe_query_1.as_str() | -| sqlx.rs:65:30:65:43 | unsafe_query_2 [&ref] | semmle.label | unsafe_query_2 [&ref] | | sqlx.rs:65:30:65:52 | unsafe_query_2.as_str() | semmle.label | unsafe_query_2.as_str() | | sqlx.rs:67:30:67:52 | unsafe_query_4.as_str() | semmle.label | unsafe_query_4.as_str() | | sqlx.rs:73:25:73:45 | safe_query_3.as_str() | semmle.label | safe_query_3.as_str() | -| sqlx.rs:74:25:74:38 | unsafe_query_1 [&ref] | semmle.label | unsafe_query_1 [&ref] | | sqlx.rs:74:25:74:47 | unsafe_query_1.as_str() | semmle.label | unsafe_query_1.as_str() | -| sqlx.rs:76:29:76:42 | unsafe_query_2 [&ref] | semmle.label | unsafe_query_2 [&ref] | | sqlx.rs:76:29:76:51 | unsafe_query_2.as_str() | semmle.label | unsafe_query_2.as_str() | | sqlx.rs:78:29:78:51 | unsafe_query_4.as_str() | semmle.label | unsafe_query_4.as_str() | subpaths diff --git a/rust/ql/test/utils-tests/modelgenerator/CaptureSummaryModels.expected b/rust/ql/test/utils-tests/modelgenerator/CaptureSummaryModels.expected index b05e99f352b0..e827c7320cc7 100644 --- a/rust/ql/test/utils-tests/modelgenerator/CaptureSummaryModels.expected +++ b/rust/ql/test/utils-tests/modelgenerator/CaptureSummaryModels.expected @@ -1,12 +1,4 @@ unexpectedModel | Unexpected summary found: repo::test;::clone;Argument[self].Field[crate::option::MyOption::MySome(0)].Reference;ReturnValue.Field[crate::option::MyOption::MySome(0)];value;dfc-generated | -| Unexpected summary found: repo::test;::from;Argument[0].Field[crate::option::MyOption::MySome(0)];ReturnValue.Field[crate::option::MyOption::MySome(0)].Reference;value;dfc-generated | | Unexpected summary found: repo::test;::cloned;Argument[self].Field[crate::option::MyOption::MySome(0)].Reference;ReturnValue.Field[crate::option::MyOption::MySome(0)];value;dfc-generated | -| Unexpected summary found: repo::test;::get_or_insert;Argument[0];Argument[self].Field[crate::option::MyOption::MySome(0)];value;dfc-generated | -| Unexpected summary found: repo::test;::get_or_insert;Argument[self].Field[crate::option::MyOption::MySome(0)];ReturnValue.Reference;value;dfc-generated | -| Unexpected summary found: repo::test;::get_or_insert_default;Argument[self].Field[crate::option::MyOption::MySome(0)];ReturnValue.Reference;value;dfc-generated | -| Unexpected summary found: repo::test;::get_or_insert_with;Argument[self].Field[crate::option::MyOption::MySome(0)];ReturnValue.Reference;value;dfc-generated | -| Unexpected summary found: repo::test;::insert;Argument[self].Field[crate::option::MyOption::MySome(0)];ReturnValue.Reference;value;dfc-generated | expectedModel -| Expected summary missing: repo::test;::take_if;Argument[self].Reference.Field[crate::option::MyOption::MySome(0)];Argument[0].Parameter[0].Reference;value;dfc-generated | -| Expected summary missing: repo::test;::take_if;Argument[self].Reference;ReturnValue;value;dfc-generated | From c263d3faf93f29d691612f947482365d2e08fa14 Mon Sep 17 00:00:00 2001 From: Simon Friis Vindum Date: Wed, 30 Apr 2025 17:39:22 +0200 Subject: [PATCH 2/2] Rust: Remove predicates unused after refactor --- .../lib/codeql/rust/internal/TypeInference.qll | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/rust/ql/lib/codeql/rust/internal/TypeInference.qll b/rust/ql/lib/codeql/rust/internal/TypeInference.qll index b999b72240bf..f553351868b3 100644 --- a/rust/ql/lib/codeql/rust/internal/TypeInference.qll +++ b/rust/ql/lib/codeql/rust/internal/TypeInference.qll @@ -662,15 +662,6 @@ private module CallExprBaseMatchingInput implements MatchingInputSig { tAdj = t ) } - - pragma[nomagic] - additional Type inferReceiverType(AstNode n) { - exists(Access a, AccessPosition apos | - result = inferType(n) and - n = a.getNodeAt(apos) and - apos.isSelf() - ) - } } private module CallExprBaseMatching = Matching; @@ -813,15 +804,6 @@ private module FieldExprMatchingInput implements MatchingInputSig { tAdj = t ) } - - pragma[nomagic] - additional Type inferReceiverType(AstNode n) { - exists(Access a, AccessPosition apos | - result = inferType(n) and - n = a.getNodeAt(apos) and - apos.isSelf() - ) - } } private module FieldExprMatching = Matching;