Python: Remove imprecise container steps #2 by owen-mc · Pull Request #21888 · github/codeql

owen-mc · 2026-05-21T21:19:31Z

Supersedes #17493.

We used to have taint steps from any element of a collection to the entire collection (see here).
These are very imprecise, leading to false positives (e.g. seen #17008 (comment) and #16976).
They are also at odds with how other languages treat collections, see https://github.com/github/codeql-python-team/issues/728?reload=1 about this.

We wish to keep the semantics, that if a collection is tainted, then all elements are considered tainted. Therefor we now try to not taint collections, if we have precise information about which elements are tainted.
For a list, if an element is tainted, we do not know which one, so any read is potentially reading tainted information.
There is not much difference between the list having content and the list being tainted.
But for a dictionary, if an entry is tainted and we know which one, only reads of the appropriate key is reading tainted information. All other reads should ideally be considered safe (they used to not be). If we do not know that other keys are safe, e.g. if the collection came from an untrusted source, we can taint the collection itself, and all reads will be considered dangerous. So for collections with precise content, there is a big difference between having content and the collection being tainted.

Thus we wish to remove these imprecise taint steps for tuples and dictionaries, where we track content precisely (we keep them for lists and sets, where content is imprecise anyway).
Changes

In this PR we do the following:

remove tupleStoreStep and dictStoreStep from containerStep These are imprecise compared to the content being precise.
add implicit reads to recover taint at sinks
add implicit read steps for decoders to supplement the AdditionalTaintStep that now only covers when the full container is tainted.

Status:
Potential confusions:

A comprehension is no longer tainted even if it has tainted elements. See the taint test for Tornado for an example.
Dict.items is no longer tainted for a tainted dict (but Dict.values are). We could choose to change this.

Improvements:

Fixed FP in test_unpacking
Fixed FP in CleartextLogging
Nicer paths in NoSqlInjection test

Closes#17493.

- remove `tupleStoreStep` and `dictStoreStep` from `containerStep` These are imprecise compared to the content being precise. - add implicit reads to recover taint at sinks - add implicit read steps for decoders to supplement the `AdditionalTaintStep` that now only covers when the full container is tainted.

We now find an alert on this line as we hope to It is not an alert for _full_ SSRF, though, since that configuration cannot handle multiple substitutions.

and adjust collection test

owen-mc · 2026-05-27T14:55:00Z

The first DCA run showed major performance problems, as expected from the original PR. I tracked this down to the problem that ContentSet was created to address, but we weren't defining the correct ContentSets to fix the problem. I got copilot to fix that and ran DCA again. The results of this second DCA run were good: all the alert changes stayed the same, but performance improved, to the point where it is pretty much performance neutral on average (between a 4% speedup to a 5% slowdown). There is a small increase in analysis time for some proejcts due to NodeEx.toString, which seems unavoidable.

Copilot

Pull request overview

This PR refines Python’s new dataflow/taint tracking for containers by removing imprecise “element taints whole container” steps for tuples and dictionaries (where content is tracked precisely), and compensates by adding implicit-read mechanisms so taint can still be recovered at sinks and through certain conversions/decoders. It updates flow-summary/content-set plumbing to support wildcard content sets for better scalability and adjusts a broad set of query and library tests accordingly.

Changes:

Remove imprecise container bubbling for tuple/dict stores and introduce wildcard ContentSets plus implicit taint reads at sinks.
Add/adjust conversion-related read steps (e.g., decoders, % formatting, format_map) to preserve intended taint behavior without container-wide tainting.
Update query-test .expected baselines and library tests to reflect the new, more precise paths/alerts.

Show a summary per file

File	Description
python/ql/lib/semmle/python/dataflow/new/internal/TaintTrackingPrivate.qll	Adds default implicit read policy (wildcard tuple/dict element reads) and removes tuple/dict store bubbling from `containerStep`.
python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll	Introduces wildcard-capable `ContentSet` representation (`AnyTupleElement`/`AnyDictionaryElement`) and singleton wrapper helper.
python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll	Refactors read/store/clear steps to operate via singleton/wildcard `ContentSet`s and adds conversion read steps.
python/ql/lib/semmle/python/dataflow/new/internal/FlowSummaryImpl.qll	Updates summary encoding/summary-components to use singleton `ContentSet`s and adds encoding for wildcard sets.
python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackingImpl.qll	Wraps tracked content in singleton `ContentSet`s when delegating to summary flow.
python/ql/lib/semmle/python/frameworks/Stdlib.qll	Adjusts stdlib summary behavior to taint both list element content and (imprecisely) the list where appropriate.
python/ql/src/Variables/LoopVariableCapture/LoopVariableCaptureQuery.qll	Updates implicit read allowlist logic to use wildcard tuple/dict element checks and store-content inspection.
python/ql/consistency-queries/DataFlowConsistency.ql	Excludes new conversion read steps from consistency checks where appropriate.
python/ql/test/library-tests/frameworks/tornado/taint_test.py	Updates tornado taint expectations around comprehensions/element reads under the new container semantics.
python/ql/test/library-tests/frameworks/stdlib/test_re.py	Updates `re` modeling expectations to rely on implicit reads at sinks for Match object content.
python/ql/test/library-tests/dataflow/tainttracking/defaultAdditionalTaintStep/test_unpacking.py	Removes a now-fixed spurious taint expectation for tuple/list unpacking.
python/ql/test/library-tests/dataflow/tainttracking/defaultAdditionalTaintStep/test_collections.py	Adjusts dict `.items()` taint expectations to match new dict precision semantics.
python/ql/test/library-tests/dataflow/sensitive-data/test.py	Updates sensitive-data expectations for non-sensitive dict entries with precise dict content.
python/ql/test/query-tests/Security/CWE-943-NoSqlInjection/NoSqlInjection.expected	Updates expected NoSQL injection path output to reflect refined dict/tuple content steps.
python/ql/test/query-tests/Security/CWE-918-ServerSideRequestForgery/PartialServerSideRequestForgery.expected	Updates expected SSRF partial-path edges/nodes under new tuple content behavior.
python/ql/test/query-tests/Security/CWE-312-CleartextLogging/CleartextLogging.expected	Updates expected cleartext logging results (removes prior spurious dict cross-talk path).
python/ql/test/query-tests/Security/CWE-209-StackTraceExposure/StackTraceExposure.expected	Updates expected stack trace exposure path with refined dict/str conversion steps.
python/ql/test/query-tests/Security/CVE-2018-1281/BindToAllInterfaces.expected	Updates expected bind-to-all-interfaces results to reflect tuple element precision.
python/ql/test/experimental/query-tests/Security/CWE-1427-PromptInjection/PromptInjection.expected	Updates expected prompt-injection results to reflect reduced container-wide tainting.
python/ql/test/experimental/query-tests/Security/CWE-1427-PromptInjection/openai_test.py	Adjusts inline alert annotation expectations to match updated analysis behavior.

Copilot's findings

Files reviewed: 20/20 changed files
Comments generated: 3

+ [(k, v) for (k, v) in request.headers.get_all()], # The comprehension is not tainted, only the elements
+ list([(k, v) for (k, v) in request.headers.get_all()]), # Here, all the elements of the list are tainted, but the list is not.


-# since we have taint-step from store of `password`, we will consider any item in the
-# dictionary to be a password :(
-print(_config["sleep_timer"]) # $ SPURIOUS: SensitiveUse=password
+# since we have precise dictionary content, other items of the config are not tainted


+ # returns Match object, which is tested properly below. (note: the match objects contain
+ # tainted values but are not themselves tainted - this test relies on implicit reads at sinks).


owen-mc · 2026-05-28T08:47:27Z

I edited the ContentSet commit after reviewing what copilot did more closely, so I've rerun DCA. The results are still good. A few repos have a slight slowdown (up to 5%) which is caused by NodeEx.toString suddenly having to make extra strings with " [Ext]" on the end for all the data flow nodes. This is unavoidable with our current architecture. Still, the overall average performance affect is neutral. The ContentSet commit does not change alerts at all, so @yoff's previous analysis of them still stands.

hvitved · 2026-05-28T09:01:20Z

+ nodeFrom = decoding.getAnInput() and
+ nodeTo = decoding.getOutput()
+ ) and
+ (c.isAnyTupleElement() or c.isAnyDictionaryElement())


You could also add another TAnyTupleOrDictionaryElement to TContentSet to represent this union.

How would that be better?

It reduces the size of the predicate by 50 %; it may not matter much, but I also see this pattern elsewhere where having a dedicated ContentSet entity could remove code duplication as well.

At first sight it seemed weird to have a ContentSet which is the union of two other ones, but I guess there's no reason not to. I've done it in a new commit.

hvitved · 2026-05-28T09:04:45Z

@@ -176,10 +186,6 @@ predicate containerStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
 or
 DataFlowPrivate::setStoreStep(nodeFrom, _, nodeTo)


Is there a reason why this PR doesn't also fix the issue with lists and sets?

hvitved · 2026-05-28T09:06:52Z

@@ -176,10 +186,6 @@ predicate containerStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
 or
 DataFlowPrivate::setStoreStep(nodeFrom, _, nodeTo)
 or


I would expect there to be something like
exists(ContentSet cs | readStep(nodeFrom, cs, nodeTo) | cs.isAnyTupleElement() or cs.isAnyDictionaryElement() )
See e.g.
codeql/rust/ql/lib/codeql/rust/dataflow/internal/TaintTrackingImpl.qll
Lines 57 to 63 in 17fe3e4
// Read steps give rise to taint steps. This has the effect that if `foo`
// is tainted and an operation reads from `foo` (e.g., `foo.bar`) then
// taint is propagated.
exists(ContentSetcs|
DataFlow::readStep(pred,cs,succ)and
notexcludedTaintStepContent(cs.getAReadContent())
)

