Skip to content

fix(security): clear open high-severity CodeQL alerts (SEC-X1) - #1534

Merged
NitinKumar004 merged 1 commit into
developmentfrom
fix/codeql-high-alerts-x1
Oct 10, 2026
Merged

NitinKumar004 merged 1 commit into
developmentfrom
fix/codeql-high-alerts-x1

Conversation

@NitinKumar004

Copy link
Copy Markdown
Collaborator

Summary

Clears the four open high-severity CodeQL alerts on development (#55, #93, #94, #95). Each one is fixed at the cause. There are no //nolint or // lgtm suppressions.

#93 Weak password hash, providers/aws/cognito/passwords.go

Data flow: the InitiateAuth / AdminInitiateAuth PASSWORD parameter (auth.go) goes to verifyPassword, then to the old fallback sha256.Sum256(salt + password).

Cause: new hashes were already PBKDF2-SHA256 with a random 16-byte salt per user (pbkdf2-sha256$<iterations>$<hex>). The fallback still checked the earlier bare SHA-256 format, so it ran a fast hash over the password on every sign-in.

Fix: remove the fallback. A stored hash without the PBKDF2 prefix never matches, and sign-in returns NotAuthorizedException, the same as a wrong password. The iteration count stays at 10,000 as a named constant, with a comment that it is deliberately low for an emulator (fast tests, no real secrets). Each hash stores its own iteration count, so changing the constant later keeps existing hashes valid.

Compatibility: no release ever wrote the old format. It existed only on development between #1378 (Sep 27) and #1399 (Oct 3). v2.12.0 has no Cognito user records, and v2.13.0 already includes the PBKDF2 change. If a snapshot from a development build in that window holds such users, they can be recovered with AdminSetUserPassword (aws cognito-idp admin-set-user-password --permanent).

Tests: in TestVerifyPassword, the old-format case now expects a rejection. The new TestSignInRejectsUnprefixedHash checks that a user with a bare SHA-256 hash cannot sign in and that AdminSetUserPassword recovers the account. Both fail on the old code.

#94, #95 Integer narrowing, internal/vtl/eval.go toInt

Data flow: 64-bit integers from strconv.ParseInt (number literals, parsed JSON) and float64 values reach toInt, which converts them with int(t). toInt feeds:

  • list index reads and writes
  • the range operator [a..b]
  • every int method argument (get, set, charAt, substring, ...)

Cause: there was no bounds check before narrowing. On 32-bit targets the int64 value truncates. On every target, converting NaN, infinity or an out-of-range float to int is undefined. For example, [0..1e19] produced a 1001-item list on arm64 and something else on amd64.

Fix: toInt now accepts only the int32 range, which is the Java int that Velocity uses for indexes, offsets and range bounds. NaN, infinities and out-of-range values are treated like any other non-integer argument: the result is null, set does nothing, and a bad range evaluates to null. listRemove narrowed its int64 index with int(i) directly, so it now goes through the same check. Fractions still truncate.

Tests: TestToIntRange and TestOutOfRangeNumbersInTemplates cover ranges, index reads and writes, get, set, remove, substring and charAt with values past int32, the int64 maximum, 1e19, NaN and infinity. The out-of-range cases fail on the old code.

#55 Reflected XSS, features/vcr/vcr.go

Data flow: a request value echoed into an error body (for example X-Amz-Target in an ACM UnknownOperationException) goes to wire.WriteJSONError, then through the Azure overlay's writeJSONResponse, to the VCR recorder's captureWriter.Write, and finally to the live writer.

Cause: at runtime the response was already safe. Upstream writers set a JSON/XML content type, and the recorder kept it (or set application/octet-stream) and added X-Content-Type-Options: nosniff. But the headers were set in WriteHeader while the body was forwarded from Write. So the body write's safety relied on another method having run first, and CodeQL could not see a content type at the write.

Fix: WriteHeader and Write now both go through one send method, the only path to the live writer. Before the headers go out, send sets the content type (the handler's, or the default) and nosniff on that writer. Replay already works this way in writeRecorded. The body is not escaped: SDKs parse these API responses byte for byte, so the recorder must not change them. Only the first status is forwarded, as net/http does.

Tests: TestRecordSetsSafeHeaders adds cases for an explicit status with no content type, a text/plain type that should be kept, and a second WriteHeader call (the first one wins). The new TestRecordKeepsBodyAndStatus checks that the recorder forwards and records several body writes and the status unchanged. Behaviour is unchanged here, so these tests guard against regressions and do not fail on the old code.

Verification

  • go build ./..., go vet and go test -race on providers/aws/cognito, internal/vtl, features/vcr, providers/aws/apigateway, server/aws/apigateway, server/aws/cognito, server/serverkit and persist.
  • golangci-lint --new-from-rev on the touched packages reports 0 issues. go generate produces no diff.
  • Live run against cloudemu serve:
    • Cognito (AWS CLI): sign-up, confirm, sign-in, a wrong password returning NotAuthorizedException, admin-set-user-password, then the old password rejected and the new one accepted.
    • API Gateway MOCK integration: the response template uses 1e19, the int64 maximum, +Inf and NaN in get, substring, charAt and ranges. It renders empty values with HTTP 200 and no panic.
    • VCR: recorded Azure calls (resource group PUT with a <script> tag value, and a 404 with a reflected name) return application/json and nosniff in both record and replay.

…VTL int narrowing, VCR headers)

Drop the unreleased bare SHA-256 password fallback in Cognito, bounds-check VTL int conversions to the int32 range, and pin VCR record-mode headers at the body write.
@NitinKumar004
NitinKumar004 marked this pull request as ready for review October 10, 2026 16:54
@NitinKumar004
NitinKumar004 merged commit 8d5294b into development Oct 10, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant