fix(ruleGenerators): sanitize customRules site/ip to prevent URL injection - #414
Open
sebastionoss wants to merge 1 commit into
Open
fix(ruleGenerators): sanitize customRules site/ip to prevent URL injection#414sebastionoss wants to merge 1 commit into
sebastionoss wants to merge 1 commit into
Conversation
|
@sebastiondev is attempting to deploy a commit to the 7sageer's projects Team on Vercel. A member of the Team first needs to authorize it. |
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.
Summary
User-supplied
customRules[].siteandcustomRules[].ipidentifiers are interpolated directly into rule-set download URLs generated for sing-box and Clash/mihomo subscriptions. BecausetoStringArray()accepted any string, an attacker crafting a subscription link could embed URL-significant characters (/,:,@,?,#, etc.) in those identifiers to escape the intended${SITE_RULE_SET_BASE_URL}${site}.srspath and cause a victim's proxy client to fetch an attacker-controlled rule-set from an arbitrary host.Concretely, in
src/config/ruleGenerators.jsthe pre-fix code did:with
sitecoming straight from the request query/body. A value likeevil.example.com/pwnalters the resolved URL that the downstream proxy client will request when the victim imports the subscription.Weakness class: URL injection into a downstream fetch (CWE-918 family — here the fetch is performed by the victim's proxy client using a URL the sublink-worker instance authored from attacker input).
Affected file / functions:
src/config/ruleGenerators.js—generateRuleSets()andgenerateClashRuleSets()(URLs built fromrule.site/rule.ip).Fix
Introduce
sanitizeRuleIds()which restricts identifiers to the conservative charset that matches legitimate upstream rule-set filenames (letters, digits,-,_, dot-separated), and rejects..sequences:Applied everywhere
rule.site/rule.ipare used to construct URLs or rule tags (both sing-boxgenerateRuleSetsand ClashgenerateClashRuleSets, plussite_rules/ip_rulesingenerateRulesfor consistency). Any identifier that would break out of the intended path — anything containing/,:,@,?,#,%, whitespace, or path-traversal — is silently dropped. Legitimate identifiers (google,category-ads-all,private) are unaffected because they already match the pattern used by upstream.srsfilenames.Rationale for filter-and-drop rather than throw: existing callers pass unvalidated request data and don't handle exceptions; silently dropping malformed entries preserves the current UX for legitimate users while closing the injection.
Proof of Concept
Before the fix, a crafted request injects an attacker-controlled URL into the generated sing-box config. Reproduce locally against the repo:
Pre-fix output contains:
[{"tag":"evil.example.com/pwn","type":"remote","format":"binary", "url":"https://.../evil.example.com/pwn.srs"}]When a victim imports the resulting subscription into sing-box or Clash, their client fetches the attacker-controlled rule-set URL and applies the attacker's routing rules — enabling traffic redirection / MITM of the victim's proxied traffic.
Post-fix,
sanitizeRuleIdsrejects the identifier andsite_rule_setsis empty. Re-running withsite:'google'still produces the expected legitimate entry, confirming the allowlist doesn't over-restrict real identifiers.Testing
npm test)..srsfilenames (google,category-ads-all,private) pass, and that URL-injection payloads (a/b,a:b,a@b,a?b,../x,a b) are dropped.generateRuleSets) and Clash (generateClashRuleSets) output to confirm no injected entries survive in either format.Security analysis
Preconditions to exploit:
Impact: attacker-controlled routing rule-sets are loaded by the victim's proxy client, letting the attacker steer arbitrary destinations through their own infrastructure (traffic interception, credential theft against proxied sites, selective routing manipulation).
Mitigation the fix provides: the identifier can no longer contain any character that alters URL structure, so
${BASE}${id}.srsalways resolves under the intended host and path.Adversarial review
Before submitting we tried to disprove the finding. We checked whether any upstream validation or framework protection blocks the payload — there is none;
customRulesis parsed from the request and passed straight to the generators. We also considered whether the worker itself makes the outbound request (which would be classic server-side SSRF); it does not — the injected URL lands in the config the victim downloads, and their proxy client performs the fetch. That still causes real harm because a service the victim trusts authors a fetch URL from attacker input, so CWE-918 remains the closest classification. Finally we verified the allowlist doesn't over-restrict real upstream identifiers by scanning predefined rule names used elsewhere in the repo — all matchSAFE_RULE_ID_RE.Discovered by the Sebastion AI GitHub App.