C# : Add query to detect SSRF

[ghost]

This query adds support for detecting Server Side Request Forgery(SSRF) vulnerabilities in C#. This has at least one valid detection in chhans/Kurs19_Public.

I am working on adding a few more sinks. I am new to C#. Can someone tell me which libraries would be good to include here?

[tamasvajk]

Thank you for implementing this check. I added some comments inline.
Also, in general, all HTTP request making APIs seem vulnerable to this attack. I think the most common ones are System.Net.Http.HttpClient, System.Net.HttpWebRequest ( WebRequest.Create()), and System.Net.WebClient; and maybe handling the base classes would also be good, such as System.Net.Http.HttpMessageInvoker. Finally, HttpClient and WebClient allow setting the BaseAddress property, so flow into that should also be covered.

[ghost]

@tamasvajk I have added new sinks for the methods we discussed above. I have also squashed and rebased as the main had diverged quite a bit.

[tamasvajk]

I think the check got a lot better, and could provide value. Here's a real use case that's found by it. However at the moment it's quite noisy:

• Most of the HTTP clients accept relative URLs too, and when data flows into a relative URL that should be fine. Maybe an option to overcome this would be to check if there's any assignment to the base address, such as here.
• There are some cases when data flows into the sinks through string concatenation, and not in the base address part of the string: https://lgtm.com/query/1044480058280809211/ or string interpolation.
• The implemented sanitizer is quite specific, there could be other ways to filter too. Let's see if the LGTM analysis shows any other recurring patterns.

[ghost]

@tamasvajk Sorry for the delay. I tried adding that. But now I get the following error message. Can you please help me figure this out? I have rebased this pr to the latest main and added the wip changes temporarily to help debugging.

[24399] [ERROR]   Unable to resolve reference 'HtmlAgilityPack.dll'
[24399] [ERROR]   Unable to resolve reference 'Microsoft.Extensions.Configuration.dll'
[24399] [ERROR]   Unable to resolve reference 'Microsoft.Extensions.Logging.dll'
[24399] [ERROR]   Unable to resolve reference 'Microsoft.AspNetCore.Mvc.dll'
[24399] [ERROR]   Compilation error: /home/ydj/work/codeql/csharp/ql/test/experimental/CWE-918/stubs.cs(50,35): error CS0535: 'HtmlNodeCollection' does not implement interface member 'IEnumerable.GetEnumerator()'
[24399] [ERROR]   Compilation error: /home/ydj/work/codeql/csharp/ql/test/experimental/CWE-918/stubs.cs(773,32): error CS0737: 'GroupCollection' does not implement interface member 'IList.this[int]'. 'GroupCollection.this[int]' cannot implement an interface member because it is not public.
[24399] [ERROR]   Compilation error: /home/ydj/work/codeql/csharp/ql/test/experimental/CWE-918/stubs.cs(773,122): error CS0737: 'GroupCollection' does not implement interface member 'IReadOnlyList<Group>.this[int]'. 'GroupCollection.this[int]' cannot implement an interface member because it is not public.
[24399] [ERROR]   Compilation error: /home/ydj/work/codeql/csharp/ql/test/experimental/CWE-918/stubs.cs(773,515): error CS0737: 'GroupCollection' does not implement interface member 'IList<Group>.this[int]'. 'GroupCollection.this[int]' cannot implement an interface member because it is not public.
[24399] [ERROR]   Compilation error: /home/ydj/work/codeql/csharp/ql/test/experimental/CWE-918/stubs.cs(785,12): error CS0111: Type 'GroupCollection' already defines a member called 'this' with the same parameter types

[tamasvajk]

@porcupineyhairs There are multiple issues here:

• Unable to resolve reference 'HtmlAgilityPack.dll' and similar issues are caused by the fact that your test is referencing these DLLs, but they can't be found, because the path is not correctly specified. Prepending ../../resources/assemblies/ solves these.
• The errors in the stubs.cs are compilation errors. The stub generator seems to have failed on generating a correct stub. In the last days I have worked on the stub generator quite a bit, see here: C#: Stub generation #5664. However, the generator still fails to stub the complete asp.net core public API surface. You could give it a try though, the csharp/ql/src/Stubs/make_stubs.py only generates the minimal stub, and I might have fixed the bits that are required for your test case. Another option would be to manually stub the required methods. The latter also shouldn't be too painful, because the main bits of asp.net (not core) are stubbed already, and you could simplify your test file to remove unneeded classes/interfaces, such as ILogger<HomeController>, IConfiguration. You also don't need IHttpClientFactory as you're only using the returned HttpClient, which could be manually stubbed with a single SendAsync method.

In fact, I gave the latter a quick try, and simplified your test to the below:

// semmle-extractor-options: ${testdir}/../../resources/stubs/System.Web.cs /r:System.Threading.Tasks.dll /r:System.Collections.Specialized.dll

using System;
using System.Threading.Tasks;
using System.Web.Mvc;
using System.Net.Http;

namespace SsrfChallenge.Controllers
{
    public class HomeController : Controller
    {
        [HttpPost]
        public async Task<ActionResult> Bad(string url)
        {
            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;
    }
}

[ghost]

@tamasvajk Thanks a lot. The tests work fine now. I have pushed the new changes. PTAL.

[tamasvajk]

@tamasvajk Thanks a lot. The tests work fine now. I have pushed the new changes. PTAL.

@porcupineyhairs I removed a test case above for simplicity. Could you please add the case with the sanitizer? Also, I'm unsure if you've already seen/covered the comments in #5110 (review).

[ghost]

@tamasvajk I have made the changes now. I have also started a new run on the 61 projects which threw an alert in the earlier run. There are 12 independent projects. All of them appear to be TP. PTAL.

[ghost]

@tamasvajk I have squashed and rebased to the latest main while this is pending review.

[tamasvajk]

I've restarted the LGTM analysis: https://lgtm.com/query/5526907158523374183/

[ghost]

@tamasvajk Changes done!

[tamasvajk]

@hvitved Could you please also have a final look at this PR?

[ghost]

@hvitved Sorry for the delay is responding to your comments. I have made some changes now. PTAL.

[hvitved]

The tests are failing:

ERROR: Could not resolve type Barrier (/home/runner/work/semmle-code/semmle-code/ql/csharp/ql/src/experimental/CWE-918/RequestForgery.qll:58,79-86)

[ghost]

@tamasvajk Changes done! PTAL.

[tamasvajk]

@porcupineyhairs I think you might have forgotten to push your changes. The last commit in this PR is from July, and since then @hvitved also provided some feedback. Could you please double check?

[ghost]

@tamasvajk @hvitved Yes, indeed, that was the case, I didn't push the changes here properly. I have rebased to the latest main and forced pushed the changes now. PTAL.

[hvitved]

The RequestForgery.qlref test is still failing:

--- expected
+++ actual
@@ -3,5 +3,6 @@
 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 method of WebClient. | RequestForgery.cs:14:52:14:54 | url | User-provided value |

[ghost]

@hvitved It seems like the subpath section in the output was added recently to CodeQL. My CLI version was from earlier and hence, it didn't include it. I have added the new section and pushed the changes again. I can confirm the test run clear on my local node now.

[ghost]

@tamasvajk Changes done!

[ghost]

@tamasvajk Any updates here?

[tamasvajk]

@porcupineyhairs Thank you for this contribution.