(tooling-agents) branch main updated: ASVS audit: 5.3.2.md (redacted) [source: apache/steve/v3 @ d0aa7e9]

[as...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

asf-tooling pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tooling-agents.git


The following commit(s) were added to refs/heads/main by this push:
     new f31f289  ASVS audit: 5.3.2.md (redacted) [source: apache/steve/v3 @ d0aa7e9]
f31f289 is described below

commit f31f289d18d70798dcbaf0dd531aaeb1ca4962d4
Author: ASF Tooling Team <ro...@apache.org>
AuthorDate: Tue May 5 22:56:49 2026 +0200

    ASVS audit: 5.3.2.md (redacted) [source: apache/steve/v3 @ d0aa7e9]
---
 ASVS/reports/steve/v3/d0aa7e9/all/5.3.2.md | 189 +++++++++++++++++++++++++++++
 1 file changed, 189 insertions(+)

diff --git a/ASVS/reports/steve/v3/d0aa7e9/all/5.3.2.md b/ASVS/reports/steve/v3/d0aa7e9/all/5.3.2.md
new file mode 100644
index 0000000..6959040
--- /dev/null
+++ b/ASVS/reports/steve/v3/d0aa7e9/all/5.3.2.md
@@ -0,0 +1,189 @@
+# Security Audit Report: ASVS 5.3.2
+
+## Executive Summary
+
+| Field | Value |
+|-------|-------|
+| **Repository** | apache/steve/v3 |
+| **Audit Date** | May 05, 2026 |
+| **Auditor** | Tooling Agents (bundled-pass mode) |
+| **ASVS Requirement** | ASVS 5.3.2 |
+| **Files Analyzed** | 6 relevant / 49 total |
+| **Files Skipped** | 12 |
+
+**Requirement description:** Verify that when the application creates file paths for file operations, instead of user-submitted filenames, it uses internally generated or trusted data, or if user-submitted filenames or file metadata must be used, strict validation and sanitization must be applied. This is to protect against…
+
+### Findings Overview
+
+| Severity | Count |
+|----------|-------|
+| 🔴 Critical | 0 |
+| 🟠 High | 0 |
+| 🟡 Medium | 1 |
+| 🟢 Low | 1 |
+
+---
+
+## ASVS-5.3.2: Path Traversal Protection
+
+### Findings for 5.3.2
+
+### [MEDIUM]
+
+**Finding ID:** ASVS-532-MED-001  
+**Location:** `v3/server/pages.py`, `serve_doc()`, lines 560–574  
+
+**Description:** The `serve_doc()` function uses the user-supplied `docname` URL parameter directly in `send_from_directory()` without application-level validation. The developer explicitly acknowledged this gap with the comment `### verify the propriety of DOCNAME.` (Type B gap — developer knows validation is needed but has not implemented it). While Quart's `send_from_directory()` internally uses `safe_join()` to prevent path traversal, there is no defense-in-depth validation at the ap [...]
+
+```python
+@APP.get('/docs/<iid>/<docname>')
+@asfquart.auth.require
+async def serve_doc(iid, docname):
+    result = await basic_info()
+    db = steve.election.Election.open_database(DB_FNAME)
+    row = db.q_get_mayvote.first_row(result.uid, iid)
+    if not row:
+        quart.abort(404)
+    ### verify the propriety of DOCNAME.
+    return await quart.send_from_directory(DOCSDIR / iid, docname)
+```
+
+**Data flow:** URL parameter `docname` → no validation → `send_from_directory(DOCSDIR / iid, docname)` → file system access
+
+**Impact:** While Quart's `safe_join` provides framework-level protection, relying solely on framework internals without application-level validation creates risk if:
+1. The framework is upgraded and `safe_join` behavior changes
+2. A bypass is discovered in `safe_join`
+3. The code is refactored to use a different file-serving mechanism
+
+The explicit TODO comment indicates the developer considers this incomplete. Standard path parameters in Quart won't match `/` characters, limiting the immediate traversal risk to `..` sequences without slashes.
+
+**Proof of Concept:**
+```
+GET /docs/abc1234567/..%2F..%2Fetc%2Fpasswd HTTP/1.1
+```
+Note: Quart's default string converter won't match paths with `/` (even URL-encoded), so this specific PoC would not route. However, `GET /docs/abc1234567/..` could resolve to the parent directory depending on `safe_join` behavior.
+
+**Remediation:**
+```python
+import re
+
+SAFE_DOCNAME_RE = re.compile(r'^[a-zA-Z0-9][a-zA-Z0-9._-]*$')
+
+@APP.get('/docs/<iid>/<docname>')
+@asfquart.auth.require
+async def serve_doc(iid, docname):
+    result = await basic_info()
+    
+    # Validate docname: only safe characters, no traversal
+    if not SAFE_DOCNAME_RE.match(docname) or '..' in docname:
+        quart.abort(400)
+    
+    db = steve.election.Election.open_database(DB_FNAME)
+    row = db.q_get_mayvote.first_row(result.uid, iid)
+    if not row:
+        quart.abort(404)
+    
+    return await quart.send_from_directory(DOCSDIR / iid, docname)
+```
+
+### [LOW]
+
+**Finding ID:** ASVS-532-LOW-001  
+**Location:** `v3/server/pages.py`, `rewrite_description()`, lines 49–56  
+
+**Description:** The `rewrite_description()` function extracts filenames from issue descriptions using a regex pattern `doc:([^\s]+)` and constructs URL paths without sanitization. While filenames originate from authorized users (committers storing issue descriptions), a malicious committer could craft a description containing path traversal sequences in the `doc:` pattern.
+
+```python
+def rewrite_description(issue):
+    import re
+    desc = issue.description
+    def repl(match):
+        filename = match.group(1)
+        return f'<a href="/docs/{issue.iid}/{filename}">{filename}</a>'
+    desc = re.sub(r'doc:([^\s]+)', repl, desc)
+    issue.description = f'<pre>{desc}</pre>'
+```
+
+**Data flow:** Database (issue.description) → regex extraction → unvalidated `filename` → HTML href attribute
+
+**Impact:** A committer could store an issue description containing `doc:../../sensitive-file` which generates a link to `/docs/<iid>/../../sensitive-file`. When a voter clicks this link, it would be handled by `serve_doc()` where framework-level safe_join would prevent traversal, but the link itself could confuse users or be used in social engineering.
+
+**Remediation:**
+```python
+def rewrite_description(issue):
+    import re
+    SAFE_DOCNAME_RE = re.compile(r'^[a-zA-Z0-9][a-zA-Z0-9._-]*$')
+    desc = issue.description
+    def repl(match):
+        filename = match.group(1)
+        if not SAFE_DOCNAME_RE.match(filename):
+            return f'[invalid doc reference]'
+        from markupsafe import escape
+        safe_filename = escape(filename)
+        return f'<a href="/docs/{issue.iid}/{safe_filename}">{safe_filename}</a>'
+    desc = re.sub(r'doc:([^\s]+)', repl, desc)
+    issue.description = f'<pre>{desc}</pre>'
+```
+
+### Security Controls Inventory for 5.3.2
+
+| Control | Location | Status | Coverage |
+|---------|----------|--------|----------|
+| `send_from_directory()` with `safe_join` | `pages.py:574` | APPLIED | Protects `serve_doc()` at framework level |
+| `send_from_directory()` with `safe_join` | `pages.py:578` | APPLIED | Protects `serve_static()` at framework level |
+| Database validation of `iid` | `pages.py:567` | APPLIED | Ensures `iid` exists in mayvote before path construction |
+| Quart string converter (no `/` in params) | Framework routing | APPLIED | Limits characters in `<iid>` and `<docname>` |
+| Application-level docname validation | `pages.py` | **MISSING** (Type B) | Developer acknowledged gap with TODO comment |
+| Schema CHECK constraint on IID | `schema.sql:89-91` | APPLIED | IID must be exactly 10 hex chars |
+| EID schema CHECK constraint | `schema.sql:22-24` | APPLIED | EID must be exactly 10 hex chars |
+
+### Positive Patterns for 5.3.2
+
+- **Database-validated path components:** The `iid` parameter is checked against `q_get_mayvote` before being used in path construction, ensuring only valid 10-hex-char IIDs reach the file system.
+- **Framework-level safe_join:** `send_from_directory()` provides built-in path traversal protection.
+- **Routing constraints:** Standard Quart string converters prevent `/` in path parameters for `<iid>` and `<docname>`.
+- **Internally generated IDs:** Election IDs and Issue IDs are generated by `crypto.create_id()` (random hex), not derived from user input.
+- **Schema-enforced format:** SQLite CHECK constraints ensure EID/IID are exactly 10 hex characters.
+
+---
+
+
+
+## Cross-cutting Architecture Observations
+
+1. **Defense-in-Depth Gap in File Serving:** The `serve_doc()` endpoint is the primary file-serving entry point with user-controlled parameters. It relies entirely on Quart's `send_from_directory()` for security (path traversal, content type, headers) without any application-level validation. The developer explicitly acknowledged this gap with the `### verify the propriety of DOCNAME.` comment.
+
+2. **No File Upload in Scope:** The codebase does not contain any file upload handling. The docs directory appears to be populated through external means (potentially admin scripts or manual placement). This makes several ASVS file upload requirements (5.2.6, 5.3.3) not directly applicable but creates a gap in documentation of how files enter the system.
+
+3. **Strong Internal ID Generation:** The application generates random 10-hex-character IDs for elections and issues (`crypto.create_id()`), enforced by SQLite CHECK constraints. This significantly reduces path traversal risk since IDs used in file paths are always internally-generated, non-guessable values.
+
+4. **Authorization Model Partially Implemented:** While authentication is enforced via `@asfquart.auth.require`, fine-grained authorization (checking election ownership before management operations) is consistently marked with `### check authz` TODOs. This means any committer could potentially access any election's documents via `serve_doc()` if they can guess/discover a valid IID.
+
+5. **Framework Reliance Pattern:** The application consistently relies on framework-provided security controls (safe_join, routing converters, send_from_directory) without explicit application-level validation. While these framework controls are generally robust, this pattern creates single-point-of-failure risk.
+
+## Cross-cutting Recommendations
+
+### Immediate (Critical/High Priority)
+
+1. **Implement `docname` validation in `serve_doc()`:** Add an explicit allowlist regex (e.g., `^[a-zA-Z0-9][a-zA-Z0-9._-]{1,100}$`) and reject requests with invalid filenames. This addresses the developer's own TODO and provides defense-in-depth.
+
+2. **Add Content-Disposition header to document responses:** Use `as_attachment=True` in `send_from_directory()` or explicitly set `Content-Disposition` with properly-encoded filenames per RFC 6266. Add `X-Content-Type-Options: nosniff` header.
+
+3. **HTML-encode filenames in `rewrite_description()`:** Use `markupsafe.escape()` for display text and `urllib.parse.quote()` for href attributes to prevent potential injection.
+
+### Short-term (Medium Priority)
+
+4. **Implement file extension allowlist for documents:** Only serve files with known-safe extensions (`.pdf`, `.txt`, `.md`, etc.) from the docs directory to prevent serving executable or HTML content.
+
+5. **Add Content-Security-Policy headers:** For document responses, set restrictive CSP to prevent execution of any active content in served documents.
+
+6. **Document the file provisioning process:** Since no upload handler exists in this code, document how files enter the `DOCSDIR` to ensure that process has its own security controls.
+
+### Long-term (Architecture)
+
+7. **Implement the authorization checks:** Complete the `### check authz` TODOs throughout the codebase to ensure proper ownership verification before serving documents and managing elections.
+
+8. **Consider content scanning:** If the docs directory will accept user-contributed files in the future, implement content scanning (antivirus, magic-byte validation) before files are made available.
+
+9. **Add image upload controls proactively:** If image upload is planned, implement pixel dimension limits, file size limits, and format validation (magic bytes + extension) before the feature is deployed.
+        
\ No newline at end of file


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@tooling.apache.org
For additional commands, e-mail: commits-help@tooling.apache.org