diff --git a/csharp/ql/src/experimental/CWE-918/RequestForgery.cs b/csharp/ql/src/experimental/CWE-918/RequestForgery.cs new file mode 100644 index 000000000000..d9b83eeea3ce --- /dev/null +++ b/csharp/ql/src/experimental/CWE-918/RequestForgery.cs @@ -0,0 +1,33 @@ +namespace RequestForgery.Controllers +{ + public class SSRFController : Controller + { + [HttpPost] + [ValidateAntiForgeryToken] + public async Task Bad(string url) + { + var request = new HttpRequestMessage(HttpMethod.Get, url); + + var client = new HttpClient(); + await client.SendAsync(request); + + return View(); + } + + [HttpPost] + [ValidateAntiForgeryToken] + public async Task Good(string url) + { + string baseUrl = "www.mysecuresite.com/"; + if (url.StartsWith(baseUrl)) + { + var request = new HttpRequestMessage(HttpMethod.Get, url); + var client = new HttpClient(); + await client.SendAsync(request); + + } + + return View(); + } + } +} \ No newline at end of file diff --git a/csharp/ql/src/experimental/CWE-918/RequestForgery.qhelp b/csharp/ql/src/experimental/CWE-918/RequestForgery.qhelp new file mode 100644 index 000000000000..fb04540e13da --- /dev/null +++ b/csharp/ql/src/experimental/CWE-918/RequestForgery.qhelp @@ -0,0 +1,35 @@ + + + + + +

Directly incorporating user input into a HTTP request without validating the input +can facilitate Server Side Request Forgery (SSRF) attacks. In these attacks, the server +may be tricked into making a request and interacting with an attacker-controlled server. +

+ +
+ + +

To guard against SSRF attacks, it is advisable to avoid putting user input +directly into the request URL. Instead, maintain a list of authorized +URLs on the server; then choose from that list based on the user input provided.

+ +
+ + +

The following example shows an HTTP request parameter being used directly in a forming a +new request without validating the input, which facilitates SSRF attacks. +It also shows how to remedy the problem by validating the user input against a known fixed string. +

