+
+An original client IP address is retrieved from an http header (X-Forwarded-For or X-Real-IP or Proxy-Client-IP
+etc.), which is used to ensure security. Attackers can forge the value of these identifiers to
+bypass a ban-list, for example.
+
+
+
+
+Do not trust the values of HTTP headers allegedly identifying the originating IP. If you are aware your application will run behind some reverse proxies then the last entry of a X-Forwarded-For header value may be more trustworthy than the rest of it because some reverse proxies append the IP address they observed to the end of any remote-supplied header.
+
+
+
+
+The following examples show the bad case and the good case respectively.
+In Bad1 method and Bad2 method, the client ip the X-Forwarded-For is split into comma-separated values, but the less-trustworthy first one is used. Both of these examples could be deceived by providing a forged HTTP header. The method
+Good1 similarly splits an X-Forwarded-For value, but uses the last, more-trustworthy entry.
+
+
+
+
+
+
+Dennis Schneider:
+Prevent IP address spoofing with X-Forwarded-For header when using AWS ELB and Clojure Ring
+
+
+Security Rule Zero: A Warning about X-Forwarded-For
+
+
+
+
diff --git a/csharp/ql/src/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql b/csharp/ql/src/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql
new file mode 100644
index 000000000000..aeb2aa579ee3
--- /dev/null
+++ b/csharp/ql/src/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql
@@ -0,0 +1,41 @@
+/**
+ * @name IP address spoofing
+ * @description A remote endpoint identifier is read from an HTTP header. Attackers can modify the value
+ * of the identifier to forge the client ip.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id csharp/ip-address-spoofing
+ * @tags security
+ * external/cwe/cwe-348
+ */
+
+import csharp
+import ClientSuppliedIpUsedInSecurityCheckLib
+import DataFlow::PathGraph
+
+/**
+ * Taint-tracking configuration tracing flow from obtaining a client ip from an HTTP header to a sensitive use.
+ */
+class ClientSuppliedIpUsedInSecurityCheckConfig extends TaintTracking::Configuration {
+ ClientSuppliedIpUsedInSecurityCheckConfig() { this = "ClientSuppliedIpUsedInSecurityCheckConfig" }
+
+ override predicate isSource(DataFlow::Node source) {
+ source instanceof ClientSuppliedIpUsedInSecurityCheckSource
+ }
+
+ override predicate isSink(DataFlow::Node sink) {
+ sink instanceof ClientSuppliedIpUsedInSecurityCheckSink
+ }
+
+ override predicate isSanitizer(DataFlow::Node node) {
+ node instanceof ClientSuppliedIpUsedInSecurityCheckSanitizer
+ }
+}
+
+from
+ DataFlow::PathNode source, DataFlow::PathNode sink,
+ ClientSuppliedIpUsedInSecurityCheckConfig config
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "IP address spoofing might include code from $@.",
+ source.getNode(), "this user input"
diff --git a/csharp/ql/src/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll b/csharp/ql/src/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll
new file mode 100644
index 000000000000..f7a7b7710c78
--- /dev/null
+++ b/csharp/ql/src/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheckLib.qll
@@ -0,0 +1,170 @@
+private import semmle.code.csharp.security.dataflow.flowsources.Remote
+private import semmle.code.csharp.frameworks.microsoft.AspNetCore
+private import semmle.code.csharp.frameworks.System
+private import semmle.code.csharp.security.dataflow.SqlInjectionQuery
+
+/** A data flow source for ip address forgery vulnerabilities. */
+abstract class ClientSuppliedIpUsedInSecurityCheckSource extends DataFlow::Node { }
+
+/** A data flow sink for ip address forgery vulnerabilities. */
+abstract class ClientSuppliedIpUsedInSecurityCheckSink extends DataFlow::ExprNode { }
+
+/** A data flow sanitizer for ip address forgery vulnerabilities. */
+abstract class ClientSuppliedIpUsedInSecurityCheckSanitizer extends DataFlow::ExprNode { }
+
+/**
+ * A data flow source of the client ip obtained according to the remote endpoint identifier specified
+ * (`X-Forwarded-For`, `X-Real-IP`, `Proxy-Client-IP`, etc.) in the header.
+ *
+ * For example: `Request.Headers["X-Forwarded-For"]` or `request.ServerVariables["HTTP_X_FORWARDED_FOR"]`.
+ */
+private class AspNetQueryStringSource extends ClientSuppliedIpUsedInSecurityCheckSource,
+ DataFlow::ExprNode {
+ AspNetQueryStringSource() {
+ exists(RemoteFlowSource rfs, RefType rt, IndexerAccess ia, Call c |
+ rfs.asExpr() = ia.getQualifier() and
+ ia.getTarget() = rt.getAnIndexer() and
+ c.getTarget() = rt.getAnIndexer().getGetter() and
+ c.getArgument(0).(StringLiteral).getValue().toLowerCase() = clientIpParameterName() and
+ this.getExpr() = c
+ )
+ or
+ // `Request.Headers.TryGetValue("X-Forwarded-For", out var xHeader)`
+ exists(RemoteFlowSource rfs, MethodCall mc |
+ rfs.asExpr() = mc.getQualifier() and
+ mc.getArgument(0).(StringLiteral).getValue().toLowerCase() = clientIpParameterName() and
+ this.getExpr() = mc
+ )
+ }
+}
+
+/**
+ * A data flow source of the client ip obtained according to the remote endpoint identifier specified
+ * (`X-Forwarded-For`, `X-Real-IP`, `Proxy-Client-IP`, etc.) in the header.
+ *
+ * For example: `[FromHeader(Name = "X-Forwarded-For")]`.
+ */
+private class AspNetCoreActionMethodParameterSource extends ClientSuppliedIpUsedInSecurityCheckSource,
+ DataFlow::ParameterNode {
+ AspNetCoreActionMethodParameterSource() {
+ exists(RemoteFlowSource rfs, Parameter p |
+ rfs.asParameter() = p and
+ p.getAnAttribute().getType().hasQualifiedName("Microsoft.AspNetCore.Mvc.FromHeaderAttribute") and
+ p.getAnAttribute().getNamedArgument("Name").(StringLiteral).getValue().toLowerCase() =
+ clientIpParameterName() and
+ this.getParameter() = p
+ )
+ }
+}
+
+/**
+ * A data flow sink for remote client ip comparison.
+ *
+ * For example: `if (!ip.StartsWith("192.168."))` determine whether the client ip starts
+ * with `192.168.`, and the program can be deceived by forging the ip address.
+ */
+private class CompareSink extends ClientSuppliedIpUsedInSecurityCheckSink {
+ CompareSink() {
+ exists(MethodCall mc |
+ mc.getTarget().hasQualifiedName("System.String", ["Equals", "Contains"]) and
+ mc.getTarget().getDeclaringType() instanceof StringType and
+ (
+ // ip.Equals("127.0.0.1")
+ this.getExpr() = mc.getQualifier() and
+ mc.getArgument(0).getValue() instanceof PrivateHostName and
+ not mc.getArgument(0).getValue() = "0:0:0:0:0:0:0:1"
+ or
+ // "127.0.0.1".Equals(ip)
+ this.getExpr() = mc.getArgument(0) and
+ mc.getQualifier().(StringLiteral).getValue() instanceof PrivateHostName and
+ not mc.getQualifier().(StringLiteral).getValue() = "0:0:0:0:0:0:0:1"
+ )
+ )
+ or
+ exists(ComparisonOperation co |
+ (co instanceof EQExpr or co instanceof NEExpr) and
+ this.getExpr() = co and
+ if co.getRightOperand().hasValue()
+ then (
+ // ip == "127.0.0.1"
+ co.getRightOperand().getType() instanceof StringType and
+ co.getRightOperand().(StringLiteral).getValue() instanceof PrivateHostName and
+ not co.getRightOperand().(StringLiteral).getValue() = "0:0:0:0:0:0:0:1"
+ ) else (
+ // "127.0.0.1" == ip
+ co.getLeftOperand().getType() instanceof StringType and
+ co.getLeftOperand().(StringLiteral).getValue() instanceof PrivateHostName and
+ not co.getLeftOperand().(StringLiteral).getValue() = "0:0:0:0:0:0:0:1"
+ )
+ )
+ or
+ exists(MethodCall mc |
+ mc.getTarget().hasQualifiedName("System.String", ["StartsWith", "Contains"]) and
+ mc.getTarget().getDeclaringType() instanceof StringType and
+ (
+ // ip.StartsWith("192.168.")
+ this.getExpr() = mc.getQualifier() and
+ mc.getArgument(0).(StringLiteral).getValue().regexpMatch(getIpAddressRegex())
+ or
+ // "192.168.".StartsWith(ip)
+ this.getExpr() = mc.getArgument(0) and
+ mc.getQualifier().(StringLiteral).getValue().regexpMatch(getIpAddressRegex())
+ )
+ )
+ }
+}
+
+/** A data flow sink for sql operation. */
+private class SqlOperationSink extends ClientSuppliedIpUsedInSecurityCheckSink {
+ SqlOperationSink() { this instanceof SqlInjectionExprSink }
+}
+
+/**
+ * Splitting a header value by `,` and taking an entry other than the first is sanitizing, because
+ * later entries may originate from more-trustworthy intermediate proxies, not the original client.
+ */
+private class SplitMethodSanitizer extends ClientSuppliedIpUsedInSecurityCheckSanitizer {
+ SplitMethodSanitizer() {
+ // Split(',')[0]
+ exists(ArrayAccess aa, MethodCall mc, SystemStringClass s |
+ this.getExpr() = aa.getQualifier() and
+ mc = aa.getQualifier() and
+ mc.getTarget() = s.getSplitMethod() and
+ not aa.getAnIndex().(IntLiteral).getValue() = "0"
+ )
+ or
+ // Split(',').FirstOrDefault()
+ exists(MethodCall mc, SystemStringClass s |
+ this.getExpr() = mc and
+ mc.getTarget() instanceof ExtensionMethod and
+ mc.getArgument(0).(MethodCall).getTarget() = s.getSplitMethod() and
+ not mc.getTarget()
+ .hasQualifiedName("System.Linq.Enumerable",
+ ["FirstOrDefault