Secure code review

Secure Code Review

Overview

Review code through the eyes of an attacker. For each finding, name the vulnerability class, show how it's exploited, rate severity, and give a concrete fix. Scope to the diff/files provided; pull in just enough surrounding code (callers, trust boundaries) to judge exploitability.

Keywords: security review, secure code review, vulnerability, OWASP, injection, SQLi, XSS, command injection, authentication, authorization, IDOR, broken access control, crypto, secrets, hardcoded credentials, deserialization, SSRF, path traversal, input validation, CSRF.

Workflow

1. Map trust boundaries. Identify where untrusted data enters: HTTP params/body/headers, file uploads, env, message queues, DB rows from other tenants, third-party APIs. Untrusted input is where most bugs live.
2. Trace tainted data to sinks. Follow each untrusted input to dangerous sinks (SQL, shell, file paths, HTML output, deserializers, redirects, template engines). A flow from source → sink without sanitization is a finding.
3. Review each vulnerability class using references/vuln-patterns.md: injection, broken access control/authz, auth & session, crypto, secrets, deserialization, SSRF, path traversal, CSRF, security misconfig, sensitive-data exposure.
4. Check authorization on every sensitive operation. Authentication ≠ authorization. Confirm object-level checks (does this user own this record?) to catch IDOR / broken access control.
5. Scan for low-hanging fruit with scripts/secret_scan.py (hardcoded secrets, dangerous function calls). Triage hits — it's a starting point, not proof.
6. Rate severity (Critical/High/Medium/Low) by exploitability × impact, using references/checklist.md.
7. Write findings with: class, file:line, exploit scenario, severity, and a concrete fix (prefer a diff). End with a verdict and a prioritized fix list.

Decision Heuristics

• Assume all external input is hostile until validated/escaped at the sink.
• Escape at the sink, validate at the boundary — parameterized queries beat input filtering.
• Deny by default for access control; explicit allow per resource and action.
• Never trust client-side checks — they're UX, not security.
• Secrets in code = Critical, even in tests or comments, because of git history.
• If uncertain it's exploitable, mark "Possible:" and state what would confirm it.

Worked Example

# src/api/users.py:88  — VULNERABLE
query = f"SELECT * FROM users WHERE email = '{email}'"
db.execute(query)

Finding — [Critical] SQL Injection — src/api/users.py:88 An attacker submitting email=' OR '1'='1 reads all rows; '; DROP TABLE users;-- can destroy data. Fix: use a parameterized query:

db.execute("SELECT * FROM users WHERE email = %s", (email,))

Best Practices

• Prefer parameterized queries / prepared statements over string building everywhere.
• Context-aware output encoding for XSS (HTML, attribute, JS, URL contexts differ).
• Use vetted libraries for crypto and auth — never hand-roll. Use hmac.compare_digest for secret comparison.
• Allow-list inputs (paths, redirect targets, file types) rather than block-listing.
• Keep secrets in a vault/KMS or env, never in source; rotate anything that leaked.
• Add a regression test for each fixed vuln.

Common Pitfalls

• Treating authentication as authorization — logged-in users still need per-object checks (IDOR).
• Block-list validation that attackers trivially bypass; prefer allow-lists and escaping at the sink.
• Custom crypto (homemade tokens, ECB mode, static IVs, MD5/SHA1 for passwords).
• eval/pickle/yaml.load/native deserialization on untrusted data → RCE.
• Verbose error messages / stack traces leaking internals to users.
• Reflected user input into HTML, headers, or redirects without encoding/allow-listing.
• Reviewing only the happy path — vulns hide in error handlers and edge cases.