+ + + +
+ +
  • + OWASP SSRF +
  • + +
    +
    diff --git a/csharp/ql/src/experimental/CWE-918/RequestForgery.ql b/csharp/ql/src/experimental/CWE-918/RequestForgery.ql new file mode 100644 index 000000000000..8b2025c4ce2a --- /dev/null +++ b/csharp/ql/src/experimental/CWE-918/RequestForgery.ql @@ -0,0 +1,19 @@ +/** + * @name Uncontrolled data used in network request + * @description Sending network requests with user-controlled data allows for request forgery attacks. + * @kind path-problem + * @problem.severity error + * @precision high + * @id cs/request-forgery + * @tags security + * external/cwe/cwe-918 + */ + +import csharp +import RequestForgery::RequestForgery +import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph + +from RequestForgeryConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink +where c.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "$@ flows to here and is used in a server side web request.", + source.getNode(), "User-provided value" diff --git a/csharp/ql/src/experimental/CWE-918/RequestForgery.qll b/csharp/ql/src/experimental/CWE-918/RequestForgery.qll new file mode 100644 index 000000000000..0f73e0f3d8b6 --- /dev/null +++ b/csharp/ql/src/experimental/CWE-918/RequestForgery.qll @@ -0,0 +1,234 @@ +import csharp + +module RequestForgery { + import semmle.code.csharp.controlflow.Guards + import semmle.code.csharp.frameworks.System + import semmle.code.csharp.frameworks.system.Web + import semmle.code.csharp.frameworks.Format + import semmle.code.csharp.security.dataflow.flowsources.Remote + + /** + * A data flow source for server side request forgery vulnerabilities. + */ + abstract private class Source extends DataFlow::Node { } + + /** + * A data flow sink for server side request forgery vulnerabilities. + */ + abstract private class Sink extends DataFlow::ExprNode { } + + /** + * A data flow BarrierGuard which blocks the flow of taint for + * server side request forgery vulnerabilities. + */ + abstract private class BarrierGuard extends DataFlow::BarrierGuard { } + + /** + * A data flow configuration for detecting server side request forgery vulnerabilities. + */ + class RequestForgeryConfiguration extends DataFlow::Configuration { + RequestForgeryConfiguration() { this = "Server Side Request forgery" } + + override predicate isSource(DataFlow::Node source) { source instanceof Source } + + override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + override predicate isAdditionalFlowStep(DataFlow::Node prev, DataFlow::Node succ) { + interpolatedStringFlowStep(prev, succ) + or + stringReplaceStep(prev, succ) + or + uriCreationStep(prev, succ) + or + formatConvertStep(prev, succ) + or + toStringStep(prev, succ) + or + stringConcatStep(prev, succ) + or + stringFormatStep(prev, succ) + or + pathCombineStep(prev, succ) + } + + override predicate isBarrierGuard(DataFlow::BarrierGuard guard) { + guard instanceof BarrierGuard + } + } + + /** + * A remote data flow source taken as a source + * for Server Side Request Forgery(SSRF) Vulnerabilities. + */ + private class RemoteFlowSourceAsSource extends Source { + RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource } + } + + /** + * An url argument to a `HttpRequestMessage` constructor call + * taken as a sink for Server Side Request Forgery(SSRF) Vulnerabilities. + */ + private class SystemWebHttpRequestMessageSink extends Sink { + SystemWebHttpRequestMessageSink() { + exists(Class c | c.hasQualifiedName("System.Net.Http.HttpRequestMessage") | + c.getAConstructor().getACall().getArgument(1) = this.asExpr() + ) + } + } + + /** + * An argument to a `WebRequest.Create` call taken as a + * sink for Server Side Request Forgery(SSRF) Vulnerabilities. * + */ + private class SystemNetWebRequestCreateSink extends Sink { + SystemNetWebRequestCreateSink() { + exists(Method m | + m.getDeclaringType().hasQualifiedName("System.Net.WebRequest") and m.hasName("Create") + | + m.getACall().getArgument(0) = this.asExpr() + ) + } + } + + /** + * An argument to a new HTTP Request call of a `System.Net.Http.HttpClient` object + * taken as a sink for Server Side Request Forgery(SSRF) Vulnerabilities. + */ + private class SystemNetHttpClientSink extends Sink { + SystemNetHttpClientSink() { + exists(Method m | + m.getDeclaringType().hasQualifiedName("System.Net.Http.HttpClient") and + m.hasName([ + "DeleteAsync", "GetAsync", "GetByteArrayAsync", "GetStreamAsync", "GetStringAsync", + "PatchAsync", "PostAsync", "PutAsync" + ]) + | + m.getACall().getArgument(0) = this.asExpr() + ) + } + } + + /** + * An url argument to a method call of a `System.Net.WebClient` object + * taken as a sink for Server Side Request Forgery(SSRF) Vulnerabilities. + */ + private class SystemNetClientBaseAddressSink extends Sink { + SystemNetClientBaseAddressSink() { + exists(Property p | + p.hasName("BaseAddress") and + p.getDeclaringType() + .hasQualifiedName(["System.Net.WebClient", "System.Net.Http.HttpClient"]) + | + p.getAnAssignedValue() = this.asExpr() + ) + } + } + + /** + * A method call which checks the base of the tainted uri is assumed + * to be a guard for Server Side Request Forgery(SSRF) Vulnerabilities. + * This guard considers all checks as valid. + */ + private class BaseUriGuard extends BarrierGuard, MethodCall { + BaseUriGuard() { this.getTarget().hasQualifiedName("System.Uri", "IsBaseOf") } + + override predicate checks(Expr e, AbstractValue v) { + // we consider any checks against the tainted value to sainitize the taint. + // This implies any check such as shown below block the taint flow. + // Uri url = new Uri("whitelist.com") + // if (url.isBaseOf(`taint1)) + (e = this.getArgument(0) or e = this.getQualifier()) and + v.(AbstractValues::BooleanValue).getValue() = true + } + } + + /** + * A method call which checks if the Uri starts with a white-listed string is assumed + * to be a guard for Server Side Request Forgery(SSRF) Vulnerabilities. + * This guard considers all checks as valid. + */ + private class StringStartsWithBarrierGuard extends BarrierGuard, MethodCall { + StringStartsWithBarrierGuard() { + this.getTarget().hasQualifiedName("System.String", "StartsWith") + } + + override predicate checks(Expr e, AbstractValue v) { + // Any check such as the ones shown below + // "https://myurl.com/".startsWith(`taint`) + // `taint`.startsWith("https://myurl.com/") + // are assumed to sainitize the taint + (e = this.getQualifier() or this.getArgument(0) = e) and + v.(AbstractValues::BooleanValue).getValue() = true + } + } + + private predicate stringFormatStep(DataFlow::Node prev, DataFlow::Node succ) { + exists(FormatCall c | c.getArgument(0) = prev.asExpr() and c = succ.asExpr()) + } + + private predicate pathCombineStep(DataFlow::Node prev, DataFlow::Node succ) { + exists(MethodCall combineCall | + combineCall.getTarget().hasQualifiedName("System.IO.Path", "Combine") and + combineCall.getArgument(0) = prev.asExpr() and + combineCall = succ.asExpr() + ) + } + + private predicate uriCreationStep(DataFlow::Node prev, DataFlow::Node succ) { + exists(ObjectCreation oc | + oc.getTarget().getDeclaringType().hasQualifiedName("System.Uri") and + oc.getArgument(0) = prev.asExpr() and + oc = succ.asExpr() + ) + } + + private predicate interpolatedStringFlowStep(DataFlow::Node prev, DataFlow::Node succ) { + exists(InterpolatedStringExpr i | + // allow `$"http://{`taint`}/blabla/");"` or + // allow `$"https://{`taint`}/blabla/");"` + i.getText(0).getValue().matches(["http://", "http://"]) and + i.getInsert(1) = prev.asExpr() and + succ.asExpr() = i + or + // allow `$"{`taint`}/blabla/");"` + i.getInsert(0) = prev.asExpr() and + succ.asExpr() = i + ) + } + + private predicate stringReplaceStep(DataFlow::Node prev, DataFlow::Node succ) { + exists(MethodCall mc, SystemStringClass s | + mc = s.getReplaceMethod().getACall() and + mc.getQualifier() = prev.asExpr() and + succ.asExpr() = mc + ) + } + + private predicate stringConcatStep(DataFlow::Node prev, DataFlow::Node succ) { + exists(AddExpr a | + a.getLeftOperand() = prev.asExpr() + or + a.getRightOperand() = prev.asExpr() and + a.getLeftOperand().(StringLiteral).getValue() = ["http://", "https://"] + | + a = succ.asExpr() + ) + } + + private predicate formatConvertStep(DataFlow::Node prev, DataFlow::Node succ) { + exists(Method m | + m.hasQualifiedName("System.Convert", + ["FromBase64String", "FromHexString", "FromBase64CharArray"]) and + m.getParameter(0) = prev.asParameter() and + succ.asExpr() = m.getACall() + ) + } + + private predicate toStringStep(DataFlow::Node prev, DataFlow::Node succ) { + exists(MethodCall ma | + ma.getTarget().hasName("ToString") and + ma.getQualifier() = prev.asExpr() and + succ.asExpr() = ma + ) + } +} diff --git a/csharp/ql/test/experimental/CWE-918/RequestForgery.cs b/csharp/ql/test/experimental/CWE-918/RequestForgery.cs new file mode 100644 index 000000000000..32347505b352 --- /dev/null +++ b/csharp/ql/test/experimental/CWE-918/RequestForgery.cs @@ -0,0 +1,58 @@ +// semmle-extractor-options: ${testdir}/../../resources/stubs/System.Web.cs /r:System.Threading.Tasks.dll /r:System.Collections.Specialized.dll /r:System.Runtime.dll /r:System.Private.Uri.dll + +using System; +using System.Threading.Tasks; +using System.Web.Mvc; +using System.Net.Http; + +namespace RequestForgery.Controllers +{ + public class SSRFController : Controller + { + [HttpPost] + [ValidateAntiForgeryToken] + public async Task Bad(string url) + { + var request = new HttpRequestMessage(HttpMethod.Get, url); + + var client = new HttpClient(); + await client.SendAsync(request); + + return View(); + } + + [HttpPost] + [ValidateAntiForgeryToken] + public async Task Good(string url) + { + string baseUrl = "www.mysecuresite.com/"; + if (url.StartsWith(baseUrl)) + { + var request = new HttpRequestMessage(HttpMethod.Get, url); + var client = new HttpClient(); + await client.SendAsync(request); + + } + + return View(); + } + } +} +// Missing stubs: +namespace System.Net.Http +{ + public class HttpClient + { + public async Task SendAsync(HttpRequestMessage request) => throw null; + } + + public class HttpRequestMessage + { + public HttpRequestMessage(HttpMethod method, string requestUri) => throw null; + } + + public class HttpMethod + { + public static readonly HttpMethod Get; + } +} diff --git a/csharp/ql/test/experimental/CWE-918/RequestForgery.expected b/csharp/ql/test/experimental/CWE-918/RequestForgery.expected new file mode 100644 index 000000000000..70ba2594fdff --- /dev/null +++ b/csharp/ql/test/experimental/CWE-918/RequestForgery.expected @@ -0,0 +1,8 @@ +edges +| RequestForgery.cs:14:52:14:54 | url : String | RequestForgery.cs:16:66:16:68 | access to parameter url | +nodes +| RequestForgery.cs:14:52:14:54 | url : String | semmle.label | url : String | +| RequestForgery.cs:16:66:16:68 | access to parameter url | semmle.label | access to parameter url | +subpaths +#select +| RequestForgery.cs:16:66:16:68 | access to parameter url | RequestForgery.cs:14:52:14:54 | url : String | RequestForgery.cs:16:66:16:68 | access to parameter url | $@ flows to here and is used in a server side web request. | RequestForgery.cs:14:52:14:54 | url | User-provided value | diff --git a/csharp/ql/test/experimental/CWE-918/RequestForgery.qlref b/csharp/ql/test/experimental/CWE-918/RequestForgery.qlref new file mode 100644 index 000000000000..3d529ae5a2ca --- /dev/null +++ b/csharp/ql/test/experimental/CWE-918/RequestForgery.qlref @@ -0,0 +1 @@ +experimental/CWE-918/RequestForgery.ql \ No newline at end of file