BBlack has submitted this change and it was merged.
Change subject: varnish: sanitize XFF better
......................................................................
varnish: sanitize XFF better
Bug: T118769
Change-Id: I493fc7af7c2e99b81fb87b47d0e4bdd3a640a3df
---
M modules/varnish/templates/vcl/wikimedia.vcl.erb
1 file changed, 22 insertions(+), 13 deletions(-)
Approvals:
BBlack: Verified; Looks good to me, approved
diff --git a/modules/varnish/templates/vcl/wikimedia.vcl.erb
b/modules/varnish/templates/vcl/wikimedia.vcl.erb
index c65b3cd..c299861 100644
--- a/modules/varnish/templates/vcl/wikimedia.vcl.erb
+++ b/modules/varnish/templates/vcl/wikimedia.vcl.erb
@@ -401,6 +401,23 @@
unset req.http.X-Real-IP;
}
+ if (req.http.X-Forwarded-For) {
+ // To make further parsing/sanitizing simpler, convert all
whitespace
+ // in XFF to single spaces, and make sure all commas have a
space
+ // suffix but no space prefix.
+ set req.http.X-Forwarded-For =
regsuball(req.http.X-Forwarded-For, "[ \t]+", " ");
+ set req.http.X-Forwarded-For =
regsuball(req.http.X-Forwarded-For, " ?, ?", ", ");
+
+ // Now fully-sanitize it to only the strict form "X(, X)*",
where X is
+ // a string of legal characters in IPv[46] addresses. Note
+ // that injections can still leave well-formed junk on the
+ // left, but it's up to the trusted proxy code to ignore that,
+ // e.g.:
+ // "junk2, 123.123.123.123" -> "2, 123.123.123.123"
+ set req.http.X-Forwarded-For = regsub(req.http.X-Forwarded-For,
+ "^.*?([0-9A-Fa-f:.]+(, [0-9A-Fa-f:.]+)*)? *$", "\1");
+ }
+
// There are two possible cases here: either nginx acted as our TLS
// proxy and already set X-Real-IP (as well as appended the same value
// as XFF), or the traffic was direct to varnish-fe, in which case
@@ -408,6 +425,7 @@
if (!req.http.X-Real-IP) {
// direct-to-port-80 case, set XRIP ourselves
set req.http.X-Real-IP = client.ip;
+ set req.http.X-Client-IP = req.http.X-Real-IP;
set req.http.X-Trusted-Proxy = netmapper.map("proxies",
req.http.X-Real-IP);
// normalize to boolean post-netmapper (varnish-3.0.4...)
if (req.http.X-Trusted-Proxy == "") {
@@ -415,14 +433,13 @@
}
if (req.http.X-Trusted-Proxy && req.http.X-Forwarded-For) {
// get last from trusted-proxy-supplied XFF
- set req.http.X-Client-IP =
regsub(req.http.X-Forwarded-For, "^([^,]+,\s*)+", "");
- // Clean up XCIP leading/trailing whitespace
- set req.http.X-Client-IP = regsub(req.http.X-Client-IP,
"^\s*(\S*)\s*$", "\1");
+ set req.http.X-Client-IP =
regsub(req.http.X-Forwarded-For, "^([^,]+, )+", "");
}
} else {
// XRIP from nginx, XFF set/appended by nginx and contains at
// least XRIP at the end, possibly prepended by other addrs
// set externally by some proxy.
+ set req.http.X-Client-IP = req.http.X-Real-IP;
set req.http.X-Trusted-Proxy = netmapper.map("proxies",
req.http.X-Real-IP);
// normalize to boolean post-netmapper (varnish-3.0.4...)
if (req.http.X-Trusted-Proxy == "") {
@@ -435,17 +452,9 @@
// alias XRIP by definition), there would be no commas
// to match, so this would set XCIP = XRIP, which is
// the desirable/default state.
- set req.http.X-Client-IP =
regsub(req.http.X-Forwarded-For, ",\s*[^,]+$", "");
- set req.http.X-Client-IP = regsub(req.http.X-Client-IP,
"^([^,]+,\s*)+", "");
- // Clean up XCIP leading/trailing whitespace
- set req.http.X-Client-IP = regsub(req.http.X-Client-IP,
"^\s*(\S*)\s*$", "\1");
+ set req.http.X-Client-IP =
regsub(req.http.X-Forwarded-For, ", [^,]+$", "");
+ set req.http.X-Client-IP = regsub(req.http.X-Client-IP,
"^([^,]+, )+", "");
}
- }
-
- // If XCIP not set above, or looks invalid due to bad external XFF
- // input, just alias to XRIP
- if (!req.http.X-Client-IP || req.http.X-Client-IP !~
"^[0-9a-fA-F:.]+$") {
- set req.http.X-Client-IP = req.http.X-Real-IP;
}
// Now check carrier database for setting X-Carrier based on XCIP
--
To view, visit https://gerrit.wikimedia.org/r/257984
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I493fc7af7c2e99b81fb87b47d0e4bdd3a640a3df
Gerrit-PatchSet: 4
Gerrit-Project: operations/puppet
Gerrit-Branch: production
Gerrit-Owner: BBlack <[email protected]>
Gerrit-Reviewer: BBlack <[email protected]>
Gerrit-Reviewer: CSteipp <[email protected]>
Gerrit-Reviewer: Faidon Liambotis <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
