Skip to content

Rust: Source/sink/barrier MaD trait models apply to implementations - #3

Open
anurag6569201 wants to merge 1 commit into
qa/agent-github-codeql/pr-03-22445/basefrom
qa/agent-github-codeql/pr-03-22445/head
Open

anurag6569201 wants to merge 1 commit into
qa/agent-github-codeql/pr-03-22445/basefrom
qa/agent-github-codeql/pr-03-22445/head

Conversation

@anurag6569201

Copy link
Copy Markdown

This PR applies the same inheritance logic to source/sink/barrier MaD models for trait functions as for summary models, that is, a model for <Foo>::bar applies to any function implementing the trait function foo, unless an explicit model exists for that particular implementation.

In order to ensure that the same prioritization rules apply as for summary models, this logic is defined in the shared library, but currently only Rust and C++ make use of it.

The updated test output shows that we fix some existing FNs, and DCA shows that we both remove some alerts (presumably because we now have more barriers) and we gain some alerts (presumably because we now have more sources and sinks).

Source merge-base: 56dac46817d63cf9ca36d160e5f03249b65e8b74
Source head: 2d69fc39e538a8c7d92357e327769add57e99778

@shipwright-agent

Copy link
Copy Markdown

⚠️ Shipwright · Approve with conditions

Recommendation: approve PR #3 with conditions · Tier T3
Checks: 0 total · 0 needing attention

Next step: an authorized approver must satisfy the approval condition.

Findings (6)

  • HIGH The new 'RelevantModel' abstraction uses a higher-order 'modelSig/6' and a 'modelKind' string to unify summary/source/sink/barrier/barrier-guard relevance. · shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll:515
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH The shared 'RelevantModel' predicate changes precedence semantics for generated vs manual models. · shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll:515
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH In the Rust 'interpretPath' predicate, the disjunction 'f = f0 and isExact = true and p = orig or f.implements(f0) and isExact = false and p = "hq-generated"' has no parentheses ar · rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll:177
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH The change removes the explicit 'SourceBase'/'SinkBase' abstraction and makes 'SourceElement', 'SinkElement', 'BarrierElement', and 'BarrierGuardElement' all extend 'SummarizedCall · shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll:345
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH The Rust 'interpretPath' change now applies inherited trait models with 'isExact = false' and provenance '"hq-generated"' for sources, sinks, barriers, and barrier guards. · rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll:177
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • LOW The Rust 'FlowSourceFromModel'/'FlowSinkFromModel' classes now store 'kind_', 'orig', 'p_', and 'isExact_' as fields, but the constructor binds 'kind_' while 'isSource'/'isSink' re · rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll:256
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

Conditions

  • human approval required (T3): apply the approval label

Fireworks usage: 46,924 input · 970 output · 47,894 total tokens · $0.0110 · 18s · 0 fix iteration(s)

Open the Shipwright check for full evidence and the audit bundle. Use /shipwright rerun to verify again.

)
}

bindingset[path]

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

In the Rust 'interpretPath' predicate, the disjunction 'f = f0 and isExact = true and p = orig or f.implements(f0) and isExact = false and p = "hq-generated"' has no parentheses ar

Impact: In the Rust 'interpretPath' predicate, the disjunction 'f = f0 and isExact = true and p = orig or f.implements(f0) and isExact = false and p = "hq-generated"' has no parentheses around the two alternatives. In QL, 'and' binds tighter than 'or', so this parses as intended, but the missing grouping is fragile and easy to break during refactoring; a misplaced edit could make 'isExact'/'p' unconstrained or alter precede…

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

)
}

bindingset[path]

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 Rust 'interpretPath' change now applies inherited trait models with 'isExact = false' and provenance '"hq-generated"' for sources, sinks, barriers, and barrier guards.

Impact: The Rust 'interpretPath' change now applies inherited trait models with 'isExact = false' and provenance '"hq-generated"' for sources, sinks, barriers, and barrier guards. The test expectations show new inherited models appearing (e.g., '<_ as main::MySinkTrait3>::consume3'). This expands the attack surface of model propagation to all implementors of a trait, which can cause over-approximation and false positi…

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant