X Tutup
The Wayback Machine - https://web.archive.org/web/20260303063258/https://github.com/github/codeql/pull/5110
Skip to content

C# : Add query to detect SSRF#5110

Merged
tamasvajk merged 4 commits intomainfrom
unknown repository
Nov 2, 2021
Merged

C# : Add query to detect SSRF#5110
tamasvajk merged 4 commits intomainfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Feb 7, 2021

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?

@ghost ghost self-requested a review as a code owner February 7, 2021 20:01
@github github deleted a comment from johnw545 Feb 8, 2021
@ghost ghost self-requested a review as a code owner February 9, 2021 11:28
@github-actions github-actions bot added the Java label Feb 9, 2021
tamasvajk
tamasvajk suggested changes Feb 19, 2021
View reviewed changes
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

ghost commented Mar 18, 2021

@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 tamasvajk removed the request for review from a team March 19, 2021 08:55
tamasvajk
tamasvajk suggested changes Mar 19, 2021
View reviewed changes
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
ghost commented Mar 22, 2021
View reviewed changes
@aschackmull aschackmull removed the Java label Mar 22, 2021
@ghost ghost mentioned this pull request Mar 22, 2021
Closed
@tamasvajk tamasvajk mentioned this pull request Mar 26, 2021
Merged
tamasvajk
tamasvajk reviewed Mar 29, 2021
View reviewed changes
@ghost
Copy link
Author

ghost commented Apr 20, 2021

@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
Copy link
Contributor

tamasvajk commented Apr 27, 2021

@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
Copy link
Author

ghost commented Apr 27, 2021

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

@tamasvajk
Copy link
Contributor

tamasvajk commented Apr 27, 2021

@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
Copy link
Author

ghost commented Apr 27, 2021 •
edited by ghost
Loading

@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
Copy link
Author

ghost commented May 19, 2021

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

tamasvajk
tamasvajk suggested changes May 25, 2021
View reviewed changes
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ghost
Copy link
Author

ghost commented May 26, 2021

@tamasvajk Changes done!

@tamasvajk
Copy link
Contributor

tamasvajk commented Jun 7, 2021

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

hvitved
hvitved requested changes Jun 16, 2021
View reviewed changes
@ghost
Copy link
Author

ghost commented Jul 19, 2021

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

@hvitved
Copy link
Contributor

hvitved commented Aug 2, 2021

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
Copy link
Author

ghost commented Sep 19, 2021

@tamasvajk Changes done! PTAL.

@tamasvajk
Copy link
Contributor

tamasvajk commented Sep 24, 2021

@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 ghost dismissed a stale review via b9c0816 September 28, 2021 22:45
@ghost
Copy link
Author

ghost commented Sep 28, 2021

@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
Copy link
Contributor

hvitved commented Oct 1, 2021

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
Copy link
Author

ghost commented Oct 1, 2021 •
edited by ghost
Loading

@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.

tamasvajk
tamasvajk suggested changes Oct 8, 2021
View reviewed changes
@ghost
Copy link
Author

ghost commented Oct 21, 2021

@tamasvajk Changes done!

@ghost
Copy link
Author

ghost commented Oct 29, 2021

@tamasvajk Any updates here?

tamasvajk
tamasvajk approved these changes Oct 29, 2021
View reviewed changes
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@porcupineyhairs Thank you for this contribution.

@tamasvajk tamasvajk merged commit 18b0806 into github:main Nov 2, 2021
@ghost ghost deleted the ssrfCsharp branch November 2, 2021 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@hvitved hvitved

@tamasvajk tamasvajk

Labels

C# documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tamasvajk @hvitved @aschackmull
X Tutup