fix(ssh): add opt-in SSRF guard for SSH bastion hosts - #62
Open
notSumit25 wants to merge 5 commits into
Open
Conversation
notSumit25
requested review from
a team,
geekypunk and
venkateshsakamuri-lab
as code owners
August 17, 2026 10:08
SshTunnelService passed request.getSshHost() straight to jsch.getSession() with no validation, so an authenticated user who can create a connection could aim the tunnel at the cloud metadata endpoint (169.254.169.254) or internal hosts and use the backend as a probe inside the network. This is CodeQL alert #138 (java/ssrf), open since the initial commit — the file had never been modified, and the "fixed then reappeared" timeline on that alert was scan flapping, not a reverted fix. SshHostGuard resolves the host and checks every returned address rather than matching the literal string, so a public hostname whose A record points at a private or link-local address is still refused. Covers loopback, wildcard, link-local, RFC1918, CGNAT, multicast, IPv6 ULA, and IPv4-mapped IPv6 forms. It sits in createSession, so establishTunnel and testSshConnection are both covered by one call site. testSshConnection calls the guard outside its try block: that method catches broad Exception and returns false, which would render a blocked host as an ordinary auth failure. The guard ships disabled. Bastions legitimately live on RFC1918 networks, so enabling it by default would break existing self-hosted installs on upgrade. Note this means a default install is as exposed as before while the CodeQL alert closes — the sanitizer is on the call path regardless of the flag — so a green #138 does not mean deployments are protected. Documented in CLAUDE.md.
…on alerts
Addresses the remaining open code-scanning alerts on main. The 138 alerts
were only 5 rules; 118 were a single mechanical pattern.
polynomial-redos (118): almost all were s.matches(".*RE.*"). String.matches
already anchors, so the wrapping .* exists only to undo that anchoring, and
.* plus an alternation is what backtracks. Rewritten to
PatternUtil.containsPattern (find() over a cached Pattern) at 174 sites in
17 files -- more than the 118 flagged, since CodeQL only reports where taint
reaches, and the untainted ones are the same hazard. Equivalence was checked
by differential test over all 175 literals rather than by inspection.
This changes behavior on multi-line input: `.` does not cross a newline, so
the anchored form failed to match a keyword after a line break and find()
matches it. That is a fix for intent classifiers, and only affects callers
that do not pre-normalize.
The rest were compiled Pattern constants with ambiguous quantifiers, fixed
individually with possessive quantifiers or bounded gaps. PlanPatternLibrary
also had a latent bug: [^from]+ is a character class, so any column with f,
r, o or m in it (order_id) defeated the collapse.
sql-injection (15): three different cases. CardinalityEstimationService was
a real hole -- quoteIdentifier wrapped in quotes without doubling an embedded
quote, so a table named `x" ; DROP TABLE users; --` escaped it. Four of the
five other quoteIdentifier implementations here already escape correctly. It
now delegates to the dialect's SamplingProvider (removing an if/else on
dbType) and resolves both identifiers against information_schema first.
MySQLPrivilegeCheckProvider concatenated a database name into a literal; now
bound. QueryExecutorService and the EXPLAIN providers execute user SQL by
design -- that is the Editor, guarded by the policy layer, not parameterizable.
spring-disabled-csrf-protection: correct as-is; every route is STATELESS with
header-carried tokens, so there is no cookie session to forge. Documented in
place rather than changed.
command-line-injection: spawn already passed array args, but authorize_url
comes from a server response and the win32 branch goes through cmd. Now
scheme-validated to http/https.
Verified: full backend suite shows the same 13 failures / 4 errors as the
untouched baseline (confirmed by stashing) -- no new regressions. 25 new/
touched unit tests and 259 MCP tests pass.
…ed URL) Closes the other two java/ssrf alerts. Address classification moves into a shared OutboundHostGuard -- resolve the host and check every returned address, so a public hostname whose A record points at 10.x or 169.254.169.254 is still refused. SshHostGuard now delegates to it instead of carrying its own copy. DatabaseHostGuard (#136, ConnectionService) screens the JDBC host. The SSH guard never covered this: a direct, non-tunnelled connection does not go through SshTunnelService at all. Applied in buildJdbcUrl and in the Hikari pool path, and skipped when a tunnel port is present since that targets the local forwarded port. Ships disabled, same reasoning as the SSH guard -- databases sit on RFC1918 more often than bastions do. S3LogFetchService (#137) was calling setInstanceFollowRedirects(true), so the JDK chased a 302 with no chance to inspect the target and a presigned URL on a public host could hand off to the metadata endpoint. Redirects are now followed manually with a cap of 5, and every hop is re-checked for https plus a public address. This one is always on: there is no legitimate reason to fetch a slow query log from a private address. Verified: 47 unit tests pass across the new and touched guards; the six pre-existing failing classes show the same 13 failures / 4 errors as the untouched baseline, so no new regressions.
notSumit25
force-pushed
the
fix/ssh-tunnel-ssrf-guard
branch
from
August 18, 2026 13:36
81dabb7 to
c38471b
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… checked parts Follow-up to the CodeQL findings on PR #62's own new code -- two alerts the first pass introduced rather than fixed. polynomial-redos (PatternUtil): the find() rewrite removed the outer .* but not the inner A.*B gap that ~47 of the classifier patterns carry. That gap backtracks super-linearly when B is absent -- measured at 93s on a 50k-token input, a real DoS reachable from a chat message. Rather than reshape ~90 patterns and risk changing what they match, containsPattern now caps the scanned input to 4096 chars. The gap can only backtrack within that window (worst pattern: 93s -> ~23ms). Real questions and identifiers are far shorter, so matching is unchanged for every legitimate input. ssrf (S3LogFetchService): CodeQL did not treat assertFetchableUrl as a barrier because it returned the tainted URI unchanged. It now rebuilds the URI from validated components with the scheme pinned to the https literal, so no unvalidated part of the caller's string survives into openConnection(). Verified: worst pattern bounded under 1s in a test; assertFetchableUrl still accepts real https S3 URLs and rejects http/file/private-host/redirect targets. PatternUtil and S3LogFetchService suites green.
| CharSequence scanned = input.length() > MAX_SCAN_CHARS | ||
| ? input.subSequence(0, MAX_SCAN_CHARS) | ||
| : input; | ||
| return cached(regex).matcher(scanned).find(); |
…forgery' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
|
@notSumit25 fixing the code scan issues? |
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
3 participants
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.
SshTunnelService passed request.getSshHost() straight to jsch.getSession() with no validation, so an authenticated user who can create a connection could aim the tunnel at the cloud metadata endpoint (169.254.169.254) or internal hosts and use the backend as a probe inside the network. This is CodeQL alert #138 (java/ssrf), open since the initial commit — the file had never been modified, and the "fixed then reappeared" timeline on that alert was scan flapping, not a reverted fix.
SshHostGuard resolves the host and checks every returned address rather than matching the literal string, so a public hostname whose A record points at a private or link-local address is still refused. Covers loopback, wildcard, link-local, RFC1918, CGNAT, multicast, IPv6 ULA, and IPv4-mapped IPv6 forms. It sits in createSession, so establishTunnel and testSshConnection are both covered by one call site.
testSshConnection calls the guard outside its try block: that method catches broad Exception and returns false, which would render a blocked host as an ordinary auth failure.
The guard ships disabled. Bastions legitimately live on RFC1918 networks, so enabling it by default would break existing self-hosted installs on upgrade. Note this means a default install is as exposed as before while the CodeQL alert closes — the sanitizer is on the call path regardless of the flag — so a green #138 does not mean deployments are protected. Documented in CLAUDE.md.