Skip to content

Commit c91054d

Browse files
fix(iast): avoid ast patch subscript if in store context [backport 2.5] (#8538)
Backport e660622 from #8536 to 2.5. IAST: Fix an issue where we were unintentionally patching subscript operations that were not reading a value (but storing or deleting instead). ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [x] If change touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) Co-authored-by: Federico Mon <[email protected]>
1 parent a2aa055 commit c91054d

File tree

4 files changed

+19
-0
lines changed

4 files changed

+19
-0
lines changed

ddtrace/appsec/_iast/_ast/visitor.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,11 @@ def visit_Subscript(self, subscr_node): # type: (ast.Subscript) -> Any
709709
if hasattr(subscr_node, "avoid_convert"):
710710
return subscr_node
711711

712+
# We only want to convert subscript nodes that are being used as a load.
713+
# That means, no Delete or Store contexts.
714+
if not isinstance(subscr_node.ctx, ast.Load):
715+
return subscr_node
716+
712717
# Optimization: String literal slices and indexes are not patched
713718
if self._is_string_node(subscr_node.value):
714719
return subscr_node
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
fixes:
3+
- |
4+
Vulnerability Management for Code-level (IAST): This fix addresses AST patching issues where other subscript operations than ``Load`` were being unintentionally patched, leading to compilation errors for the patched module.

tests/appsec/iast/_ast/test_ast_patching.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,7 @@ def test_astpatch_source_changed_with_future_imports(module_name):
112112
("tests.appsec.iast.fixtures.ast.str.__init__"), # Empty __init__.py
113113
("tests.appsec.iast.fixtures.ast.str.non_utf8_content"), # EUC-JP file content
114114
("tests.appsec.iast.fixtures.ast.str.empty_file"),
115+
("tests.appsec.iast.fixtures.ast.subscript.store_context"),
115116
],
116117
)
117118
def test_astpatch_source_unchanged(module_name):
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
#!/usr/bin/env python3
2+
3+
4+
def fixture_subscript_store_context():
5+
my_list = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
6+
7+
for i in range(10):
8+
for my_list[i] in range(10):
9+
yield my_list

0 commit comments

Comments
 (0)