fix: address Mend SAST findings from #215 (4 of 5 High) - #226
Open
peatey wants to merge 3 commits into
Open
Conversation
SafePath deleted every literal ".." substring and then called filepath.Clean.
Doing the removal first is what breaks it: any sequence that re-forms a
traversal after deletion survives, because Clean then resolves it.
SafePath("/scratch", "....//....//etc/passwd") -> "/scratch/etc/passwd"
SafePath("/scratch", "..././..././etc/shadow") -> "/scratch/etc/shadow"
The same substring deletion silently corrupted legitimate names, rewriting
a cluster id of "v1..2-cluster" to "v12-cluster" and writing samples to the
wrong directory.
Replaces the substring deletion with segment-aware filtering: split on the
separator and discard only segments that are exactly ".." or ".". A segment
that merely contains dots is an ordinary filename and is preserved, so
"...." and "v1..2-cluster" survive intact and neither can resolve to a
parent reference. Adds an explicit containment check against the base
element as defence in depth, rather than relying on that reasoning alone.
The documented contract is unchanged: parent segments are dropped rather
than resolved, so "test/scratch/../file" still yields "test/scratch/file"
and not "test/file", and trailing separators are still preserved. All
eight pre-existing test cases pass untouched; six were added covering the
two bypasses, base escape, and the legitimate-name regression.
This is the source Mend traces for three of the five high severity findings
in #215 -- the path traversal at uploader.go:469 and the file manipulation
findings at uploader.go:371 and uploader.go:507 all flow from the
SafePath call at uploader.go:322.
Refs #215
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The upload URL is not chosen by the agent. It is read from the "location" field of a Cloudability API response and then used as the destination for a PUT carrying the cluster's entire resource inventory. Nothing checked it, so a tampered or malicious response could redirect that payload to any host (CWE-918, Mend #215, cldy/metrics_collector.go:241). Adds validateUploadURL, called at the top of uploadPayloadToPresignedURL: - the scheme must be https, so a response cannot downgrade the upload to plaintext - the host must sit under one of amazonaws.com, cloudability.com or apptio.com Suffix matching is anchored on a dot boundary, so a lookalike host such as "evil-amazonaws.com" does not satisfy "amazonaws.com". uploadPayloadToPresignedURL is shared by both upload paths, so this covers the Apptio Frontdoor flow as well as the metrics-collector flow even though Mend only flagged the latter. Test fixtures previously returned locations on example.com, and in the uploader mocks a bare relative path with no scheme or host at all. Those were never realistic -- a relative location would fail in production at request time regardless -- so they now use a representative S3 presigned host. The countByPath assertions move with them, since an absolute URL yields a leading slash on Path. Adds five cases covering an attacker-controlled host, a lookalike host, a non-dot-boundary suffix match, a plaintext downgrade and a schemeless location. Confirmed by mutation: with the validation call removed the new cases fail, so they are exercising the control rather than passing incidentally. NOTE FOR REVIEW: the allowlist is the one part of this change that could break uploads in production if it is wrong, and it was derived from reading the code rather than from the API contract. Anyone who owns that contract should confirm no region or deployment returns a presigned URL on a domain outside those three. Refs #215 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The generated output tree was created with 0755 directories and 0644 files. Mend reports the directory mode as CWE-732, insecure directory permissions (#215, main.go:42). This is a developer code-generation tool rather than part of the agent runtime, and the content it writes is not sensitive, which is why it was initially left out of the runtime permissions fix. It is a one-line change that closes a high severity finding, so there is no reason not to make it. Directories move to 0750 and files to 0600, both named as constants. Refs #215 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes 4 of the 5 High severity findings in #215. Three commits, revertible
separately.
uploader.go:469uploader.go:371uploader.go:507metrics_collector.go:241bingen-to-json/main.go:42nodes/config.go:45,48clients.go:753,emitter.go:110util.go:951.
SafePathwas not safe — closes three Highs at onceMend taints all three
uploader.goHighs back to one source: theSafePath(samplePath)call atuploader.go:322. It doesn't trustSafePathasa sanitiser, and it's right not to.
The function deleted every literal
..substring and then calledfilepath.Clean. Doing the removal first is the bug — any sequence thatre-forms a traversal after deletion survives, because
Cleanthen resolves it:The same substring deletion also silently corrupted legitimate names — a cluster
id of
v1..2-clusterbecamev12-cluster, writing samples to the wrongdirectory.
Replaced with segment-aware filtering: split on the separator, discard only
segments that are exactly
..or.. A segment that merely contains dots isan ordinary filename, so
....andv1..2-clustersurvive intact and neithercan resolve to a parent reference. An explicit containment check against the
base element is added as defence in depth rather than relying on that reasoning.
The documented contract is unchanged — parent segments are dropped rather
than resolved (
test/scratch/../file→test/scratch/file, nottest/file),and trailing separators are preserved. All eight pre-existing test cases pass
untouched; six were added for the two bypasses, base escape, and the
legitimate-name regression.
2. CWE-918 — presigned upload destination was unvalidated
uploadPayloadToPresignedURLPUTs the cluster's entire resource inventory to aURL read from the
locationfield of an API response. Nothing checked it, so atampered response could redirect that payload anywhere.
validateUploadURLnow requireshttps(no plaintext downgrade) and a hostunder
amazonaws.com,cloudability.comorapptio.com, matched on a dotboundary so
evil-amazonaws.comdoesn't satisfyamazonaws.com.This function is shared by both upload paths, so the Apptio Frontdoor flow is
covered too, even though Mend only flagged the metrics-collector one.
Test fixtures previously returned locations on
example.com, and the uploadermocks used a bare relative path with no scheme or host at all — never realistic,
since a relative location would fail at request time in production regardless.
They now use a representative S3 presigned host, and the
countByPathassertions move with them since an absolute URL yields a leading slash on
Path.3. CWE-732 —
bingen-to-jsonoutput permissions0755directories,0644files →0750/0600. I'd originally excluded thisas "a dev tool, not runtime". Mend rates it High and it's a one-line change, so
that distinction wasn't worth defending.
Verification
The five new SSRF specs were mutation-checked: with the
validateUploadURLcall removed they fail, and with it restored they pass — so they exercise the
control rather than passing incidentally.
Deliberately not in this PR
InsecureSkipVerify: true. This is the kubelet TLS issue: the service-account bearer token is sent to node IPs over unverified TLS. The secure branch trusts the API server CA, which doesn't sign kubelet serving certs on most distros, so operators get pushed towardINSECURE=true. That needs a design decision (refuse to attach the token on an insecure transport, and fall back to the kube-proxy path?), not a lint fix. Happy to write up options.stringand therefore un-zeroable. Worth noting these are not purely theoretical here:/debug/pprof/heapis exposed on the same unauthenticated port whenPPROF_ENABLEDis set, which is a real path to reading the API key out of a heap dump. Fixing properly means moving these to[]bytewith the zeroing pattern already used elsewhere in the file.util.go:95. Genuine false positive — it's the S3Content-MD5header the API requires, not a security primitive. Should be suppressed asfalse-positiverather than fixed.Relationship to other open PRs
Independent of #223 (deps), #224 (govulncheck CI) and #225 (timeouts + runtime
dir perms). Branched from
develop; no overlap in files with #225 despite bothtouching
cldy/, sinceSafePath's signature is unchanged.Refs #215
🤖 Generated with Claude Code