diff --git a/csharp/ql/src/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.cs b/csharp/ql/src/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.cs new file mode 100644 index 000000000000..573b3ed03801 --- /dev/null +++ b/csharp/ql/src/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.cs @@ -0,0 +1,69 @@ +using System; +using Microsoft.AspNetCore.Mvc; + +namespace Testing.Controllers +{ + [Route("api/[controller]")] + [ApiController] + public class ClientSuppliedIpUsedInSecurityCheck : ControllerBase + { + [HttpGet("bad1")] + public IActionResult Bad1() + { + var ip = GetClientIp(); + if (!ip.StartsWith("192.168.")) + { + throw new Exception("illegal ip"); + } + + return Ok("Bad"); + } + + [HttpGet("bad2")] + public IActionResult Bad2() + { + var ip = GetClientIp(); + if (!ip.Equals("127.0.0.1")) + { + throw new Exception("illegal ip"); + } + + return Ok("Bad"); + } + + [HttpGet("good1")] + public IActionResult Good1() + { + string ip = Request.Headers["X-Forwarded-For"]; + // Good: if this application runs behind a reverse proxy it may append the real, + // remote IP to the end of any client-supplied X-Forwarded-For header. + ip = ip.Split(",")[ip.Split(",").Length - 1]; + + if (!ip.StartsWith("192.168.")) + { + throw new Exception("illegal ip"); + } + + return Ok(); + } + + private string GetClientIp() + { + var xfHeader = Request.Headers["X-Forwarded-For"]; + if (!string.IsNullOrEmpty(xfHeader)) + { + return xfHeader.ToString().Split(",")[0]; + } + + xfHeader = Request.HttpContext + .Connection.RemoteIpAddress?.ToString(); + + if (string.IsNullOrEmpty(xfHeader)) + { + throw new Exception("ip not found"); + } + + return xfHeader; + } + } +} diff --git a/csharp/ql/src/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp b/csharp/ql/src/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp new file mode 100644 index 000000000000..dbb0168c253f --- /dev/null +++ b/csharp/ql/src/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qhelp @@ -0,0 +1,35 @@ + + + +

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", "First"]) + ) + } +} + +/** + * A string matching private host names of IPv4 and IPv6, which only matches the host portion therefore checking for port is not necessary. + * Several examples are localhost, reserved IPv4 IP addresses including 127.0.0.1, 10.x.x.x, 172.16.x,x, 192.168.x,x, and reserved IPv6 addresses including [0:0:0:0:0:0:0:1] and [::1] + */ +private class PrivateHostName extends string { + bindingset[this] + PrivateHostName() { + this.regexpMatch("(?i)localhost(?:[:/?#].*)?|127\\.0\\.0\\.1(?:[:/?#].*)?|10(?:\\.[0-9]+){3}(?:[:/?#].*)?|172\\.16(?:\\.[0-9]+){2}(?:[:/?#].*)?|192.168(?:\\.[0-9]+){2}(?:[:/?#].*)?|\\[?0:0:0:0:0:0:0:1\\]?(?:[:/?#].*)?|\\[?::1\\]?(?:[:/?#].*)?") + } +} + +string clientIpParameterName() { + result in [ + "x-forwarded-for", "x-real-ip", "proxy-client-ip", "wl-proxy-client-ip", + "http_x_forwarded_for", "http_x_forwarded", "http_x_cluster_client_ip", "http_client_ip", + "http_forwarded_for", "http_forwarded", "http_via", "remote_addr" + ] +} + +string getIpAddressRegex() { + result = + "^((10\\.((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?)|(192\\.168\\.)|172\\.(1[6789]|2[0-9]|3[01])\\.)((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)$" +} diff --git a/csharp/ql/test/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.cs b/csharp/ql/test/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.cs new file mode 100644 index 000000000000..573b3ed03801 --- /dev/null +++ b/csharp/ql/test/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.cs @@ -0,0 +1,69 @@ +using System; +using Microsoft.AspNetCore.Mvc; + +namespace Testing.Controllers +{ + [Route("api/[controller]")] + [ApiController] + public class ClientSuppliedIpUsedInSecurityCheck : ControllerBase + { + [HttpGet("bad1")] + public IActionResult Bad1() + { + var ip = GetClientIp(); + if (!ip.StartsWith("192.168.")) + { + throw new Exception("illegal ip"); + } + + return Ok("Bad"); + } + + [HttpGet("bad2")] + public IActionResult Bad2() + { + var ip = GetClientIp(); + if (!ip.Equals("127.0.0.1")) + { + throw new Exception("illegal ip"); + } + + return Ok("Bad"); + } + + [HttpGet("good1")] + public IActionResult Good1() + { + string ip = Request.Headers["X-Forwarded-For"]; + // Good: if this application runs behind a reverse proxy it may append the real, + // remote IP to the end of any client-supplied X-Forwarded-For header. + ip = ip.Split(",")[ip.Split(",").Length - 1]; + + if (!ip.StartsWith("192.168.")) + { + throw new Exception("illegal ip"); + } + + return Ok(); + } + + private string GetClientIp() + { + var xfHeader = Request.Headers["X-Forwarded-For"]; + if (!string.IsNullOrEmpty(xfHeader)) + { + return xfHeader.ToString().Split(",")[0]; + } + + xfHeader = Request.HttpContext + .Connection.RemoteIpAddress?.ToString(); + + if (string.IsNullOrEmpty(xfHeader)) + { + throw new Exception("ip not found"); + } + + return xfHeader; + } + } +} diff --git a/csharp/ql/test/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected b/csharp/ql/test/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected new file mode 100644 index 000000000000..ab915389fdc2 --- /dev/null +++ b/csharp/ql/test/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.expected @@ -0,0 +1,23 @@ +edges +| ClientSuppliedIpUsedInSecurityCheck.cs:13:22:13:34 | call to method GetClientIp : Object | ClientSuppliedIpUsedInSecurityCheck.cs:14:18:14:19 | access to local variable ip | +| ClientSuppliedIpUsedInSecurityCheck.cs:25:22:25:34 | call to method GetClientIp : Object | ClientSuppliedIpUsedInSecurityCheck.cs:26:18:26:19 | access to local variable ip | +| ClientSuppliedIpUsedInSecurityCheck.cs:52:28:52:61 | access to indexer : StringValues | ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:31 | access to local variable xfHeader : StringValues | +| ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:31 | access to local variable xfHeader : StringValues | ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:42 | call to method ToString : String | +| ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:42 | call to method ToString : String | ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:53 | call to method Split [element] : Object | +| ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:53 | call to method Split [element] : Object | ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:56 | access to array element : Object | +| ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:56 | access to array element : Object | ClientSuppliedIpUsedInSecurityCheck.cs:13:22:13:34 | call to method GetClientIp : Object | +| ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:56 | access to array element : Object | ClientSuppliedIpUsedInSecurityCheck.cs:25:22:25:34 | call to method GetClientIp : Object | +nodes +| ClientSuppliedIpUsedInSecurityCheck.cs:13:22:13:34 | call to method GetClientIp : Object | semmle.label | call to method GetClientIp : Object | +| ClientSuppliedIpUsedInSecurityCheck.cs:14:18:14:19 | access to local variable ip | semmle.label | access to local variable ip | +| ClientSuppliedIpUsedInSecurityCheck.cs:25:22:25:34 | call to method GetClientIp : Object | semmle.label | call to method GetClientIp : Object | +| ClientSuppliedIpUsedInSecurityCheck.cs:26:18:26:19 | access to local variable ip | semmle.label | access to local variable ip | +| ClientSuppliedIpUsedInSecurityCheck.cs:52:28:52:61 | access to indexer : StringValues | semmle.label | access to indexer : StringValues | +| ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:31 | access to local variable xfHeader : StringValues | semmle.label | access to local variable xfHeader : StringValues | +| ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:42 | call to method ToString : String | semmle.label | call to method ToString : String | +| ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:53 | call to method Split [element] : Object | semmle.label | call to method Split [element] : Object | +| ClientSuppliedIpUsedInSecurityCheck.cs:55:24:55:56 | access to array element : Object | semmle.label | access to array element : Object | +subpaths +#select +| ClientSuppliedIpUsedInSecurityCheck.cs:14:18:14:19 | access to local variable ip | ClientSuppliedIpUsedInSecurityCheck.cs:52:28:52:61 | access to indexer : StringValues | ClientSuppliedIpUsedInSecurityCheck.cs:14:18:14:19 | access to local variable ip | IP address spoofing might include code from $@. | ClientSuppliedIpUsedInSecurityCheck.cs:52:28:52:61 | access to indexer | this user input | +| ClientSuppliedIpUsedInSecurityCheck.cs:26:18:26:19 | access to local variable ip | ClientSuppliedIpUsedInSecurityCheck.cs:52:28:52:61 | access to indexer : StringValues | ClientSuppliedIpUsedInSecurityCheck.cs:26:18:26:19 | access to local variable ip | IP address spoofing might include code from $@. | ClientSuppliedIpUsedInSecurityCheck.cs:52:28:52:61 | access to indexer | this user input | diff --git a/csharp/ql/test/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref b/csharp/ql/test/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref new file mode 100644 index 000000000000..67afef9fabcd --- /dev/null +++ b/csharp/ql/test/experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.qlref @@ -0,0 +1 @@ +experimental/Security Features/CWE-348/ClientSuppliedIpUsedInSecurityCheck.ql diff --git a/csharp/ql/test/experimental/Security Features/CWE-348/options b/csharp/ql/test/experimental/Security Features/CWE-348/options new file mode 100644 index 000000000000..ef338c0b1c12 --- /dev/null +++ b/csharp/ql/test/experimental/Security Features/CWE-348/options @@ -0,0 +1,2 @@ +semmle-extractor-options: /nostdlib /noconfig +semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/_frameworks/Microsoft.AspNetCore.App/Microsoft.AspNetCore.App.csproj