Skip to content

security: fix 13 findings from security audit#843

Merged
javuto merged 2 commits into
jmpsec:mainfrom
alvarofraguas:pr/security-audit-fixes
May 22, 2026
Merged

security: fix 13 findings from security audit#843
javuto merged 2 commits into
jmpsec:mainfrom
alvarofraguas:pr/security-audit-fixes

Conversation

@alvarofraguas
Copy link
Copy Markdown
Collaborator

Summary

Addresses the 13 security findings identified in the evilsocket audit report. All fixes have been deployed and verified on a dockerized test environment (osctrl-api + osctrl-admin + osctrl-tls + Keycloak + Auth0 SAML) with 24 automated integration tests passing.

Findings fixed

# Severity Finding Fix
1 High IDOR — delete node in env A by UUID belonging to env B GetByUUIDEnv lookup before delete in nodes.go
2 High Empty-password login for SSO-provisioned users Reject empty passwords with timing-safe dummy bcrypt; hash random token for JIT users instead of empty string
3 High Federated login overwrites local admin account Guard in resolveFederatedUser: reject when AuthSource=="" (local); allow cross-protocol switch with ChangeAuthSource()
4 High Same local-account hijack in legacy admin OIDC flow Mirror guard in cmd/admin/oidc.go resolveOIDCUser
5 Medium OIDC email claim trusted without email_verified Gate on email_verified in claims.go; reject unverified emails
6 High CarveBlock accepted for wrong environment carve.EnvironmentID != env.ID check in post.go
7 High Path traversal in carved file reassembly filepath.Base() on every path segment in carves/utils.go
8 High Log ingestion cross-env — node in env A logs to env B Hard return on envid != node.EnvironmentID in logging/process.go
9 Medium Environment detail leaks secrets to non-admin users projectEnvironmentView() strips Secret, Certificate, Flags, ConfigTLS for non-admin
10 Medium XSS in admin node detail template jQuery .text() instead of .html() for user-controlled fields
11 Medium Carve permission check uses wrong constant CarveLevel throughout carves.go handlers
12 Medium Cross-env carve archive download Env ownership check before serving archive in get.go
13a Medium S3 carve block_id unbounded — write to arbitrary keys Bounds check [0, 9998] in s3.go
13b Medium TLS secret comparison not timing-safe subtle.ConstantTimeCompare in post.go

Additional changes

  • Login: Removed per-environment login requirement — POST /api/v1/login authenticates against all environments. Simplifies the SPA login flow (no env dropdown needed pre-auth).
  • SAML cross-protocol: Users who were JIT-provisioned via OIDC can now log in via SAML (and vice versa) with auth_source updated on each federated login.
  • Logout protocol detection: Logout handler now uses the osctrl_id_token cookie (set only by OIDC callback) to determine which IdP logout flow to trigger, instead of relying on the user's creation-time auth_source. Password users no longer get incorrectly redirected to the SAML IdP logout URL.

Test plan

  • 24 automated integration tests on dockerized environment (Keycloak OIDC + Auth0 SAML + password auth)
  • Verified IDOR: delete node in env B from env A returns 404
  • Verified empty-password rejection for SSO users (timing-safe)
  • Verified local-account hijack blocked for both OIDC and SAML
  • Verified cross-env carve block / log ingestion / archive download blocked
  • Verified env detail response stripped for non-admin
  • Verified XSS escape in admin node template
  • Verified S3 block_id bounds and timing-safe secret comparison
  • Verified logout protocol detection: OIDC → end_session_endpoint, SAML → /v2/logout with returnTo, password → /login
  • go vet ./... clean
  • Frontend builds clean (npm run build)

…raversal

- Cross-env node deletion IDOR: verify node belongs to env before delete
- Blank-password bypass on JIT SSO accounts: reject empty passwords,
  store unusable hash for SSO-only users
- OIDC/SAML username-collision account takeover: reject federated login
  when username belongs to a local account or different auth source
- Same fix in legacy admin binary
- Missing email_verified gate on OIDC email claim as username
- CarveBlockHandler env-membership check: verify carve env matches URL env
- Path traversal via host_identifier in carve archive names: filepath.Base
- Query result forgery: hard return on node/env mismatch in ProcessLogQueryResult
- Enroll secret leaked to UserLevel users: apply projectEnvironmentView
  for non-admin callers in env list endpoint
- Stored XSS in legacy admin status logs: HTML-escape DataTables render
- Wrong permission level (QueryLevel→CarveLevel) in CarvesRunHandler
  extra-environment loop
- Cross-env carve archive download: verify carve env matches requested env
- S3 block_id bounds check: reject out-of-range values before int32 cast
- Non-constant-time secret comparison: use subtle.ConstantTimeCompare
  in OsqueryConfigEndpointHandler
@alvarofraguas alvarofraguas force-pushed the pr/security-audit-fixes branch from 035cd79 to 2d36304 Compare May 21, 2026 19:58
@alvarofraguas alvarofraguas changed the title security: fix 13 findings from evilsocket audit security: fix 13 findings from security audit May 21, 2026
Allow federated users to login via both OIDC and SAML (same IdP, different
protocol) by updating auth_source on each login instead of rejecting. Local
accounts are still blocked from federated takeover.

Wire SAML logout URL (--saml-logout-url / SAML_LOGOUT_URL) into the logout
handler response so the SPA redirects to the IdP's logout endpoint,
terminating the IdP session cookie. Without this, clicking "Continue with
SAML" after logout silently re-authenticated.
@alvarofraguas alvarofraguas force-pushed the pr/security-audit-fixes branch from 2d36304 to 4ed2fbf Compare May 21, 2026 20:01
@javuto javuto added osctrl-tls osctrl-tls related changes osctrl-api osctrl-api related changes osctrl-admin osctrl-admin related changes 🚧 bugfix Fix for an existing bug 🔐 security Security related issues labels May 22, 2026
@javuto javuto merged commit 4e2723d into jmpsec:main May 22, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🚧 bugfix Fix for an existing bug osctrl-admin osctrl-admin related changes osctrl-api osctrl-api related changes osctrl-tls osctrl-tls related changes 🔐 security Security related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants