C#: Add query for insecure certificate validation

[intrigus-lgtm]

No description provided.

[ghsecuritylab]

Hello intrigus-lgtm 👋
You have submitted this pull request as a bug bounty report in the github/securitylab repository and therefore this pull request has been put into draft state to give time for the GitHub Security Lab to assess the PR. When GitHub Security Lab has finished assessing your pull request, it will be marked automatically as Ready for review. Until then, please don't change the draft state.

In the meantime, feel free to make changes to the pull request. If you'd like to maximize payout for your this and future submissions, here are a few general guidelines, that we might take into consideration when reviewing a submission.

• the submission models widely-used frameworks/libraries
• the vulnerability modeled in the submission is impactful
• the submission finds new true positive vulnerabilities
• the submission finds very few false positives
• code in the submission is easy to read and will be easy to maintain
• documentation is written clearly, highlighting the impact of the issue it finds and is written without grammatical or other errors. The code samples clearly show the vulnerability
• the submission includes tests, change note etc.

Please note that these are guidelines, not rules. Since we have a lot of different types of submissions, the guidelines might vary for each submission.

Happy hacking!

[intrigus-lgtm]

I would like to reduce FPs by doing something similar to Java:

codeql/java/ql/lib/semmle/code/java/security/UnsafeHostnameVerificationQuery.qll

Lines 80 to 104 in f9ae44c

	/**
	* Flags suggesting a deliberately unsafe `HostnameVerifier` usage.
	*/
	private class UnsafeHostnameVerificationFlag extends FlagKind {
	UnsafeHostnameVerificationFlag() { this = "UnsafeHostnameVerificationFlag" }
	bindingset[result]
	override string getAFlagName() {
	result
	.regexpMatch("(?i).*(secure|disable|selfCert|selfSign|validat|verif|trust|ignore|nocertificatecheck).*") and
	result != "equalsIgnoreCase"
	}
	}
	/** Gets a guard that represents a (likely) flag controlling an unsafe `HostnameVerifier` use. */
	private Guard getAnUnsafeHostnameVerifierFlagGuard() {
	result = any(UnsafeHostnameVerificationFlag flag).getAFlag().asExpr()
	}
	/** Holds if `node` is guarded by a flag that suggests an intentionally insecure use. */
	predicate isNodeGuardedByFlag(DataFlow::Node node) {
	exists(Guard g | g.controls(node.asExpr().getBasicBlock(), _) |
	g = getASecurityFeatureFlagGuard() or g = getAnUnsafeHostnameVerifierFlagGuard()
	)
	}

But as far as I know, there is no equivalent library for C#.

[Kwstubbs]

@intrigus-lgtm Greetings, it seems that this query has many FPs due to two reasons.

1. SSL is often disabled in test and examples, so please add a filter to the query that removes any paths that contain "sample", "example", "demo", "test".
2. Many examples are guarded by an if statement that checks a setting on whether to enable SSL. Ex:

if (!settings.ConnectivitySettings.TlsVerifyCert)
					handler.ServerCertificateCustomValidationCallback = delegate { return true; };

I will ping the codeql C# team and hopefully they can point you in the right direction.
Thanks you.

[Kwstubbs]

@github/codeql-csharp Hi team, this query is ready for review. If you could help intrigus in removing some FPs that would be great.

[Kwstubbs]

@github/codeql-csharp Hi team, we hoping to finish up the last remaining CodeQL bounties so it would be great if we could get some feedback on this PR. Thanks

[michaelnebel]

Thank you for the contribution @intrigus-lgtm and all the reviewing of PRs that you are doing :-)

The PR has not yet been set to "Ready for review". Are you expecting a review of the code as is or just some recommendations?
A couple of clarifying questions:

1. The exceptions mentioned "sample", "example", "demo", "test" (maybe more) are they referring to code that are in file paths that contain these key words?
2. Wrt the false positives. It appears that some guarding logic is needed. In C# there is a Guards module (Guards.qll) with a GuardedExpr class - maybe that will help?

Have you attempted to write this is a data flow query instead of a path problem? (where sources are callables, lambdas etc that returns true and sinks are writes to CertificateValidationProperties)

[Kwstubbs]

Hi @michaelnebel,

The PR has not yet been set to "Ready for review". Are you expecting a review of the code as is or just some recommendations?

We ask bounty submitters (you can see the bounty process here) to keep their PRs in draft so that the CodeQL team doesn't get ahead of our team, whose job is to view the results and assign a score based on the criticality of the issue the query finds. This is one of the last few bounty submissions since we closed the bug bounty program, so future PRs should be business as usual.

The exceptions mentioned "sample", "example", "demo", "test" (maybe more) are they referring to code that are in file paths that contain these key words?

Yup, that what I was hoping to eliminate. Certain settings (like disabling ssl) are very common in test files or example projects, but we don't want to alert on those.

[michaelnebel]

Hi @intrigus-lgtm
I tried to push some changes to this PR, but I don't have write access to the repo.
Instead I opened a PR: #17603. You are more than welcome to cherry pick the commits and apply them to this PR.
The commits does the following:

• Turns the query into a data flow query (which allows us to "track" sources (callables that always returns true) to the possible sinks (the properties in question))
• Adds a sanitizer to the query, which allows you to filter out results that are guarded by a condition.
• Adds some more test examples and base them on .NET Core stubs instead of DLLs.

I have added some comments to #17603 which needs to be addressed after the commits are cherry picked onto this PR.

Once again, thank you very much for looking into this and I apologise for the slow feedback loop and let me know if you any questions whatsoever.

[intrigus-lgtm]

Hi @intrigus-lgtm I tried to push some changes to this PR, but I don't have write access to the repo.

Hmm, that's weird; I have "Allow edits and access to secrets by maintainers" enabled.

Instead I opened a PR: #17603. You are more than welcome to cherry pick the commits and apply them to this PR. The commits does the following:

* Turns the query into a data flow query (which allows us to "track" sources (callables that always returns true) to the possible sinks (the properties in question))

* Adds a sanitizer to the query, which allows you to filter out results that are guarded by a condition.

* Adds some more test examples and base them on .NET Core stubs instead of DLLs.

I have added some comments to #17603 which needs to be addressed after the commits are cherry picked onto this PR.

Once again, thank you very much for looking into this and I apologise for the slow feedback loop and let me know if you any questions whatsoever.

Thanks for adding sanitizers and using stubs. I'm not too familiar with how .NET works so I copied the use of DLLs from other tests^^

[michaelnebel]

@intrigus-lgtm : Great! Do you have enough to continue from here or do you have any questions/concerns?

[intrigus-lgtm]

@intrigus-lgtm : Great! Do you have enough to continue from here or do you have any questions/concerns?

I think I can continue from here, but I'm pretty busy, so expect a slow(er) response.

[kevinbackhouse]

Hi @intrigus-lgtm. Please could you move this PR into ready-for-review? We're wrapping up our bounty program, so we'd like to move this one forward so that we can complete the process as quickly as possible. Thanks!