You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tooling.apache.org by as...@apache.org on 2026/04/22 10:37:47 UTC
(tooling-agents) branch main updated: ASVS L3 audit: 3.5.3 (ch03_general)
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 89f419e ASVS L3 audit: 3.5.3 (ch03_general)
89f419e is described below
commit 89f419e57895171a9dbbab76ee8393b453879052
Author: ASF Tooling Team <ro...@apache.org>
AuthorDate: Wed Apr 22 12:37:43 2026 +0200
ASVS L3 audit: 3.5.3 (ch03_general)
---
.../reports/steve/v3/d0aa7e9/ch03_general/3.5.3.md | 401 +++++++++++++++++++++
1 file changed, 401 insertions(+)
diff --git a/repos/apache/ASVS/reports/steve/v3/d0aa7e9/ch03_general/3.5.3.md b/repos/apache/ASVS/reports/steve/v3/d0aa7e9/ch03_general/3.5.3.md
new file mode 100644
index 0000000..0ef8cb4
--- /dev/null
+++ b/repos/apache/ASVS/reports/steve/v3/d0aa7e9/ch03_general/3.5.3.md
@@ -0,0 +1,401 @@
+# ASVS 3.5.3 Security Audit Report
+
+## Executive Summary
+
+**Repository:** apache/steve
+**Audit Date:** Apr 22, 2026
+**Auditor:** Tooling Agents
+**ASVS Requirement:** 3.5.3 — Verify that HTTP requests to sensitive functionality use appropriate HTTP methods such as POST, PUT, PATCH, or DELETE, and not methods defined by the HTTP specification as "safe" such as HEAD, OPTIONS, or GET. Alternatively, strict validation of the Sec-Fetch-* request header fields can be used to ensure that the request did not originate from an inappropriate cross-origin call, a navigation request, or a resource load (such as an image source) where this is [...]
+
+**Files Analyzed:** 3 relevant out of 49 total, 12 skipped
+
+### Finding Summary
+
+| Severity | Count | Status |
+|----------|-------|--------|
+| Critical | 2 | 🔴 Requires immediate action |
+| High | 0 | ✅ None found |
+| Medium | 0 | ✅ None found |
+| Low | 0 | ✅ None found |
+
+**Key Finding:** Two critical state-changing election operations (open/close) use GET methods instead of POST/PUT/PATCH/DELETE. No Sec-Fetch-* header validation exists as an alternative control. This represents a Type B gap where the correct pattern exists (7 of 9 endpoints use POST correctly) but is inconsistently applied.
+
+---
+
+## Security Controls Inventory
+
+### HTTP Method Usage for State-Changing Endpoints
+
+| Endpoint | Method | Operation | Correct? |
+|---|---|---|---|
+| `/do-vote/<eid>` | `POST` | Submit vote | ✅ |
+| `/do-create-election` | `POST` | Create election | ✅ |
+| `/do-add-issue/<eid>` | `POST` | Add issue | ✅ |
+| `/do-edit-issue/<eid>/<iid>` | `POST` | Edit issue | ✅ |
+| `/do-delete-issue/<eid>/<iid>` | `POST` | Delete issue | ✅ |
+| `/do-set-open_at/<eid>` | `POST` | Set open date | ✅ |
+| `/do-set-close_at/<eid>` | `POST` | Set close date | ✅ |
+| **`/do-open/<eid>`** | **`GET`** | **Open election** | **❌ CRITICAL** |
+| **`/do-close/<eid>`** | **`GET`** | **Close election** | **❌ CRITICAL** |
+
+**Coverage:** 77.8% (7/9 endpoints compliant)
+
+### Sec-Fetch-* Header Validation
+
+| Control | Defined? | Applied? |
+|---|---|---|
+| `Sec-Fetch-Site` validation | ❌ No | N/A |
+| `Sec-Fetch-Mode` validation | ❌ No | N/A |
+| `Sec-Fetch-Dest` validation | ❌ No | N/A |
+| Middleware-level Sec-Fetch check | ❌ No | N/A |
+
+**Status:** Neither the primary control (correct HTTP methods) nor the alternative control (Sec-Fetch-* validation) is applied to the two affected endpoints.
+
+---
+
+## Critical File Review
+
+### `v3/server/pages.py` — Route Definitions
+
+| Line | Function | HTTP Method | State-Changing? | Compliant? |
+|------|----------|-------------|-----------------|------------|
+| ~136 | `home_page` | GET | No | ✅ N/A |
+| ~147 | `voter_page` | GET | No | ✅ N/A |
+| ~254 | `vote_on_page` | GET | No | ✅ N/A |
+| ~373 | `do_set_open_at_endpoint` | POST | Yes | ✅ |
+| ~379 | `do_set_close_at_endpoint` | POST | Yes | ✅ |
+| ~393 | `do_vote_endpoint` | POST | Yes | ✅ |
+| ~435 | `admin_page` | GET | No | ✅ N/A |
+| ~469 | `manage_page` | GET | No | ✅ N/A |
+| ~493 | `manage_stv_page` | GET | No | ✅ N/A |
+| ~517 | `do_create_endpoint` | POST | Yes | ✅ |
+| **~536** | **`do_open_endpoint`** | **GET** | **Yes** | **❌ CRITICAL** |
+| **~555** | **`do_close_endpoint`** | **GET** | **Yes** | **❌ CRITICAL** |
+| ~572 | `do_add_issue_endpoint` | POST | Yes | ✅ |
+| ~597 | `do_edit_issue_endpoint` | POST | Yes | ✅ |
+| ~617 | `do_delete_issue_endpoint` | POST | Yes | ✅ |
+
+### `v3/steve/election.py` — State Machine Transitions
+
+| Method | Transition | Irreversible? | Called via GET? |
+|--------|-----------|---------------|----------------|
+| `open(pdb)` | editable → open | Yes | **Yes** ❌ |
+| `close()` | open → closed | Yes | **Yes** ❌ |
+| `add_vote()` | N/A (data) | N/A | No ✅ |
+| `add_issue()` | N/A (data) | N/A | No ✅ |
+| `delete_issue()` | N/A (data) | N/A | No ✅ |
+
+---
+
+## Findings
+
+### Critical Severity
+
+#### ASVS-353-CRT-001: Election Open Operation Uses GET Method for State-Changing Functionality
+
+**Severity:** CRITICAL
+**Gap Type:** Type B — Control EXISTS (POST used on 7 of 9 state-changing endpoints) but NOT CALLED on this endpoint
+**File:** `v3/server/pages.py`
+**Function:** `do_open_endpoint`
+**Lines:** ~536–553
+
+**Related Functions Checked:**
+- `election.open(pdb)` in `v3/steve/election.py` (~line 73)
+- `election.add_salts()` in `v3/steve/election.py`
+- `election.gather_election_data(pdb)` in `v3/steve/election.py`
+- `crypto.gen_opened_key()` in crypto module
+
+##### Vulnerable Code
+
+```python
+@APP.get('/do-open/<eid>') # ← GET method for state-changing operation
+@asfquart.auth.require({R.committer})
+@load_election
+async def do_open_endpoint(election):
+ result = await basic_info()
+
+ ### check authz
+
+ pdb = steve.persondb.PersonDB.open(DB_FNAME)
+
+ # Open the Election.
+ election.open(pdb) # ← Irreversible state change via GET
+ ...
+ return quart.redirect(f'/manage/{election.eid}', code=303)
+```
+
+##### State Change Implementation
+
+From `v3/steve/election.py` (`Election.open()`, ~line 73):
+
+```python
+def open(self, pdb):
+ assert self.is_editable()
+ self.add_salts() # Generates crypto salts for all voters
+ edata = self.gather_election_data(pdb) # Assembles election data
+ salt = crypto.gen_salt() # Generates election-level salt
+ opened_key = crypto.gen_opened_key(edata, salt) # Derives tamper-detection key
+ self.c_open.perform(salt, opened_key, self.eid) # Writes to database
+```
+
+This operation:
+1. Generates cryptographic salts for every voter/issue pair
+2. Derives an anti-tamper key from the entire election definition
+3. Transitions the election from `editable` → `open` (irreversible)
+4. After this point, issues and voters **cannot** be added/removed
+
+##### Data Flow
+
+```
+Attacker-controlled URL → GET /do-open/<eid> → election.open(pdb) → Database state change
+ ↑ No method restriction ↑ No Sec-Fetch-* check
+```
+
+##### Proof of Concept
+
+An attacker who knows an election ID can forge a request that a logged-in committer will trigger:
+
+**Via image tag injection** (e.g., in any HTML context the victim renders):
+```html
+<img src="https://voting.example.org/do-open/abc123def456" style="display:none">
+```
+
+**Via link prefetch:**
+```html
+<link rel="prefetch" href="https://voting.example.org/do-open/abc123def456">
+```
+
+**Via simple link in email/chat:**
+```
+Click here for the election details: https://voting.example.org/do-open/abc123def456
+```
+
+When any authenticated committer's browser processes these, the GET request fires with their session cookie (assuming `SameSite=Lax` or `None`), opening the election prematurely.
+
+##### Impact
+
+- **Election integrity compromised**: An election can be opened before all issues/voters are configured, locking the election definition permanently
+- **Cryptographic binding to incomplete data**: The `opened_key` anti-tamper hash binds to whatever data exists at the time of opening — missing voters or issues become permanent
+- **No rollback**: The `open()` operation has no undo; once opened, the election cannot return to editable state
+- **Trivially exploitable**: Requires only knowing an election ID (which appears in URLs and logs) and getting a committer to load an attacker-controlled page
+- **Violates election integrity guarantees**: The cryptographic tamper-detection mechanism becomes meaningless if an attacker can force premature opening
+
+##### Remediation
+
+```python
+# Change from GET to POST
+@APP.post('/do-open/<eid>') # ← Changed from @APP.get
+@asfquart.auth.require({R.committer})
+@load_election
+async def do_open_endpoint(election):
+ result = await basic_info()
+ ### check authz — verify result.uid is the election owner
+ pdb = steve.persondb.PersonDB.open(DB_FNAME)
+ election.open(pdb)
+ _LOGGER.info(f'User[U:{result.uid}] opened election[E:{election.eid}]')
+ title = election.get_metadata().title
+ await flash_success(f'Opened election: {title}')
+ return quart.redirect(f'/manage/{election.eid}', code=303)
+```
+
+**Additional Required Changes:**
+- Update corresponding template/JavaScript to use a form POST or fetch with `method: 'POST'` instead of a link/anchor tag
+- Add CSRF token validation once CSRF protection is implemented (see TODO.md items #6/#8)
+
+---
+
+#### ASVS-353-CRT-002: Election Close Operation Uses GET Method for State-Changing Functionality
+
+**Severity:** CRITICAL
+**Gap Type:** Type B — Control EXISTS (POST used on 7 of 9 state-changing endpoints) but NOT CALLED on this endpoint
+**File:** `v3/server/pages.py`
+**Function:** `do_close_endpoint`
+**Lines:** ~555–571
+
+**Related Functions Checked:**
+- `election.close()` in `v3/steve/election.py` (~line 94)
+- `election.is_open()` in `v3/steve/election.py`
+
+##### Vulnerable Code
+
+```python
+@APP.get('/do-close/<eid>') # ← GET method for state-changing operation
+@asfquart.auth.require({R.committer})
+@load_election
+async def do_close_endpoint(election):
+ result = await basic_info()
+
+ ### check authz
+
+ # Close the Election.
+ election.close() # ← Irreversible state change via GET
+ ...
+ return quart.redirect(f'/manage/{election.eid}', code=303)
+```
+
+##### State Change Implementation
+
+From `v3/steve/election.py` (`Election.close()`, ~line 94):
+
+```python
+def close(self):
+ assert self.is_open()
+ self.c_close.perform(self.eid) # Sets closed=1 in database
+```
+
+This transitions the election from `open` → `closed`, permanently ending the voting period.
+
+##### Data Flow
+
+```
+Attacker-controlled URL → GET /do-close/<eid> → election.close() → Database state change
+ ↑ No method restriction ↑ No Sec-Fetch-* check
+```
+
+##### Proof of Concept
+
+```html
+<!-- Injected anywhere a logged-in committer will render -->
+<img src="https://voting.example.org/do-close/abc123def456" width="0" height="0">
+```
+
+Or via browser navigation:
+```
+https://voting.example.org/do-close/abc123def456
+```
+
+##### Impact
+
+- **Election disruption**: An active election can be closed prematurely, disenfranchising voters who haven't yet cast ballots
+- **No rollback**: Once closed, the election cannot be reopened — the state machine only moves forward
+- **Denial of service against democratic process**: A single forged GET request ends an entire election
+- **Any committer can trigger**: The authorization check is `{R.committer}` (not owner-only), meaning any authenticated committer's session can be weaponized
+- **Democratic legitimacy compromised**: Premature closure can invalidate election results and undermine trust in the voting system
+
+##### Remediation
+
+```python
+# Change from GET to POST
+@APP.post('/do-close/<eid>') # ← Changed from @APP.get
+@asfquart.auth.require({R.committer})
+@load_election
+async def do_close_endpoint(election):
+ result = await basic_info()
+ ### check authz — verify result.uid is the election owner
+ election.close()
+ _LOGGER.info(f'User[U:{result.uid}] closed election[E:{election.eid}]')
+ title = election.get_metadata().title
+ await flash_success(f'Closed election: {title}')
+ return quart.redirect(f'/manage/{election.eid}', code=303)
+```
+
+**Additional Required Changes:**
+- Update corresponding template/JavaScript to use a form POST or fetch with `method: 'POST'` instead of a link/anchor tag
+- Add CSRF token validation once CSRF protection is implemented (see TODO.md items #6/#8)
+
+---
+
+## Positive Security Patterns
+
+1. **Consistent POST usage for most mutations**: 7 of 9 state-changing endpoints correctly use `@APP.post()`, showing the development team understands the principle and has established the correct pattern throughout most of the codebase.
+
+2. **Authentication on all sensitive routes**: All state-changing endpoints have `@asfquart.auth.require()` decorators, ensuring only authenticated users with appropriate roles can access sensitive functionality.
+
+3. **POST-Redirect-GET pattern**: POST endpoints correctly redirect with `code=303`, preventing form resubmission issues and following HTTP best practices.
+
+4. **Election state machine assertions**: `election.py` uses state assertions (`is_editable()`, `is_open()`) before performing operations, providing defense-in-depth against operations in wrong state (though see Architecture Observations regarding `assert` usage).
+
+5. **Structured route naming**: The `do-` prefix convention for action endpoints makes it easy to audit which routes perform mutations and provides clear semantic separation between read and write operations.
+
+6. **Authorization decorators**: Consistent use of `@asfquart.auth.require()` with role-based access control (e.g., `{R.committer}`) demonstrates security-conscious design.
+
+---
+
+## Architecture Observations
+
+1. **No Sec-Fetch-* validation layer**: The application has no middleware or per-route validation of `Sec-Fetch-Site`, `Sec-Fetch-Mode`, or `Sec-Fetch-Dest` headers. Adding this as a middleware would provide defense-in-depth for all endpoints and serve as the alternative control specified in ASVS 3.5.3.
+
+2. **TODO.md acknowledges CSRF debt**: The placeholder CSRF token (`basic.csrf_token = 'placeholder'`) and the TODO.md item #6/#8 show awareness of CSRF protection needs, but the GET method issue on open/close is a separate, more fundamental problem that CSRF tokens alone wouldn't fully address (GET requests can be triggered without any form interaction).
+
+3. **`assert` used for security checks**: `election.py` uses Python `assert` for state validation (e.g., `assert self.is_editable()`, `assert self.is_open()`). These are disabled when Python runs with `-O` (optimized mode). State checks should use explicit `if`/`raise` patterns for security-critical validation to ensure they cannot be bypassed in production environments.
+
+4. **State machine design is sound**: The election lifecycle (editable → open → closed) is well-designed with clear transitions. The security issue is purely in the HTTP method choice for triggering these transitions, not in the state machine logic itself.
+
+5. **Inconsistent application of security pattern**: The fact that 7 of 9 endpoints use the correct method suggests this is a development oversight rather than a systemic misunderstanding, making remediation straightforward.
+
+---
+
+## Recommendations Summary
+
+### Immediate Actions (Within 24-48 hours)
+
+| Priority | Action | Effort | Impact | Owner |
+|----------|--------|--------|--------|-------|
+| **1** | Change `/do-open/<eid>` from `@APP.get` to `@APP.post` | Low | Eliminates trivial CSRF via image/link/prefetch for election opening | Backend Team |
+| **2** | Change `/do-close/<eid>` from `@APP.get` to `@APP.post` | Low | Eliminates trivial CSRF via image/link/prefetch for election closing | Backend Team |
+| **3** | Update UI templates to use POST forms instead of links for open/close actions | Low | Required companion change for #1 and #2 | Frontend Team |
+
+### Short-term Actions (Within 1-2 weeks)
+
+| Priority | Action | Effort | Impact | Owner |
+|----------|--------|--------|--------|-------|
+| **4** | Add `Sec-Fetch-Site` validation middleware rejecting cross-origin requests to all `/do-*` endpoints | Medium | Defense-in-depth against cross-origin request forgery for all state-changing endpoints | Backend Team |
+| **5** | Implement real CSRF tokens (replace `'placeholder'`) with per-session random tokens validated on all POST endpoints | Medium | Completes CSRF protection for authenticated-only system | Backend Team |
+| **6** | Add comprehensive logging for all election state transitions with user ID, timestamp, and IP address | Low | Improves audit trail and incident response capabilities | Backend Team |
+
+### Long-term Actions (Within 1-3 months)
+
+| Priority | Action | Effort | Impact | Owner |
+|----------|--------|--------|--------|-------|
+| **7** | Replace `assert` with explicit `if`/`raise` for all security-relevant state checks in `election.py` | Low | Prevents security bypass when Python runs in optimized mode | Backend Team |
+| **8** | Implement comprehensive HTTP method validation test suite | Medium | Prevents regression and ensures all future endpoints use correct methods | QA Team |
+| **9** | Add `Sec-Fetch-Mode` and `Sec-Fetch-Dest` validation for additional defense layers | Medium | Further hardens against sophisticated cross-origin attacks | Backend Team |
+| **10** | Conduct security review of authorization logic to ensure election owners (not just any committer) can perform state transitions | Medium | Implements principle of least privilege for election management | Security Team |
+
+---
+
+## Appendix: Files Analyzed
+
+<details>
+<summary>Click to expand file list (3 files)</summary>
+
+### Files Reviewed
+
+1. **`v3/server/pages.py`** (Relevance: 10/10)
+ - Primary route definitions file
+ - Contains all 9 state-changing endpoints
+ - 2 critical findings identified
+
+2. **`v3/TODO.md`** (Relevance: 7/10)
+ - Documents known CSRF protection gaps
+ - Confirms awareness of security debt
+ - Placeholder CSRF token implementation noted
+
+3. **`v3/steve/election.py`** (Relevance: 4/10)
+ - Election state machine implementation
+ - `open()` and `close()` methods reviewed
+ - State validation logic analyzed
+
+### Files Skipped (12 files)
+
+Files skipped due to low relevance to ASVS 3.5.3 (HTTP method appropriateness):
+- Configuration files
+- Database schema definitions
+- Cryptographic utility modules (not related to HTTP layer)
+- Template files (reviewed separately for UI changes)
+- Test files (no tests for HTTP method validation found)
+- Documentation files
+- Deployment scripts
+- Static assets
+- Third-party dependencies
+- Migration scripts
+- Logging configuration
+- Authentication modules (covered by decorators in pages.py)
+
+</details>
+
+---
+
+**End of Report**
+
+*This audit focused specifically on ASVS Requirement 3.5.3 compliance. Additional security requirements from ASVS Level 1 should be evaluated separately.*
\ 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