mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-21 07:01:15 +00:00
[AZ-487] [AZ-488] security: cycle 2 delta audit (PASS_WITH_WARNINGS)
Step 14 (Security Audit) for cycle 2 — delta scan against the cycle-1 baseline. Verdict remains PASS_WITH_WARNINGS; no Critical/High. Scope: JWT auth boundary (AZ-487) and UAV multipart upload + ImageSharp decode of attacker-controlled bytes (AZ-488). Both new packages (JwtBearer 8.0.21, ImageSharp 3.1.11 in Services.TileDownloader) checked. Cycle-2 delta: * 0 Critical / 0 High * 2 Medium: F-AUTH-2 (iss/aud not validated — by design until admin team publishes values, AZ-487 § Constraints), F-UAV-1 (ImageSharp decode now runs on attacker-controlled bytes — mitigations sufficient; pin to GHSA subscribe-and-bump policy). * 4 Low: F-AUTH-1 (DEV-ONLY secret in appsettings.Development.json — accepted), F-AUTH-3 (rate-limit gap extends to 401 floods — folds into cycle-1 I3), F-UAV-2 (JsonDocument.Parse on signature-validated claims — bounded by Kestrel header cap), D3 (JwtBearer shares D1 patch line). * 1 Informational: F-UAV-3 (reject reasons disclose gate structure — accepted UX trade-off; documented in contract). OWASP refresh: A01 / A07 move from N/A (with caveat) to PASS_WITH_WARNINGS (per-tenant authz absent; iss/aud + revocation gaps tracked). Pre-deploy operational gate added: deploy pipeline must verify JWT_SECRET != DEV-ONLY placeholder before promoting api. Artifacts: dependency_scan.md, static_analysis.md, owasp_review.md, infrastructure_review.md, security_report.md — all appended with a "Cycle 2 Delta" section preserving cycle-1 finding IDs. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -50,6 +50,44 @@
|
||||
## Items checked clean
|
||||
|
||||
- SixLabors.ImageSharp 3.1.11 — newer than the patched 3.1.7 / 3.1.5 lines (CVE-2024-41131, CVE-2025-27598). No outstanding GHSA against 3.1.11 itself.
|
||||
|
||||
---
|
||||
|
||||
## Cycle 2 Delta (2026-05-11 — AZ-487 / AZ-488)
|
||||
|
||||
### New packages added this cycle
|
||||
|
||||
| Project | Package | Version | Notes |
|
||||
|---------|---------|---------|-------|
|
||||
| Api | `Microsoft.AspNetCore.Authentication.JwtBearer` | 8.0.21 | Added by AZ-487 (JWT validation baseline). Same patch line as `Microsoft.AspNetCore.OpenApi 8.0.21` — see D1. |
|
||||
| Services.TileDownloader | `SixLabors.ImageSharp` | 3.1.11 | Added by AZ-488 to identify + decode UAV-supplied JPEGs (`Image.Identify`, `Image.Load<L8>`). Same version as the existing Api-level reference — consistent. |
|
||||
| Tests (unit + integration) | `SixLabors.ImageSharp` | 3.1.11 | Added by AZ-488 to generate test fixtures. |
|
||||
|
||||
### New findings
|
||||
|
||||
#### D3 — `Microsoft.AspNetCore.Authentication.JwtBearer 8.0.21` shares the same 8.0.21 patch line as the D1-flagged OpenApi package (Low — production-risk: **Low**, exposure: not reachable)
|
||||
|
||||
- **Location**: `SatelliteProvider.Api/SatelliteProvider.Api.csproj` (added by AZ-487)
|
||||
- **Detail**: D1 already recommends bumping `Microsoft.AspNetCore.OpenApi` to 8.0.25 because the underlying ASP.NET Core 8.0.21 runtime ships CVE-2026-26130 (SignalR DoS, not reachable in this app). Pinning a second package in the same 8.0.21 family in cycle 2 raises the cost of *not* doing the bump: every additional package implicitly hardcodes the runtime expectation. Cycle 1 disposition (`Not exploitable — no SignalR use`) still applies; cycle 2 escalation here is purely about consistency and operator clarity.
|
||||
- **Disposition**: Same as D1 — bump *both* `Microsoft.AspNetCore.OpenApi` AND the new `Microsoft.AspNetCore.Authentication.JwtBearer` reference to the latest 8.0.x patch in a single PR. No separate Jira needed; fold into the D1 hardening task.
|
||||
|
||||
#### F-DEPS-UAV — ImageSharp decode now runs on attacker-controlled JPEG bytes (Medium — exposure increase, not a new CVE)
|
||||
|
||||
- **Location**: `SatelliteProvider.Services.TileDownloader/UavTileQualityGate.cs:60-78` — `Image.Identify(...)` and `Image.Load<L8>(...)` on `ReadOnlyMemory<byte>` originating from `POST /api/satellite/upload`.
|
||||
- **Detail**: Pre-cycle-2 the only ImageSharp call site was `GoogleMapsDownloaderV2`, which decodes responses from a known-good origin (Google's tile CDN under our API key). AZ-488 introduces a second call site that decodes **arbitrary client-supplied bytes**. ImageSharp 3.1.11 is patched against the published CVE line (CVE-2024-41131 / CVE-2025-27598 — both pre-3.1.7), so no known-CVE finding exists today. The change is in *exposure*: any future ImageSharp decode bug becomes a remote-attacker primitive on the upload endpoint.
|
||||
- **Mitigation already in place** (AZ-488):
|
||||
- Rule 1 (magic-byte check) runs *before* ImageSharp touches the bytes, narrowing the input to `FF D8 FF`-prefixed payloads.
|
||||
- Rule 2 caps per-item bytes at 5 MiB (configurable); Kestrel + FormOptions cap the envelope at `MaxBatchSize × MaxBytes = 500 MiB`.
|
||||
- Decode is wrapped in `try { … } catch (UnknownImageFormatException) { … } catch (InvalidImageContentException) { … }` so a malformed JPEG produces a structured `INVALID_FORMAT` reject, not an unhandled exception.
|
||||
- **Disposition**: Accept for cycle 2 — the mitigations align with current best practice for "decode untrusted images" surfaces. Track as recurring follow-up:
|
||||
- Subscribe to GHSA advisories for `SixLabors.ImageSharp` and bump aggressively (within 7 days of a patch).
|
||||
- Re-evaluate whether to sandbox the decode (e.g. dedicated process pool, libvips with seccomp) the next time the upload trust boundary changes.
|
||||
|
||||
### Cross-version sanity (post-cycle-2)
|
||||
|
||||
- `SixLabors.ImageSharp` is **3.1.11** in Api, Common, Services.TileDownloader, Tests, IntegrationTests — consistent across 5 csprojs. ✓
|
||||
- `Microsoft.AspNetCore.*` 8.0.21 in OpenApi + the new JwtBearer — consistent within the family but lagging one patch (8.0.25 is current). Cycle-2 D3 + cycle-1 D1 share remediation.
|
||||
- No new Newtonsoft.Json / Npgsql / Dapper changes this cycle.
|
||||
- Newtonsoft.Json 13.0.4 — past CVE-2024-21907 fix line (13.0.1).
|
||||
- Npgsql 9.0.2 — outside the 4.x / 5.x / 6.x / 7.x / 8.x ranges affected by CVE-2024-32655 (SQL injection via protocol message size overflow). 9.0.x line was never affected.
|
||||
- Dapper 2.1.35 — only "advisory" hit was a dependency-check false positive for SQLite CVE-2017-10989; not a Dapper issue.
|
||||
|
||||
@@ -85,3 +85,28 @@
|
||||
- [x] All Dockerfiles reviewed (Api + IntegrationTests)
|
||||
- [x] All CI/CD configs reviewed (`.woodpecker/01-test.yml`, `02-build-push.yml`)
|
||||
- [x] All env / config files reviewed (`appsettings*.json`, `.env`, `docker-compose*.yml`)
|
||||
|
||||
---
|
||||
|
||||
## Cycle 2 Delta (AZ-487 + AZ-488)
|
||||
|
||||
### Infra changes this cycle
|
||||
|
||||
- `docker-compose.yml:32` adds `JWT_SECRET=${JWT_SECRET}` to the `api` service `environment` block (AZ-487). Sourced from the host env (or `.env`).
|
||||
- `docker-compose.tests.yml:21` adds the same `JWT_SECRET=${JWT_SECRET}` to the integration-test runner so tests can mint matching tokens.
|
||||
- `.env.example:18` adds an empty `JWT_SECRET=` line as a deploy-time reminder.
|
||||
- `.env` (workspace root, not tracked) ships a `DEV-ONLY-CHANGE-ME-…` placeholder ≥ 32 bytes for local docker-compose use.
|
||||
- No new infrastructure for AZ-488 — the upload endpoint reuses the existing `./tiles/` volume; UAV files land under `./tiles/uav/{z}/{x}/{y}.jpg` inside the same mount.
|
||||
|
||||
### Cycle-2 verdict — clean
|
||||
|
||||
- **Secret distribution**: `JWT_SECRET` flows env-var → docker-compose `environment` → Kestrel `IConfiguration` → `AddSatelliteJwt`. No checked-in production secret. The DEV-ONLY value in `.env` is also explicitly labelled and is bound to the cycle-1 S4 follow-up (rotate-and-document workflow). ✓
|
||||
- **`.env` in `.dockerignore` (cycle-1 I5)**: still tracked under that remediation; cycle 2 does not add a new `.env` exposure path. The `JWT_SECRET` mirroring lives in compose, not the Dockerfile, so it doesn't bake into image layers. ✓
|
||||
- **No new exposed ports**: cycle 2 changes are HTTP-layer only — endpoint registration, middleware, and a multipart handler. No new listener.
|
||||
- **No new external services**: ImageSharp decode is in-process; no new outbound network call introduced by AZ-487 / AZ-488.
|
||||
- **No CI workflow changes**: existing `.woodpecker/01-test.yml` continues to run unit + integration tests; new cycle-2 unit + integration tests run inside the existing workflow.
|
||||
|
||||
### Cycle-2 operational follow-ups (NOT findings — pre-deploy verification)
|
||||
|
||||
1. The deploy pipeline must verify `JWT_SECRET` is set to a ≥ 32-byte value distinct from the DEV-ONLY placeholder before promoting `api`. The application throws at startup if the value is missing or short, so a misconfigured deploy fails fast — but a deploy that *promotes* the dev placeholder verbatim would still pass the 32-byte gate. Tracked in `security_report.md` cycle-2 recommendations.
|
||||
2. Coordinate with admin team on `iss`/`aud` values (F-AUTH-2). When values are defined, both the `AddSatelliteJwt` call site and `.env`/compose docs must be updated together.
|
||||
|
||||
@@ -38,3 +38,35 @@
|
||||
- [x] Each FAIL has at least one specific finding with evidence
|
||||
- [x] N/A categories have justification + caveat
|
||||
- [x] No `security_approach.md` exists in `_docs/00_problem/` to cross-reference (project has not declared explicit security requirements; this audit treats the architecture-vision statement "internal/trusted network service" as the de-facto requirement)
|
||||
|
||||
---
|
||||
|
||||
## Cycle 2 Refresh (AZ-487 + AZ-488)
|
||||
|
||||
Cycle 1's A01 / A07 verdicts were `N/A (with caveat)` because the service shipped without authentication. AZ-487 (JWT validation baseline) and AZ-488 (UAV upload permission policy) materially change those verdicts. The table below supersedes the cycle-1 row for A01 and A07; all other rows remain as cycle 1 left them, with cycle-2 findings appended where applicable.
|
||||
|
||||
| # | Category | Cycle 1 Status | Cycle 2 Status | Cycle-2 evidence |
|
||||
|---|----------|----------------|----------------|------------------|
|
||||
| A01 | Broken Access Control | N/A (with caveat) | **PASS_WITH_WARNINGS** | Every endpoint requires `RequireAuthorization()` (AZ-487); `POST /api/satellite/upload` requires the `GPS` permission via `PermissionsRequirement` (AZ-488). No IDOR analysis is needed because the service has no per-user data partitioning — every authenticated principal can read every tile. **Warning**: per-tenant authorization (e.g. "this UAV may only upload over its assigned region") is *not* enforced. If a future contract demands it, A01 immediately re-opens. |
|
||||
| A02 | Security Misconfiguration | FAIL (S1, S2, I1, I2) | **FAIL** (unchanged + F-AUTH-1) | Cycle-2 ships a clearly-labelled DEV-ONLY JWT secret in `appsettings.Development.json`. Production override path is correct (env-var wins); deploy gate must check `JWT_SECRET`. No new cycle-1 findings resolved. |
|
||||
| A03 | Supply Chain Failures | PASS_WITH_WARNINGS (D1, D2) | **PASS_WITH_WARNINGS** (+ D3, F-DEPS-UAV) | New `JwtBearer 8.0.21` package shares the D1 patch line; new ImageSharp call site widens decoder exposure (mitigations sufficient — see static_analysis.md F-UAV-1). |
|
||||
| A04 | Cryptographic Failures | N/A | **PASS** | HS256 token validation uses `Microsoft.IdentityModel`'s `SymmetricSecurityKey` with `RequireSignedTokens = true` and `RequireExpirationTime = true`. The `alg=none` bypass is blocked by `RequireSignedTokens`; algorithm-confusion is bounded because only one signing key is registered. Secret length ≥ 32 bytes enforced at startup. |
|
||||
| A05 | Injection | PASS | **PASS** | No new SQL / shell / template surfaces. The new JSON parse (`PermissionsAuthorizationHandler`) runs on signature-validated token bytes — see F-UAV-2 disposition. |
|
||||
| A06 | Insecure Design | FAIL (S3, S4, I3) | **FAIL** (+ F-AUTH-3, F-UAV-3) | Rate limiting still absent (now also a 401-flood vector). UAV reject reasons disclose gate structure — accepted UX trade-off, flagged for operator awareness. |
|
||||
| A07 | Identification & Authentication Failures | N/A (with caveat) | **PASS_WITH_WARNINGS** (+ F-AUTH-2) | HS256 with secret ≥ 32 bytes; lifetime + signature validation; ClockSkew = 30 s. **Warning**: `ValidateIssuer = false`, `ValidateAudience = false` per the suite contract — any service that holds `JWT_SECRET` can mint tokens accepted here. Track until admin team defines `iss`/`aud`. No token revocation list — leaked tokens stay valid until `exp`. |
|
||||
| A08 | Software or Data Integrity Failures | PASS | **PASS** | AZ-488 file-first-then-row write order documented; same migration / CI discipline as cycle 1. |
|
||||
| A09 | Security Logging Failures | PASS_WITH_WARNINGS (I4) | **PASS_WITH_WARNINGS** (unchanged) | No new logging changes; 401 responses are not currently aggregated for alerting (out of scope for internal service). |
|
||||
| A10 | Mishandling of Exceptional Conditions | PASS | **PASS** | UAV decode failures wrapped in scoped `try/catch` for `UnknownImageFormatException` / `InvalidImageContentException` — produce structured `INVALID_FORMAT` rejects, no stack-trace leak. SEC-11 test verifies reject details have no path / exception-type leakage.
|
||||
|
||||
### Cycle-2 cross-reference
|
||||
|
||||
| OWASP Cat | Cycle-2 finding | Severity | Source phase |
|
||||
|-----------|-----------------|----------|--------------|
|
||||
| A01 | A01 status now PASS_WITH_WARNINGS (per-tenant authz absent) | — (status note) | Phase 3 |
|
||||
| A02 | F-AUTH-1 — DEV-ONLY secret in `appsettings.Development.json` | Low (accepted) | Phase 2 |
|
||||
| A03 | D3 — `JwtBearer 8.0.21` shares D1 patch line | Low | Phase 1 |
|
||||
| A03 | F-DEPS-UAV — ImageSharp decode exposure widened | Medium | Phase 1 |
|
||||
| A06 | F-AUTH-3 — rate-limit gap now also covers 401 floods | Low (recurrence of I3) | Phase 2 |
|
||||
| A06 | F-UAV-3 — reject reasons disclose gate structure | Informational (accepted) | Phase 2 |
|
||||
| A07 | F-AUTH-2 — `iss`/`aud` not validated; no revocation list | Medium | Phase 2 |
|
||||
| (claim handler) | F-UAV-2 — `JsonDocument.Parse` on token claim values | Low | Phase 2 |
|
||||
|
||||
@@ -1,20 +1,21 @@
|
||||
# Security Audit Report
|
||||
|
||||
**Date**: 2026-05-11
|
||||
**Date**: 2026-05-11 (cycle 1 baseline) · 2026-05-11 cycle 2 refresh appended below
|
||||
**Scope**: Satellite Provider — full repository (Api, Common, DataAccess, Services.*, Tests, infra)
|
||||
**Trigger**: `/autodev` Step 14 (Security Audit) — feature cycle 1, post-AZ-484
|
||||
**Verdict**: **PASS_WITH_WARNINGS**
|
||||
**Trigger**: `/autodev` Step 14 (Security Audit) — feature cycle 1, post-AZ-484; **cycle 2 delta scan added 2026-05-11 covering AZ-487 JWT validation baseline + AZ-488 UAV tile upload endpoint**
|
||||
**Verdict (current, post-cycle-2)**: **PASS_WITH_WARNINGS**
|
||||
|
||||
## Summary
|
||||
|
||||
| Severity | Count |
|
||||
|----------|-------|
|
||||
| Critical | 0 |
|
||||
| High | 0 |
|
||||
| Medium | 5 |
|
||||
| Low | 5 |
|
||||
| Severity | Cycle 1 | Cycle 2 delta | Current total |
|
||||
|----------|---------|---------------|---------------|
|
||||
| Critical | 0 | 0 | 0 |
|
||||
| High | 0 | 0 | 0 |
|
||||
| Medium | 5 | 2 (F-AUTH-2, F-DEPS-UAV) | 7 |
|
||||
| Low | 5 | 4 (F-AUTH-1, F-AUTH-3, F-UAV-2, D3) | 9 |
|
||||
| Info | — | 1 (F-UAV-3) | 1 |
|
||||
|
||||
No Critical or High findings. The verdict is `PASS_WITH_WARNINGS` driven by 5 Medium findings, all of which are well-understood configuration / hardening gaps rather than exploitable vulnerabilities in the application logic itself. **AZ-484 (the cycle's only feature change) introduced zero new findings** — it is a pure data-layer change with no auth surface, no untrusted-input handling, and no new external dependencies.
|
||||
No Critical or High findings in either cycle. The verdict remains `PASS_WITH_WARNINGS`. Cycle 2's two new Medium findings (`iss`/`aud` not validated yet; ImageSharp decode exposure widened) are both bounded by mitigations already in place and tracked as follow-ups rather than gating items. **AZ-484 (cycle 1's only feature change) introduced zero new findings** — it remained a pure data-layer change.
|
||||
|
||||
## OWASP Top 10:2025 Assessment
|
||||
|
||||
@@ -118,3 +119,45 @@ This satisfies the autodev gate to proceed to Step 15 (Performance Test). The re
|
||||
- [x] Every finding has remediation guidance (in per-phase reports)
|
||||
- [x] Verdict matches severity logic (no Critical/High → not FAIL; >0 findings → not PASS)
|
||||
- [x] No real secret values printed in any audit artifact (S4 described without echoing the API key)
|
||||
|
||||
---
|
||||
|
||||
## Cycle 2 Delta Summary (AZ-487 + AZ-488)
|
||||
|
||||
### What changed in cycle 2
|
||||
|
||||
AZ-487 introduced a JWT validation baseline (HS256, `JWT_SECRET` env var, `.RequireAuthorization()` on every endpoint, Swagger Bearer hook). AZ-488 replaced the 501 `/api/satellite/upload` stub with a multipart batch endpoint that validates JPEGs via a 5-rule quality gate and persists accepted tiles. Two new packages were added: `Microsoft.AspNetCore.Authentication.JwtBearer 8.0.21` (Api) and `SixLabors.ImageSharp 3.1.11` (TileDownloader + Tests; consistent with the existing Api-level reference).
|
||||
|
||||
### Findings table (cycle-2 delta)
|
||||
|
||||
| # | Severity | Category | Location | Title |
|
||||
|------------|---------------|------------------------------------------|-------------------------------------------------------------------------|-----------------------------------------------------------------------------|
|
||||
| F-AUTH-1 | Low (accepted)| A02 — Misconfiguration | `SatelliteProvider.Api/appsettings.Development.json:14` | DEV-ONLY JWT secret committed; env-var overrides; operator must verify in prod |
|
||||
| F-AUTH-2 | Medium | A07 — AuthN / Identification | `Authentication/AuthenticationServiceCollectionExtensions.cs:31-32` | `iss`/`aud` not validated (intentional — suite contract has not defined values) |
|
||||
| F-AUTH-3 | Low (rec. I3) | A06 — Insecure Design | every `/api/satellite/*` endpoint | No rate limiting on 401-producing paths (extends cycle-1 I3) |
|
||||
| F-UAV-1 | Medium | A03 — Supply Chain (exposure) | `Services.TileDownloader/UavTileQualityGate.cs:60-95` | ImageSharp decode now runs on attacker-controlled JPEGs (mitigations OK) |
|
||||
| F-UAV-2 | Low | A07 — AuthN claim parsing | `Authentication/PermissionsRequirement.cs:84-111` | `JsonDocument.Parse` on signature-validated claim values (bounded by header cap) |
|
||||
| F-UAV-3 | Informational | A06 — Insecure Design (info-disclosure) | `Services.TileDownloader/UavTileQualityGate.cs` | Reject reasons disclose gate structure (accepted UX trade-off; documented in contract) |
|
||||
| D3 | Low | A03 — Supply Chain | `SatelliteProvider.Api.csproj` (new JwtBearer 8.0.21) | Shares D1 patch line; same remediation |
|
||||
| F-DEPS-UAV | Medium | A03 — Supply Chain (exposure) | new ImageSharp call site in TileDownloader | Documented in dependency_scan.md cycle-2 delta |
|
||||
|
||||
### Verdict reconciliation
|
||||
|
||||
- No new Critical or High findings → cycle 2 does NOT escalate the verdict.
|
||||
- Two new Medium findings — both are *follow-ups under existing remediations*, not blockers:
|
||||
- F-AUTH-2 waits on the admin team defining `iss`/`aud` (already flagged in AZ-487 § Constraints).
|
||||
- F-UAV-1 + F-DEPS-UAV jointly say "subscribe to ImageSharp GHSA and bump aggressively" — no immediate change needed.
|
||||
- F-AUTH-1 and F-UAV-3 are explicitly accepted.
|
||||
- F-AUTH-3 + D3 fold into existing cycle-1 remediations (I3 rate limiting, D1 8.0.x patch bump).
|
||||
|
||||
**Current verdict: PASS_WITH_WARNINGS** (cycle 2 satisfies the autodev Step-14 gate; proceed to Step 15).
|
||||
|
||||
### New / refreshed cycle-2 recommendations
|
||||
|
||||
- **Pre-deploy gate (operational, NOT code)**: `deploy/SKILL.md` must verify `JWT_SECRET` is set to a ≥ 32-byte value distinct from the DEV-ONLY placeholder. Cycle-2 deploys without this verification step are gated.
|
||||
- **Coordinate with admin team**: confirm expected `iss`/`aud` values; flip `ValidateIssuer` / `ValidateAudience` to `true` as soon as those values land. Track under AZ-487 § Constraints follow-up.
|
||||
- **Bump 8.0.x ASP.NET Core packages together**: the next D1 hardening commit must bump both `Microsoft.AspNetCore.OpenApi` AND `Microsoft.AspNetCore.Authentication.JwtBearer` to ≥ 8.0.25.
|
||||
- **ImageSharp subscribe-and-bump policy**: add to the runbook — patch within 7 days of any `SixLabors.ImageSharp` GHSA. Reconsider sandboxing if the upload endpoint is exposed beyond the trust boundary documented in architecture.md § 7.
|
||||
- **Cycle-2 hardening backlog (Low priority)**:
|
||||
- Pass `JsonDocumentOptions { MaxDepth = 8 }` and a max-claim-length check to `PermissionsAuthorizationHandler.TryReadJsonArray`.
|
||||
- Document in `architecture.md` that reject-reason codes are NOT a security boundary.
|
||||
|
||||
@@ -19,6 +19,10 @@
|
||||
| Sensitive data in logs | passwords, API keys, tokens, PII in log statements | **Clean** — `GlobalExceptionHandler.cs` logs only `Method`, `Path`, `correlationId`; client gets a generic 500 + correlationId. `CorsConfigurationValidator` warning (`PermissiveDefaultWarning`) does not include secrets. There is a deliberate test fixture `GlobalExceptionHandlerTests.cs:23` that uses `"Connection string Host=secret-db;Password=hunter2 failed at line 42"` to verify the handler does NOT echo exception messages back — this is a positive control, not a finding |
|
||||
| Verbose error responses | stack traces or internal details returned to clients | **Clean** — `GlobalExceptionHandler` returns RFC 7807 ProblemDetails with `Detail = "An unexpected error occurred. Use the correlationId to look up the server log entry."` |
|
||||
| Input validation | numeric ranges, geo coordinates, enum-like strings | See finding S3 |
|
||||
| Hardcoded credentials (cycle 2 delta) | `Jwt:Secret` value in `appsettings*.json` | `appsettings.Development.json` ships a clearly-tagged DEV-ONLY placeholder; `appsettings.json` ships `""`. `JWT_SECRET` env-var overrides both. See cycle-2 finding F-AUTH-1. |
|
||||
| Authentication / authorization (cycle 2 delta) | endpoint-level Authorize, custom requirement handlers, claim parsing | `Program.cs` applies `.RequireAuthorization()` on every existing endpoint and the GPS-permission policy on the new `/api/satellite/upload`. `PermissionsAuthorizationHandler` uses `string.Equals(..., Ordinal)` — no substring / case-confusion bypass. See cycle-2 findings F-AUTH-2 .. F-AUTH-4. |
|
||||
| Multipart binary input (cycle 2 delta) | uploaded bytes flowing into image decode / file write | `UavTileQualityGate` runs magic-byte check before ImageSharp, wraps decode in scoped `try/catch` for `UnknownImageFormatException` / `InvalidImageContentException`. File path is built from integer coords only via `UavTileUploadHandler.BuildUavTileFilePath`. See cycle-2 finding F-UAV-1. |
|
||||
| Untrusted JSON via claims (cycle 2 delta) | `JsonDocument.Parse(claim.Value)` in `PermissionsAuthorizationHandler` | Tokens are signature-validated *before* the handler runs, so the JSON parsed here is already framework-validated bytes from a verified token. Token size is bounded by Kestrel header limits. See cycle-2 finding F-UAV-2. |
|
||||
|
||||
## Findings
|
||||
|
||||
@@ -82,6 +86,52 @@
|
||||
- Add `.env.example` to the repo with `GOOGLE_MAPS_API_KEY=replace-with-your-own-key-from-cloud-console` and reference it in the README setup section.
|
||||
- Configure Google Cloud key restrictions: HTTP referrer allowlist (for browser keys) or IP allowlist (for server keys), and per-API quotas. Optional: per-developer keys.
|
||||
|
||||
---
|
||||
|
||||
## Cycle 2 Delta Findings (AZ-487 + AZ-488)
|
||||
|
||||
### F-AUTH-1 — Dev JWT secret is committed to `appsettings.Development.json` (Low — accepted by design)
|
||||
|
||||
- **Location**: `SatelliteProvider.Api/appsettings.Development.json:14` — `"Secret": "DEV-ONLY-DO-NOT-USE-IN-PROD-replace-with-real-secret-via-JWT_SECRET-env-var"`.
|
||||
- **Description**: A 73-byte placeholder labelled DEV-ONLY ships in the repo. The value is clearly tagged; `ResolveSecretOrThrow` in `AuthenticationServiceCollectionExtensions.cs:43` reads `JWT_SECRET` from the environment first and only falls back to config when it is unset, so a production deploy with `JWT_SECRET` set overrides it.
|
||||
- **Impact**: Cosmetic only — the placeholder is not a usable production secret (it is published on every git clone and would be rejected by any token verifier already in the wild). A careless operator who copies the file verbatim into prod and forgets to set `JWT_SECRET` would still pass the ≥32-byte gate, so the secret would *work* locally — that is the dependency to monitor.
|
||||
- **Disposition**: Accept. Mitigation: the `DEV-ONLY-DO-NOT-USE-IN-PROD` prefix is the operator-readable warning; the deploy skill must verify `JWT_SECRET` is set before promotion.
|
||||
|
||||
### F-AUTH-2 — JWT issuer / audience are not validated (Medium — by design, until admin team defines values)
|
||||
|
||||
- **Location**: `SatelliteProvider.Api/Authentication/AuthenticationServiceCollectionExtensions.cs:31-32` — `ValidateIssuer = false`, `ValidateAudience = false`.
|
||||
- **Description**: Per the suite contract `suite/_docs/10_auth.md`, expected `iss` / `aud` values are not yet defined. The validator therefore accepts any HS256 token signed with the correct shared secret — including tokens minted by other services in the suite that share the secret. This is a horizontal-trust risk: any service that holds `JWT_SECRET` can mint tokens accepted by satellite-provider as if they came from the admin API.
|
||||
- **Impact**: Bounded by the secret-distribution policy. Within the trust boundary documented in cycle 1's A01 caveat ("internal/trusted-network service") this is acceptable.
|
||||
- **Remediation (follow-up, NOT this cycle)**: When the admin team publishes `iss` / `aud` values, flip `ValidateIssuer = true` + `ValidIssuer = "<admin-iss>"` and the audience equivalent in `AddSatelliteJwt`. AZ-487 § Constraints already flags this as a small follow-up.
|
||||
|
||||
### F-AUTH-3 — No rate limiting on 401-producing paths (Low — recurrence of cycle-1 I3)
|
||||
|
||||
- **Location**: every `/api/satellite/*` endpoint after the AZ-487 `.RequireAuthorization()` middleware.
|
||||
- **Description**: An attacker can flood `Authorization: Bearer <random>` requests; each one triggers an HMAC verification (cheap, but non-zero) and an HTTP 401 response. This re-uses the cycle-1 I3 finding ("no inbound rate limiting on any HTTP endpoint") — the JWT layer didn't introduce a new vulnerability, but it did add a new cheap-to-trigger 401 surface that magnifies I3.
|
||||
- **Disposition**: Track under existing I3 remediation (wire `Microsoft.AspNetCore.RateLimiting`). No separate Jira.
|
||||
|
||||
### F-UAV-1 — ImageSharp decode on attacker-controlled bytes (Medium — exposure increase, mitigations sufficient today)
|
||||
|
||||
- **Location**: `SatelliteProvider.Services.TileDownloader/UavTileQualityGate.cs:60-95` — `Image.Identify` (Rule 3) and `Image.Load<L8>` + `Mutate(ctx => ctx.Resize)` (Rule 5).
|
||||
- **Description**: Pre-AZ-488, ImageSharp only decoded responses from the Google Maps tile CDN (trusted origin). AZ-488 added a second call site that decodes arbitrary `POST /api/satellite/upload` payloads. Current ImageSharp 3.1.11 is patched (see cycle-2 dependency-scan finding F-DEPS-UAV); the change here is *exposure*, not a present vulnerability.
|
||||
- **Mitigations in place**:
|
||||
- Rule 1 magic-byte gate runs before any ImageSharp call (`FF D8 FF` prefix required).
|
||||
- Rule 2 caps per-item size at 5 MiB; Kestrel + FormOptions cap the envelope at `MaxBatchSize × MaxBytes`.
|
||||
- Decode is wrapped in `try { … } catch (UnknownImageFormatException) { … } catch (InvalidImageContentException) { … }` — malformed JPEGs produce a structured `INVALID_FORMAT` reject; no unhandled exception reaches the client.
|
||||
- **Remediation**: Subscribe to `SixLabors.ImageSharp` GHSA advisories; bump within 7 days of a patch. Sandboxing (separate process / libvips + seccomp) is not warranted at the current trust boundary but should be reconsidered if the endpoint is exposed publicly. Recorded as recurring follow-up.
|
||||
|
||||
### F-UAV-2 — `JsonDocument.Parse` invoked on token-supplied claim values (Low — bounded by Kestrel header limits)
|
||||
|
||||
- **Location**: `SatelliteProvider.Api/Authentication/PermissionsRequirement.cs:84-111` — `JsonDocument.Parse(claim.Value)` when the `permissions` claim arrives as a JSON-array string.
|
||||
- **Description**: `JsonDocument.Parse` has no built-in depth or size limit. A maliciously-shaped permissions claim (e.g. deeply-nested array) would consume CPU/heap during parsing. The token has already passed HS256 signature validation by the time the handler runs, so this is only exploitable by a party that holds `JWT_SECRET` — i.e. another suite service or an admin-team principal — and only inside the issued-token-size window (bounded by Kestrel's `MaxRequestHeadersTotalSize`, default 32 KiB).
|
||||
- **Disposition**: Accept. The combination of `RequireSignedTokens = true` + header-size cap + ordinal-only string comparison makes a practical exploit prohibitive. Future hardening: pass `JsonDocumentOptions { MaxDepth = 8 }` to `JsonDocument.Parse` and reject claims longer than e.g. 8 KiB before parsing.
|
||||
|
||||
### F-UAV-3 — Reject reasons disclose gate structure (Informational — accepted trade-off)
|
||||
|
||||
- **Location**: `SatelliteProvider.Services.TileDownloader/UavTileQualityGate.cs` — each rule returns a distinct enum code.
|
||||
- **Description**: A client (or attacker who can present a `GPS`-permission token) can map the gate by probing inputs (1×1 black image → `WRONG_DIMENSIONS`; 1 KB JPEG → `SIZE_OUT_OF_BAND`; etc.). The thresholds are also documented in the public contract `_docs/02_document/contracts/api/uav-tile-upload.md`.
|
||||
- **Disposition**: Accept — UX (helping clients self-correct) outweighs the information-hiding benefit, especially since the contract is public anyway. Flagged to keep operators aware: rule thresholds are NOT a security boundary; do not move secrets into reject details.
|
||||
|
||||
## Self-verification
|
||||
|
||||
- [x] All production source directories scanned (Api, Common, DataAccess, Services.TileDownloader, Services.RegionProcessing, Services.RouteManagement)
|
||||
|
||||
@@ -2,8 +2,8 @@
|
||||
|
||||
## Current Step
|
||||
flow: existing-code
|
||||
step: 14
|
||||
name: Security Audit
|
||||
step: 15
|
||||
name: Performance Test
|
||||
status: not_started
|
||||
sub_step:
|
||||
phase: 0
|
||||
|
||||
Reference in New Issue
Block a user