I just didn't see it. I've added a commit for it now - see what you think.

Thanks, this is exactly what I meant.

hvitved · 2026-05-28T18:37:44Z

 TAnyTupleElement() or
- TAnyDictionaryElement()
+ TAnyDictionaryElement() or
+ TAnyTupleOrDictionaryElement()


Needs logic in getAReadContent.

Oops. Added in a new commit.

hvitved · 2026-05-29T10:02:38Z

- DataFlowPrivate::yieldStoreStep(nodeFrom, _, nodeTo)
+ exists(DataFlow::ContentSet contentSet |
+ DataFlowPrivate::readStep(nodeFrom, contentSet, nodeTo) and
+ defaultTaintReadContent(contentSet)


Here you will instead need
exists(Content c | c = contentSet.getAReadContent() | c instanceof TupleElementContent or c instanceof DictionaryElementContent or c instanceof DictionaryElementAnyContent )

Addressed in this commit, though I think that has made the python language tests start failing. Did I misunderstand what you suggested?

They were already failing prior to that change; I spot checked a few, and it actually looks like good changes to the expected test output.

Ah, I missed that. I think I've fixed them now.

No changes to alerts

hvitved

A few last comments.

hvitved · 2026-06-02T18:34:21Z

+ }
+
+ override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+ input = ["Argument[0,iterable:]", "Argument[0,iterable:].ListElement"] and


Since read steps are also lifted to taint steps, "Argument[0,iterable:]" should not be needed.

Then a test fails on python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/src/unsafe_shell_test.py:11, on " ".join(name).

I've kept both but made the ListElement one preserves = true and the other one preserves = false.

hvitved · 2026-06-02T18:40:09Z

- // TODO: We need to also translate iterable content such as list element
- // but we currently lack TupleElementAny
- input = "Argument[0]" and
+ input = ["Argument[0]", "Argument[0].ListElement"] and


Again, "Argument[0]" can be removed.

hvitved · 2026-06-02T18:40:42Z

- input = "Argument[0]" and
+ input = ["Argument[0]", "Argument[0].ListElement"] and
 output = "ReturnValue" and
 preservesValue = false


Should this now be true?

hvitved · 2026-06-02T18:48:37Z

+ // Since list content is imprecise, we also taint the list.
+ output = "ReturnValue" and
+ preservesValue = false


Is this part still needed?

👍🏻 to removing it. (Assuming CI passes.)

owen-mc · 2026-06-03T22:19:18Z

Once this is approved I will run DCA again before merging, as it's changed quite a bit since the last run.

hvitved

Great to finally have this Python tech debt fixed 🎉

hvitved · 2026-06-04T07:04:27Z

I have started a DCA run.

owen-mc · 2026-06-04T08:39:39Z

Oops, I didn't scroll down far enough to read your comment and I've started a DCA run too 😆 .

hvitved · 2026-06-04T12:31:14Z

DCA looks fine to me; the slowdown on some projects is acceptable IMO.

yoff added 9 commits May 21, 2026 16:57

Python: recover taint for % format strings
facb3b6

Python: adjust test expectations
93e7ab5
We now find an alert on this line as we hope to It is not an alert for _full_ SSRF, though, since that configuration cannot handle multiple substitutions.

Python: conversion step for format_map
9a18003
and adjust collection test

Python: reset test expectations
3275c81

Python: Make sure all imprecise taint bubbles up
f669a4f

Python: typo
0ecca91

Python: extra tests for comprehension
fa9426c

