Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions unified/ql/consistency-queries/CfgConsistency.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import unified
import ControlFlow::Consistency
5 changes: 5 additions & 0 deletions unified/ql/consistency-queries/qlpack.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
name: codeql/unified-consistency-queries
groups: [unified, test, consistency-queries]
dependencies:
codeql/unified-all: ${workspace}
warnOnImplicitThis: true
277 changes: 277 additions & 0 deletions unified/ql/lib/codeql/unified/internal/ControlFlowGraph.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,277 @@
/**
* Provides classes representing the control flow graph within callables.
*/
overlay[local?]
module;

private import unified
private import codeql.controlflow.ControlFlowGraph
private import codeql.controlflow.SuccessorType

private module Cfg0 = Make0<Location, Ast>;

private module Cfg1 = Make1<Input>;

private module Cfg2 = Make2<Input>;

private import Cfg0
private import Cfg1
private import Cfg2
import Public

/** Provides an implementation of the AST signature for Unified. */
private module Ast implements AstSig<Location> {
private import unified as U

class AstNode = U::AstNode;

private predicate skipControlFlow(AstNode e) { e instanceof Modifier or e instanceof Identifier }

AstNode getChild(AstNode n, int index) {
result.getParent() = n and

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

'getEnclosingCallable' recurses through parents but stops only when the parent is a 'Callable'.

Impact: 'getEnclosingCallable' recurses through parents but stops only when the parent is a 'Callable'. If an AST node is not enclosed by any callable (e.g., top-level declarations outside a 'TopLevel' body), the recursion can fail to produce a result or traverse unexpectedly, leaving CFG nodes without an enclosing callable and breaking scope-based queries.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

result.getParentIndex() = index and
not n instanceof Callable and
not skipControlFlow(n) and
not skipControlFlow(result)
}

Callable getEnclosingCallable(AstNode node) {
exists(AstNode parent | parent = node.getParent() |
result = parent
or
not parent instanceof Callable and
result = getEnclosingCallable(parent)
)
}

class Callable = U::Callable;

AstNode callableGetBody(Callable c) {
result = c.(AccessorDeclaration).getBody() or
result = c.(ConstructorDeclaration).getBody() or
result = c.(DestructorDeclaration).getBody() or
result = c.(FunctionDeclaration).getBody() or
result = c.(FunctionExpr).getBody() or
result = c.(InitializerDeclaration).getBody() or
result = c.(TopLevel).getBody()
}

class Parameter extends U::Parameter {
Expr getDefaultValue() { result = super.getDefault() }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · CRITICAL

The 'Ast' module implementing 'AstSig<Location>' contains numerous stub classes with 'none()' bodies (e.g., 'ExprStmt', 'IfStmt', 'ForStmt', 'GotoStmt', 'Assignment', 'UntilStmt').

Impact: The 'Ast' module implementing 'AstSig<Location>' contains numerous stub classes with 'none()' bodies (e.g., 'ExprStmt', 'IfStmt', 'ForStmt', 'GotoStmt', 'Assignment', 'UntilStmt'). Any CFG construction that encounters these node types will silently produce no control-flow edges, yielding an incomplete or incorrect CFG. This can cause downstream analyses (e.g., taint, dataflow, dead-code) to miss paths and prod…

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.


AstNode getPattern() { result = super.getPattern() }
}

Parameter callableGetParameter(Callable c, int index) {
result = c.(AccessorDeclaration).getParameter(index) or
result = c.(ConstructorDeclaration).getParameter(index) or
result = c.(FunctionDeclaration).getParameter(index) or
result = c.(FunctionExpr).getParameter(index)
}

class Stmt = U::Stmt;

class Expr = U::Expr;

class BlockStmt = U::Block;

class ExprStmt extends Stmt {
ExprStmt() { none() }

Expr getExpr() { none() }
}

class IfStmt extends Stmt {
IfStmt() { none() }

Expr getCondition() { none() }

Stmt getThen() { none() }

Stmt getElse() { none() }
}

abstract class LoopStmt extends Stmt {
Stmt getBody() { none() }
}

class WhileStmt extends LoopStmt instanceof U::WhileStmt {
override Stmt getBody() { result = U::WhileStmt.super.getBody() }

Expr getCondition() { result = super.getCondition() }
}

class DoStmt extends LoopStmt instanceof U::DoWhileStmt {
override Stmt getBody() { result = U::DoWhileStmt.super.getBody() }

Expr getCondition() { result = super.getCondition() }
}

class UntilStmt extends LoopStmt {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The 'Ast' module is a large hand-written adapter with many 'none()' stubs and TODO comments (e.g., '// TODO support foreach guard', '// TODO: sort out the relationship between Bina

Impact: The 'Ast' module is a large hand-written adapter with many 'none()' stubs and TODO comments (e.g., '// TODO support foreach guard', '// TODO: sort out the relationship between BinaryExpr and Assignment'). A new maintainer cannot determine which language constructs are actually supported by the CFG without exhaustively reading every stub. This is a maintainability hazard and likely to cause incorrect assumptions abou…

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

UntilStmt() { none() }

Expr getCondition() { none() }
}

class ForStmt extends LoopStmt {
ForStmt() { none() }

AstNode getInit(int index) { none() }

Expr getCondition() { none() }

AstNode getUpdate(int index) { none() }
}

class ForeachStmt extends LoopStmt instanceof U::ForEachStmt {
override Stmt getBody() { result = U::ForEachStmt.super.getBody() }

// TODO support foreach guard
//
// TODO: Expr != Pattern
Expr getVariable() { result = super.getPattern() }

Expr getCollection() { result = super.getIterable() }
}

class BreakStmt = U::BreakExpr;

class ContinueStmt = U::ContinueExpr;

class GotoStmt extends Stmt {
GotoStmt() { none() }
}

class ReturnStmt extends U::ReturnExpr {
Expr getExpr() { result = super.getValue() }
}

class Throw extends U::ThrowExpr {
Expr getExpr() { result = super.getValue() }
}

class TryStmt extends U::TryExpr {
AstNode getBody(int index) { index = 0 and result = super.getBody() }

CatchClause getCatch(int index) { result = super.getCatchClause(index) }

Stmt getFinally() { none() }
}

class CatchClause extends U::CatchClause {
AstNode getPattern() { result = super.getPattern() }

AstNode getVariable() { none() }

Expr getCondition() { none() }

Stmt getBody() { result = super.getBody() }
}

class Switch extends U::SwitchExpr {
Expr getExpr() { result = super.getValue() }

Case getCase(int index) { result = super.getCase(index) }

Stmt getStmt(int index) { none() }
}

class Case extends U::SwitchCase {
AstNode getPattern(int index) { result = super.getPattern() and index = 0 }

Expr getGuard() { none() }

AstNode getBody() { result = super.getBody() }
}

class DefaultCase extends Case {
DefaultCase() { not exists(super.getPattern()) }
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · CRITICAL

'BooleanLiteral.getValue()' is implemented as 'result.toString() = super.getValue()'.

Impact: 'BooleanLiteral.getValue()' is implemented as 'result.toString() = super.getValue()'. This compares a string representation of a boolean result to the raw value, which is type-incompatible or always false depending on QL type resolution. If this compiles, it will never produce a true value, breaking CFG handling of boolean literals and any conditional logic depending on them.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

class ConditionalExpr = U::IfExpr;

// TODO: sort out the relationship between BinaryExpr and Assignment
class BinaryExpr extends U::BinaryExpr {
Expr getLeftOperand() { result = super.getLeft() }

Expr getRightOperand() { result = super.getRight() }
}

class LogicalAndExpr extends BinaryExpr, U::LogicalAndExpr { }

class LogicalOrExpr extends BinaryExpr, U::LogicalOrExpr { }

class NullCoalescingExpr extends BinaryExpr, U::NullCoalescingExpr { }

class UnaryExpr = U::UnaryExpr;

class LogicalNotExpr = U::LogicalNotExpr;

// TODO
class Assignment extends BinaryExpr {
Assignment() { none() }
}

class AssignExpr extends Assignment { }

class CompoundAssignment extends Assignment { }

class AssignLogicalAndExpr extends CompoundAssignment { }

class AssignLogicalOrExpr extends CompoundAssignment { }

class AssignNullCoalescingExpr extends CompoundAssignment { }

class BooleanLiteral extends U::BooleanLiteral {
boolean getValue() { result.toString() = super.getValue() }
}

class PatternMatchExpr extends U::PatternGuardExpr {
Expr getExpr() { result = super.getValue() }

AstNode getPattern() { result = super.getPattern() }
}
}

private module Input implements InputSig1, InputSig2 {
private import codeql.util.Void

predicate cfgCachedStageRef() { CfgCachedStage::ref() }

class Label extends string {
Label() {
any(LabeledStmt l).getLabel().getValue() = this or
any(BreakExpr b).getLabel().getValue() = this or
any(ContinueExpr c).getLabel().getValue() = this
}

string toString() { result = this }
}

private Label getLabelOfStmt(Stmt s) {
exists(LabeledStmt l | s = l.getStmt() |
result = l.getLabel().getValue() or
result = getLabelOfStmt(l)
)
}

predicate hasLabel(Ast::AstNode n, Label l) {
l = getLabelOfStmt(n)
or
l = n.(BreakExpr).getLabel().getValue()
or
l = n.(ContinueExpr).getLabel().getValue()
}

class CallableContext = Void;

predicate beginAbruptCompletion(
AstNode ast, PreControlFlowNode n, AbruptCompletion c, boolean always
) {
none()
}

predicate endAbruptCompletion(AstNode ast, PreControlFlowNode n, AbruptCompletion c) { none() }

predicate step(PreControlFlowNode n1, PreControlFlowNode n2) { none() }
}
8 changes: 8 additions & 0 deletions unified/ql/lib/codeql/unified/internal/FacadeAst.qll
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ module Unified {
}
}

/** A block statement. */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

'Block.getLastStmt()' uses 'not exists(this.getStmt(i + 1))' to find the last statement.

Impact: 'Block.getLastStmt()' uses 'not exists(this.getStmt(i + 1))' to find the last statement. This relies on 'getStmt' indices being contiguous and starting at 0. If the underlying AST representation has gaps or non-zero-based indexing, this predicate will return no result or the wrong statement, and the intent is not documented.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

class Block extends G::Block {
/** Gets the last statement in this block. */
Stmt getLastStmt() {
exists(int i | result = this.getStmt(i) and not exists(this.getStmt(i + 1)))
}
}

/** An expression */
class Expr extends G::Expr {
/** Gets the string value of this expression, if it is a known string constant. */
Expand Down
40 changes: 40 additions & 0 deletions unified/ql/lib/ide-contextual-queries/printCfg.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/**

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The new 'printCfg.ql' query is tagged 'ide-contextual-queries/print-cfg' and exposes a graph representation of a file's CFG via external predicates ('selectedSourceFile', 'selected

Impact: The new 'printCfg.ql' query is tagged 'ide-contextual-queries/print-cfg' and exposes a graph representation of a file's CFG via external predicates ('selectedSourceFile', 'selectedSourceLine', 'selectedSourceColumn'). If this query is runnable in an environment where source file paths or line/column inputs are attacker-influenced, it could be used to exfiltrate code structure or probe internal source layout. The que…

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

* @name Print CFG
* @description Produces a representation of a file's Control Flow Graph.
* This query is used by the VS Code extension.
* @id unified/print-cfg
* @kind graph
* @tags ide-contextual-queries/print-cfg
*/

private import unified
private import codeql.Locations

external string selectedSourceFile();

private predicate selectedSourceFileAlias = selectedSourceFile/0;

external int selectedSourceLine();

private predicate selectedSourceLineAlias = selectedSourceLine/0;

external int selectedSourceColumn();

private predicate selectedSourceColumnAlias = selectedSourceColumn/0;

module ViewCfgQueryInput implements ControlFlow::ViewCfgQueryInputSig<File> {
predicate selectedSourceFile = selectedSourceFileAlias/0;

predicate selectedSourceLine = selectedSourceLineAlias/0;

predicate selectedSourceColumn = selectedSourceColumnAlias/0;

predicate cfgScopeSpan(
Callable scope, File file, int startLine, int startColumn, int endLine, int endColumn
) {
file = scope.getFile() and
scope.getLocation().hasLocationInfo(_, startLine, startColumn, endLine, endColumn)
}
}

import ControlFlow::ViewCfgQuery<File, ViewCfgQueryInput>
3 changes: 2 additions & 1 deletion unified/ql/lib/qlpack.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ extractor: unified
library: true
upgrades: upgrades
dependencies:
codeql/util: ${workspace}
codeql/controlflow: ${workspace}
codeql/namebinding: ${workspace}
codeql/util: ${workspace}
warnOnImplicitThis: true
compileForOverlayEval: true
1 change: 1 addition & 0 deletions unified/ql/lib/unified.qll
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ import codeql.Locations
import codeql.files.FileSystem
import codeql.unified.internal.Ast::UnifiedFinal
import codeql.unified.internal.AstExtra::Public
import codeql.unified.internal.ControlFlowGraph
import codeql.unified.internal.LocalNameBinding::Public