π‘οΈ Sentinel: [MEDIUM] Fix DoS vulnerability in render_diagnostics_report - #264
π‘οΈ Sentinel: [MEDIUM] Fix DoS vulnerability in render_diagnostics_report#264seonghobae wants to merge 1 commit into
Conversation
Switched from `json.loads` to `_load_json_bounded` to prevent Denial of Service attacks from deeply nested or overly large JSON files.
|
π Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a π emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR hardens render_diagnostics_report against JSON parsing DoS by replacing unbounded file reads and json.loads with the projectβs bounded JSON loader, aligning report rendering with existing defensive IO utilities.
Changes:
- Replaced
json.loads(Path.read_text())with_load_json_bounded(...)to enforce input byte limits and JSON nesting depth limits. - Removed the now-unused
jsonimport and adjusted imports accordingly. - Documented the JSON parsing DoS learning/prevention in
.jules/sentinel.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python/fast_mlsirm/report.py | Switches diagnostics JSON loading to _load_json_bounded to prevent memory/recursion DoS during report rendering. |
| .jules/sentinel.md | Adds a Sentinel entry documenting the JSON parsing DoS vector and the preferred bounded loader. |
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| **Learning:** For non-security purposes like generating unique dom ids, `hashlib.md5()` triggers a vulnerability warning unless `usedforsecurity=False` is passed. This allows bypassing FIPS compliance limitations as well as suppressing false positive warnings. | ||
| **Prevention:** Always add `usedforsecurity=False` parameter to `hashlib.md5` and other weak hashing functions unless they are genuinely used for secure cryptography (which they shouldn't be). | ||
|
|
||
| ## 2024-07-25 - [JSON Parsing DoS via Unbounded Recursion/Memory Allocation] |
|
μ€λ³΅ μ 리: Generated by Claude Code |
Understood. Acknowledging that this work is a duplicate of PR #203 and stopping work on this task. |
π¨ Severity: MEDIUM
π‘ Vulnerability:
python/fast_mlsirm/report.pyμrender_diagnostics_reportν¨μκ° μ§λ¨ κ²°κ³Ό JSON νμΌμ νμ±ν λ ν¬κΈ°λ μ€μ²© κΉμ΄μ μ νμ΄ μλ λ΄μ₯json.loadsμPath.read_text()λ₯Ό μ¬μ©νκ³ μμμ΅λλ€.π― Impact: μ μμ μΌλ‘ μ‘°μλ κΉκ² μ€μ²©λ JSON λ°°μ΄/κ°μ²΄ λλ λ§€μ° ν° ν¬κΈ°μ JSON νμΌμ μ λ¬λ°μ κ²½μ°
RecursionErrorκ° λ°μνκ±°λ OOM(Out of Memory)μΌλ‘ μΈν΄ νλ‘μΈμ€κ° κ°μ μ’ λ£λλ μλΉμ€ κ±°λΆ(DoS) 곡격μ μ·¨μ½νμ΅λλ€.π§ Fix:
json.loads(source.read_text())νΈμΆμ νλ‘μ νΈ λ΄μ μμ ν μ νΈλ¦¬ν°μΈ_load_json_bounded(source, source='diagnostics JSON')νΈμΆλ‘ κ΅μ²΄νμ¬ μ λ ₯ λ°μ΄νΈ μ νκ³Ό μ€μ²© κΉμ΄ μ νμ κ°μ νμ΅λλ€. μ΄ κ³Όμ μμ μ¬μ©λμ§ μκ² λjsonimportλ₯Ό μ κ±°νκ³ μ½λ μμμ ν¬λ§·ν νμ΅λλ€.β Verification:
ruff format) λ° μ μ λΆμ(ruff check)μ μννμ΅λλ€.uv run pytest tests)λ₯Ό λͺ¨λ ν΅κ³Όνμ΅λλ€.json.loadsλμ_load_json_boundedλ₯Ό μ¬μ©νμ¬ κΉκ² μ€μ²©λ λ°°μ΄μ μ λ ₯νμ λ,ValueError(exceeds the maximum JSON nesting depth) λ± μ¬μ μ μ€κ³λ μ μ΄λ μλ¬λ‘ μ²λ¦¬λ¨μ μλ κ²μ¦ μ€ν¬λ¦½νΈλ‘ νμΈνμ΅λλ€.PR created automatically by Jules for task 7868236832688815057 started by @seonghobae