fix(constant-time-analysis): restore dis offset column on Python 3.13+ - #200
Open
farhan6667 wants to merge 1 commit into
Open
fix(constant-time-analysis): restore dis offset column on Python 3.13+#200farhan6667 wants to merge 1 commit into
farhan6667 wants to merge 1 commit into
Conversation
Python 3.13 stopped printing the bytecode byte-offset column in `python -m dis` output by default (see What's New in Python 3.13), adding `-O`/`--show-offsets` to opt back in. PythonAnalyzer's _parse_dis_output regex requires that offset column, so on 3.13+ every instruction line silently failed to match: total_instructions stayed at 0 and the analyzer reported "PASSED, no violations" on code with genuine timing side-channels (e.g. an early-exit byte-by-byte comparison on secret data). _get_dis_output now detects --show-offsets support at runtime and passes it only when available, so it keeps working unmodified on Python < 3.13 where the flag doesn't exist. Verified against the bundled tests/test_samples/vulnerable.py sample: before this change it reported 0 instructions analyzed and "PASSED"; after, it correctly reports 328 instructions and multiple violations. Added a regression test that would have caught this.
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PythonAnalyzerinplugins/constant-time-analysis/ct_analyzer/script_analyzers.pycallspython -m dis <file>and parses the output with a regex that requires a numeric byte-offset column before each instruction mnemonic.Python 3.13 stopped printing that offset column by default (see What's New in Python 3.13) and added
-O/--show-offsetsto opt back in. Without that flag on 3.13+, every instruction line in_parse_dis_outputsilently fails to match:total_instructionsstays0and the analyzer reports "PASSED, no violations" on Python code with genuine timing side-channels.Reproduced with the plugin's own bundled sample:
The same is true for a minimal early-exit secret comparison (the canonical example this tool exists to catch):
Fix
_get_dis_outputnow detects whether--show-offsetsis supported (viapython -m dis --help) and passes it only when available, so behavior on Python < 3.13 (which doesn't have the flag) is unchanged.After the fix, on Python 3.13:
Testing
test_get_dis_output_includes_offsets_on_py313_plus, a regression test that fails without this fix on Python 3.13+ (skips itself on older Python where--show-offsetsdoesn't exist).python3 -m pytest ct_analyzer/tests/test_analyzer.py— 59 passed, 1 pre-existing failure unrelated to this change (test_cross_compile_arm64fails in my sandbox due to missing local glibc headers for arm64 cross-compilation, not related to the Python analyzer).ruff checkandruff format --diffboth clean on the two changed files.Note on the existing integration test
test_python_vulnerable_detectedalready exercises this exact path but its assertion isif report.error_count > 0: ..., which passes vacuously whenerror_countstays0— that's how this false negative shipped without CI catching it. I left that test as-is to keep this PR focused on the root-cause fix plus one new regression test that would actually have caught it; happy to also tighten that assertion in this PR or a follow-up if maintainers prefer.