python: fix test
fa758d6

github-actionsBot added the Python label May 21, 2026

Update test results
e877929

owen-mc force-pushed the py/remove-imprecise-container-steps branch 2 times, most recently from 1d66c7b to 20fadc8Compare May 23, 2026 06:06

Add wildcard ContentSets to avoid performance problems
ec13e1b

owen-mc force-pushed the py/remove-imprecise-container-steps branch from 20fadc8 to ec13e1bCompare May 27, 2026 14:30

owen-mc marked this pull request as ready for review May 27, 2026 20:11

owen-mc requested a review from a team as a code owner May 27, 2026 20:11

CopilotAI review requested due to automatic review settings May 27, 2026 20:11

Copilotstarted reviewing on behalf of owen-mc May 27, 2026 20:11 View session

owen-mc requested a review from tausbn May 27, 2026 20:11

CopilotAI reviewed May 27, 2026
View reviewed changes

hvitved reviewed May 28, 2026
View reviewed changes

owen-mc added 2 commits May 28, 2026 11:34

Fix TODO in containerStep
80c6f08

Add change note
812e8e6

github-actionsBot added the documentation label May 28, 2026

Add a ContentSet for any tuple or dictionary element
df15a71

hvitved reviewed May 28, 2026
View reviewed changes

Add missing code for TAnyTupleOrDictionaryElement
aee33a0

hvitved reviewed May 29, 2026
View reviewed changes

Address review comment
b384404

owen-mc force-pushed the py/remove-imprecise-container-steps branch from 815b9a0 to b384404Compare May 31, 2026 20:48

owen-mc added 6 commits June 2, 2026 16:14

Use access path for str.join model
ad97b6d

Track flow through tuple() with list with tainted elements
dede5bc

Add MaD models for lxml and xml etree.fromstringlist
c3ef1dd

Adjust expected test output
f62ebef

Accept changed edges in test output
20ce679
No changes to alerts

Update edges in expected test output
b27d08e

hvitved reviewed Jun 2, 2026
View reviewed changes

owen-mc added 3 commits June 2, 2026 21:59

Tweak model for str.join
04341c4

Remove imprecise model for list()
5042fde

Remove imprecise model for tuple()
6f2cc43

hvitved reviewed Jun 3, 2026
View reviewed changes

Comment threadpython/ql/lib/semmle/python/dataflow/new/internal/TaintTrackingPrivate.qll Outdated
Comment threadpython/ql/lib/semmle/python/frameworks/Stdlib.qll Outdated

Address review comments
da999ee

owen-mc requested a review from hvitved June 3, 2026 21:08

hvitved approved these changes Jun 4, 2026
View reviewed changes

owen-mc merged commit 1f91f91 into github:mainJun 4, 2026
19 checks passed

owen-mc deleted the py/remove-imprecise-container-steps branch June 4, 2026 21:16

hvitved mentioned this pull request Jun 8, 2026
Python: Implement ContentApprox#21941
Merged

hvitved mentioned this pull request Jul 1, 2026
Python: Improve some flow summaries #22101
Merged

yoff mentioned this pull request Jul 2, 2026
Python: switch dataflow library to new (shared) CFG + SSA #21925
Open

		[(k, v) for (k, v) in request.headers.get_all()], # The comprehension is not tainted, only the elements
		list([(k, v) for (k, v) in request.headers.get_all()]), # Here, all the elements of the list are tainted, but the list is not.

		# returns Match object, which is tested properly below. (note: the match objects contain
		# tainted values but are not themselves tainted - this test relies on implicit reads at sinks).

		@@ -176,10 +186,6 @@ predicate containerStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
		or
		DataFlowPrivate::setStoreStep(nodeFrom, _, nodeTo)

	// Read steps give rise to taint steps. This has the effect that if `foo`
	// is tainted and an operation reads from `foo` (e.g., `foo.bar`) then
	// taint is propagated.
	exists(ContentSetcs\|
	DataFlow::readStep(pred,cs,succ)and
	notexcludedTaintStepContent(cs.getAReadContent())
	)

Uh oh!

Conversation

owen-mc commented May 21, 2026

Uh oh!

owen-mc commented May 27, 2026

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

owen-mc commented May 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owen-mcJun 1, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

owen-mc commented Jun 3, 2026

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

hvitved commented Jun 4, 2026

Uh oh!

owen-mc commented Jun 4, 2026

Uh oh!

hvitved commented Jun 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

owen-mcJun 1, 2026•
edited
Loading