Skip to content

Prevent smokescreen upstream connections to its local IPs#282

Merged
rishikej-stripe merged 3 commits intomasterfrom
connect-local
Jan 9, 2026
Merged

Prevent smokescreen upstream connections to its local IPs#282
rishikej-stripe merged 3 commits intomasterfrom
connect-local

Conversation

@rishikej-stripe
Copy link
Contributor

Summary

This PR adds an additional check for the target IP to ensure the smokescreen proxy does not connect to itself if a client passes smokescreen's IP/localhost as the target address.

Motivation

Smokescreen does not prevent clients from issuing CONNECT requests that target Smokescreen's own listening address. If the ACL configuration permits connections to the proxy's IP (e.g., 127.0.0.1, localhost, its internal IP range or its external IP), a malicious client can establish a recursive connection loop. Each iteration of the loop creates a new nested tunnel, consuming file descriptors, memory, and goroutines, rapidly leading to resource exhaustion and Denial of Service (DoS).

Test plan

Tested locally. The PoC passes a burst of 10000 requests through 1000 concurrent workers to the smokescreen proxy.

Before

Client makes continuous recursive connections by keeping localhost as the target.

Screenshot 2026-01-07 at 11 53 21 PM

After
Smokescreen denies the request to any of its local IPs.

Screenshot 2026-01-07 at 11 54 11 PM

@coveralls
Copy link

coveralls commented Jan 7, 2026

Pull Request Test Coverage Report for Build 20851618744

Details

  • 52 of 66 (78.79%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.5%) to 55.992%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/smokescreen/smokescreen.go 13 19 68.42%
pkg/smokescreen/config.go 39 47 82.98%
Totals Coverage Status
Change from base Build 20331928078: 0.5%
Covered Lines: 1752
Relevant Lines: 3129

💛 - Coveralls

Copy link
Contributor

@saurabhbhatia-stripe saurabhbhatia-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, can we add some unit tests for the new lines added.

sohamsen-stripe
sohamsen-stripe previously approved these changes Jan 9, 2026
@rishikej-stripe rishikej-stripe merged commit eae8104 into master Jan 9, 2026
12 checks passed
@rishikej-stripe rishikej-stripe deleted the connect-local branch January 9, 2026 12:24
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.

4 participants