From 73fa381dbcc96975611249e09cfeec6435836a2e Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Wed, 30 Apr 2025 19:49:47 +0200 Subject: [PATCH] Rust: Remove visibility check in path resolution --- .../codeql/rust/internal/PathResolution.qll | 93 ++++++------------- .../dataflow/modeled/inline-flow.expected | 15 +++ .../library-tests/dataflow/modeled/main.rs | 2 +- .../PathResolutionConsistency.expected | 41 ++++++++ 4 files changed, 87 insertions(+), 64 deletions(-) create mode 100644 rust/ql/test/query-tests/security/CWE-022/CONSISTENCY/PathResolutionConsistency.expected diff --git a/rust/ql/lib/codeql/rust/internal/PathResolution.qll b/rust/ql/lib/codeql/rust/internal/PathResolution.qll index 5bc45afecf1..d1878ce3dae 100644 --- a/rust/ql/lib/codeql/rust/internal/PathResolution.qll +++ b/rust/ql/lib/codeql/rust/internal/PathResolution.qll @@ -87,11 +87,6 @@ abstract class ItemNode extends Locatable { /** Gets the `i`th type parameter of this item, if any. */ abstract TypeParam getTypeParam(int i); - /** Holds if this item is declared as `pub`. */ - bindingset[this] - pragma[inline_late] - predicate isPublic() { exists(this.getVisibility()) } - /** Gets an element that has this item as immediately enclosing item. */ pragma[nomagic] Element getADescendant() { @@ -207,6 +202,11 @@ abstract class ItemNode extends Locatable { result.(CrateItemNode).isPotentialDollarCrateTarget() } + pragma[nomagic] + private predicate hasSourceFunction(string name) { + this.getASuccessorFull(name).(Function).fromSource() + } + /** Gets a successor named `name` of this item, if any. */ pragma[nomagic] ItemNode getASuccessor(string name) { @@ -219,7 +219,7 @@ abstract class ItemNode extends Locatable { or not result instanceof Function or - not this.getASuccessorFull(name).(Function).fromSource() + not this.hasSourceFunction(name) ) } @@ -266,8 +266,6 @@ private class SourceFileItemNode extends ModuleLikeNode, SourceFile { override Visibility getVisibility() { none() } - override predicate isPublic() { any() } - override TypeParam getTypeParam(int i) { none() } } @@ -330,8 +328,6 @@ class CrateItemNode extends ItemNode instanceof Crate { override Visibility getVisibility() { none() } - override predicate isPublic() { any() } - override TypeParam getTypeParam(int i) { none() } } @@ -436,7 +432,7 @@ abstract class ImplOrTraitItemNode extends ItemNode { pragma[nomagic] private TypeParamItemNode resolveTypeParamPathTypeRepr(PathTypeRepr ptr) { - result = resolvePath(ptr.getPath()) + result = resolvePathFull(ptr.getPath()) } class ImplItemNode extends ImplOrTraitItemNode instanceof Impl { @@ -444,9 +440,9 @@ class ImplItemNode extends ImplOrTraitItemNode instanceof Impl { Path getTraitPath() { result = super.getTrait().(PathTypeRepr).getPath() } - ItemNode resolveSelfTy() { result = resolvePath(this.getSelfPath()) } + ItemNode resolveSelfTy() { result = resolvePathFull(this.getSelfPath()) } - TraitItemNode resolveTraitTy() { result = resolvePath(this.getTraitPath()) } + TraitItemNode resolveTraitTy() { result = resolvePathFull(this.getTraitPath()) } pragma[nomagic] private TypeRepr getASelfTyArg() { @@ -560,7 +556,7 @@ class TraitItemNode extends ImplOrTraitItemNode instanceof Trait { } pragma[nomagic] - ItemNode resolveABound() { result = resolvePath(this.getABoundPath()) } + ItemNode resolveABound() { result = resolvePathFull(this.getABoundPath()) } override AssocItemNode getAnAssocItem() { result = super.getAssocItemList().getAnAssocItem() } @@ -634,7 +630,7 @@ class TypeParamItemNode extends ItemNode instanceof TypeParam { } pragma[nomagic] - ItemNode resolveABound() { result = resolvePath(this.getABoundPath()) } + ItemNode resolveABound() { result = resolvePathFull(this.getABoundPath()) } /** * Holds if this type parameter has a trait bound. Examples: @@ -897,12 +893,6 @@ class RelevantPath extends Path { this.getQualifier().(RelevantPath).isCratePath("$crate", _) and this.getText() = name } - - // TODO: Remove once the crate graph extractor generates publicly visible paths - predicate requiresExtractorWorkaround() { - not this.fromSource() and - this = any(RelevantPath p).getQualifier() - } } private predicate isModule(ItemNode m) { m instanceof Module } @@ -1056,8 +1046,14 @@ private predicate pathUsesNamespace(Path p, Namespace n) { ) } +/** + * Gets the item that `path` resolves to, if any. + * + * Whenever `path` can resolve to both a function in source code and in library + * code, both are included + */ pragma[nomagic] -private ItemNode resolvePath1(RelevantPath path) { +private ItemNode resolvePathFull(RelevantPath path) { exists(Namespace ns | result = resolvePath0(path, ns) | pathUsesNamespace(path, ns) or @@ -1067,58 +1063,29 @@ private ItemNode resolvePath1(RelevantPath path) { } pragma[nomagic] -private ItemNode resolvePathPrivate( - RelevantPath path, ModuleLikeNode itemParent, ModuleLikeNode pathParent -) { - not path.requiresExtractorWorkaround() and - result = resolvePath1(path) and - itemParent = result.getImmediateParentModule() and - not result.isPublic() and - ( - pathParent.getADescendant() = path - or - pathParent = any(ItemNode mid | path = mid.getADescendant()).getImmediateParentModule() - ) -} - -pragma[nomagic] -private predicate isItemParent(ModuleLikeNode itemParent) { - exists(resolvePathPrivate(_, itemParent, _)) -} - -/** - * Gets a module that has access to private items defined inside `itemParent`. - * - * According to [The Rust Reference][1] this is either `itemParent` itself or any - * descendant of `itemParent`. - * - * [1]: https://doc.rust-lang.org/reference/visibility-and-privacy.html#r-vis.access - */ -pragma[nomagic] -private ModuleLikeNode getAPrivateVisibleModule(ModuleLikeNode itemParent) { - isItemParent(itemParent) and - result.getImmediateParentModule*() = itemParent +private predicate resolvesSourceFunction(RelevantPath path) { + resolvePathFull(path).(Function).fromSource() } /** Gets the item that `path` resolves to, if any. */ cached ItemNode resolvePath(RelevantPath path) { - result = resolvePath1(path) and + result = resolvePathFull(path) and ( - result.isPublic() + // when a function exists in both source code and in library code, it is because + // we also extracted the source code as library code, and hence we only want + // the function from source code + result.fromSource() or - path.requiresExtractorWorkaround() - ) - or - exists(ModuleLikeNode itemParent, ModuleLikeNode pathParent | - result = resolvePathPrivate(path, itemParent, pathParent) and - pathParent = getAPrivateVisibleModule(itemParent) + not result instanceof Function + or + not resolvesSourceFunction(path) ) } pragma[nomagic] private ItemNode resolvePathQualifier(RelevantPath path, string name) { - result = resolvePath(path.getQualifier()) and + result = resolvePathFull(path.getQualifier()) and name = path.getText() } @@ -1164,7 +1131,7 @@ private ItemNode resolveUseTreeListItemQualifier( pragma[nomagic] private ItemNode resolveUseTreeListItem(Use use, UseTree tree) { tree = use.getUseTree() and - result = resolvePath(tree.getPath()) + result = resolvePathFull(tree.getPath()) or result = resolveUseTreeListItem(use, tree, tree.getPath()) } diff --git a/rust/ql/test/library-tests/dataflow/modeled/inline-flow.expected b/rust/ql/test/library-tests/dataflow/modeled/inline-flow.expected index 198f8ab9cd8..9ff1be3b3c5 100644 --- a/rust/ql/test/library-tests/dataflow/modeled/inline-flow.expected +++ b/rust/ql/test/library-tests/dataflow/modeled/inline-flow.expected @@ -29,8 +29,15 @@ edges | main.rs:41:17:41:41 | Wrapper {...} [Wrapper] | main.rs:41:13:41:13 | w [Wrapper] | provenance | | | main.rs:41:30:41:39 | source(...) | main.rs:41:17:41:41 | Wrapper {...} [Wrapper] | provenance | | | main.rs:42:15:42:15 | w [Wrapper] | main.rs:43:13:43:28 | Wrapper {...} [Wrapper] | provenance | | +| main.rs:42:15:42:15 | w [Wrapper] | main.rs:45:17:45:17 | w [Wrapper] | provenance | | | main.rs:43:13:43:28 | Wrapper {...} [Wrapper] | main.rs:43:26:43:26 | n | provenance | | | main.rs:43:26:43:26 | n | main.rs:43:38:43:38 | n | provenance | | +| main.rs:45:13:45:13 | u [Wrapper] | main.rs:46:15:46:15 | u [Wrapper] | provenance | | +| main.rs:45:17:45:17 | w [Wrapper] | main.rs:45:17:45:25 | w.clone() [Wrapper] | provenance | generated | +| main.rs:45:17:45:25 | w.clone() [Wrapper] | main.rs:45:13:45:13 | u [Wrapper] | provenance | | +| main.rs:46:15:46:15 | u [Wrapper] | main.rs:47:13:47:28 | Wrapper {...} [Wrapper] | provenance | | +| main.rs:47:13:47:28 | Wrapper {...} [Wrapper] | main.rs:47:26:47:26 | n | provenance | | +| main.rs:47:26:47:26 | n | main.rs:47:38:47:38 | n | provenance | | | main.rs:58:13:58:13 | b [Some] | main.rs:59:23:59:23 | b [Some] | provenance | | | main.rs:58:17:58:32 | Some(...) [Some] | main.rs:58:13:58:13 | b [Some] | provenance | | | main.rs:58:22:58:31 | source(...) | main.rs:58:17:58:32 | Some(...) [Some] | provenance | | @@ -71,6 +78,13 @@ nodes | main.rs:43:13:43:28 | Wrapper {...} [Wrapper] | semmle.label | Wrapper {...} [Wrapper] | | main.rs:43:26:43:26 | n | semmle.label | n | | main.rs:43:38:43:38 | n | semmle.label | n | +| main.rs:45:13:45:13 | u [Wrapper] | semmle.label | u [Wrapper] | +| main.rs:45:17:45:17 | w [Wrapper] | semmle.label | w [Wrapper] | +| main.rs:45:17:45:25 | w.clone() [Wrapper] | semmle.label | w.clone() [Wrapper] | +| main.rs:46:15:46:15 | u [Wrapper] | semmle.label | u [Wrapper] | +| main.rs:47:13:47:28 | Wrapper {...} [Wrapper] | semmle.label | Wrapper {...} [Wrapper] | +| main.rs:47:26:47:26 | n | semmle.label | n | +| main.rs:47:38:47:38 | n | semmle.label | n | | main.rs:58:13:58:13 | b [Some] | semmle.label | b [Some] | | main.rs:58:17:58:32 | Some(...) [Some] | semmle.label | Some(...) [Some] | | main.rs:58:22:58:31 | source(...) | semmle.label | source(...) | @@ -95,5 +109,6 @@ testFailures | main.rs:22:10:22:19 | b.unwrap() | main.rs:19:34:19:43 | source(...) | main.rs:22:10:22:19 | b.unwrap() | $@ | main.rs:19:34:19:43 | source(...) | source(...) | | main.rs:27:10:27:10 | a | main.rs:26:13:26:22 | source(...) | main.rs:27:10:27:10 | a | $@ | main.rs:26:13:26:22 | source(...) | source(...) | | main.rs:43:38:43:38 | n | main.rs:41:30:41:39 | source(...) | main.rs:43:38:43:38 | n | $@ | main.rs:41:30:41:39 | source(...) | source(...) | +| main.rs:47:38:47:38 | n | main.rs:41:30:41:39 | source(...) | main.rs:47:38:47:38 | n | $@ | main.rs:41:30:41:39 | source(...) | source(...) | | main.rs:63:22:63:22 | m | main.rs:58:22:58:31 | source(...) | main.rs:63:22:63:22 | m | $@ | main.rs:58:22:58:31 | source(...) | source(...) | | main.rs:85:18:85:34 | ...::read(...) | main.rs:84:32:84:41 | source(...) | main.rs:85:18:85:34 | ...::read(...) | $@ | main.rs:84:32:84:41 | source(...) | source(...) | diff --git a/rust/ql/test/library-tests/dataflow/modeled/main.rs b/rust/ql/test/library-tests/dataflow/modeled/main.rs index 648ffd0a307..3ce3e0ecae0 100644 --- a/rust/ql/test/library-tests/dataflow/modeled/main.rs +++ b/rust/ql/test/library-tests/dataflow/modeled/main.rs @@ -44,7 +44,7 @@ mod my_clone { } let u = w.clone(); match u { - Wrapper { n: n } => sink(n), // $ MISSING: hasValueFlow=73 - lack of expanded derives means that we cannot resolve clone call above, and hence not insert implicit borrow + Wrapper { n: n } => sink(n), // $ hasValueFlow=73 } } } diff --git a/rust/ql/test/query-tests/security/CWE-022/CONSISTENCY/PathResolutionConsistency.expected b/rust/ql/test/query-tests/security/CWE-022/CONSISTENCY/PathResolutionConsistency.expected new file mode 100644 index 00000000000..03a2899da09 --- /dev/null +++ b/rust/ql/test/query-tests/security/CWE-022/CONSISTENCY/PathResolutionConsistency.expected @@ -0,0 +1,41 @@ +multiplePathResolutions +| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:8:21:8:33 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:19:21:19:33 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:25:23:25:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:26:38:26:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:39:23:39:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:40:38:40:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:52:23:52:35 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from | +| src/main.rs:53:38:53:50 | ...::from | file://:0:0:0:0 | fn from |