🛡️ Sentinel: [MEDIUM] Fix Unrestricted Audio File Loading (DoS)

[Whamp]

User description

🛡️ Sentinel: [MEDIUM] Fix Unrestricted Audio File Loading (DoS)

🚨 Severity: MEDIUM

💡 Vulnerability

The AudioFeedback class (used for start/stop sounds) loaded audio files into memory using wave.readframes without checking the file size. If a user configuration pointed start_sound_path or stop_sound_path to a very large file (e.g., gigabytes), this would cause the application to consume excessive memory and crash (Denial of Service).

🎯 Impact

A malicious or accidental configuration could crash the application or the host system by exhausting available RAM.

🔧 Fix

• Added a constant MAX_AUDIO_FILE_SIZE_BYTES = 5 * 1024 * 1024 (5MB).
• Updated AudioFeedback._load_and_cache to check path.stat().st_size before opening the file.
• Raises a ValueError if the file exceeds the limit, which is caught and logged as a warning, preventing the crash.

✅ Verification

1. New Security Test: tests/test_audio_security.py confirms that attempting to load a mocked 6MB file raises ValueError.
2. Regression Testing: tests/test_audio_feedback.py confirms that standard functionality (loading small files) works as expected (tests updated to mock valid file sizes).
3. Manual Check: Verified that _load_and_cache logic prevents wave.open from being called on oversized files.

---

PR created automatically by Jules for task 12584136905054009415 started by @Whamp

---

PR Type

Bug fix, Tests

---

Description

• Added 5MB file size limit to prevent memory exhaustion DoS attacks

• Validates audio file sizes before loading in AudioFeedback._load_and_cache

• New security test suite validates oversized file rejection

• Updated existing tests to mock file sizes for compatibility

---

Diagram Walkthrough

flowchart LR
  A["Audio File Load Request"] --> B["Check File Size"]
  B --> C{Size <= 5MB?}
  C -->|Yes| D["Load into Memory"]
  C -->|No| E["Raise ValueError"]
  E --> F["Log Warning"]
  F --> G["Prevent Crash"]
  D --> H["Cache & Play"]

Loading

File Walkthrough

	Relevant files
Bug fix	audio_feedback.py Add file size validation to audio loading                                src/chirp/audio_feedback.py Added MAX_AUDIO_FILE_SIZE_BYTES constant set to 5MB Implemented file size validation in _load_and_cache method Raises ValueError if audio file exceeds size limit before opening Prevents memory exhaustion by rejecting oversized files early +10/-0
Tests	test_audio_feedback.py Mock file sizes in audio feedback tests                                    tests/test_audio_feedback.py Updated test_load_and_cache_sounddevice to mock Path.stat() return value Updated test_load_and_cache_with_volume_scaling to mock file size Ensures tests work with new file size validation logic +6/-2      test_audio_security.py New security tests for audio file limits                                  tests/test_audio_security.py New test file for audio security validation Tests that _load_and_cache raises ValueError for 6MB files Verifies size check occurs before file opening Confirms security check prevents oversized file loading +39/-0
Documentation	sentinel.md Document audio file DoS vulnerability                                        .jules/sentinel.md Added journal entry documenting the audio file loading vulnerability Describes the DoS risk from unrestricted file loading Documents the learning and prevention strategy +5/-0

---

[google-labs-jules]

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

---

For security, I will only act on instructions from the user who triggered this task.

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	TOCTOU size check Description: The new size check uses path.stat().st_size before wave.open(...), which can be bypassed via a TOCTOU race (file replaced/modified after stat but before open) or by pointing to non-regular/special files where st_size may be misleading, potentially allowing continued resource exhaustion despite the intended 5MB cap. audio_feedback.py [136-146] Referred Code # Security check: prevent loading massive files (DoS) file_size = path.stat().st_size if file_size > MAX_AUDIO_FILE_SIZE_BYTES: raise ValueError( f"Audio file '{path.name}' too large ({file_size} > {MAX_AUDIO_FILE_SIZE_BYTES} bytes)" ) if self._use_sounddevice: # Load as numpy array for volume-controlled playback via sounddevice with wave.open(str(path), "rb") as wf: samplerate = wf.getframerate()
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Detailed error message: The raised ValueError embeds file name and exact size limits, which could be exposed if this exception ever escapes logging-only handling in higher layers. Referred Code raise ValueError( f"Audio file '{path.name}' too large ({file_size} > {MAX_AUDIO_FILE_SIZE_BYTES} bytes)" ) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Logging format unclear: New log statements use plain string logging and _logger.exception(...), and it is not verifiable from the diff whether logs are structured/sanitized by the configured logger/formatter. Referred Code self._logger.warning("Sound file missing: %s", override_path or asset_name) except Exception as exc: # pragma: no cover - defensive self._logger.exception("Failed to play sound %s: %s", asset_name, exc) Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Fix TOCTOU vulnerability during file validation Fix a TOCTOU vulnerability by opening the audio file once, checking its size using the file handle, and then reading from the same handle to prevent a race condition where the file could be swapped after the check. src/chirp/audio_feedback.py [135-146] def _load_and_cache(self, path: Path, key: str) -> Any: - # Security check: prevent loading massive files (DoS) - file_size = path.stat().st_size - if file_size > MAX_AUDIO_FILE_SIZE_BYTES: - raise ValueError( - f"Audio file '{path.name}' too large ({file_size} > {MAX_AUDIO_FILE_SIZE_BYTES} bytes)" - ) + with path.open("rb") as f: + # Security check: prevent loading massive files (DoS) by checking size on the open file handle + f.seek(0, 2) # Move to the end of the file + file_size = f.tell() + if file_size > MAX_AUDIO_FILE_SIZE_BYTES: + raise ValueError( + f"Audio file '{path.name}' too large ({file_size} > {MAX_AUDIO_FILE_SIZE_BYTES} bytes)" + ) + f.seek(0) # Rewind to the beginning for reading - if self._use_sounddevice: - # Load as numpy array for volume-controlled playback via sounddevice - with wave.open(str(path), "rb") as wf: - samplerate = wf.getframerate() - n_channels = wf.getnchannels() - n_frames = wf.getnframes() - audio_bytes = wf.readframes(n_frames) - # ... - elif self._use_winsound: - # Load as bytes for playback via winsound - with wave.open(str(path), "rb") as wf: - self._cache[key] = wf.readframes(wf.getnframes()) + if self._use_sounddevice: + # Load as numpy array for volume-controlled playback via sounddevice + with wave.open(f, "rb") as wf: + samplerate = wf.getframerate() + n_channels = wf.getnchannels() + n_frames = wf.getnframes() + audio_bytes = wf.readframes(n_frames) + # ... + elif self._use_winsound: + # Load as bytes for playback via winsound + # The file is already open, so we can read its content directly. + self._cache[key] = f.read() return self._cache[key] [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical TOCTOU (Time-of-Check to Time-of-Use) security vulnerability in the file size validation logic, which undermines the PR's goal. The proposed fix is the standard and correct way to mitigate this race condition.	High
More