Polish CWE-322: detect and exclude cases where host-checking is optional by smowton · Pull Request #261 · github/codeql-go

smowton · 2020-07-17T15:27:25Z

No description provided.

owen-mc

All looks good, except CI failing

intrigus-lgtm

There are still some minor things for which I'll add suggestions soon.

intrigus-lgtm · 2020-07-20T12:37:05Z

ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql

 }

+/**
+ * Identifies HostKeyCallbackFunc instances that reach CoientConfig.HostKeyCallback fields.


Suggested change

* Identifies HostKeyCallbackFunc instances that reach CoientConfig.HostKeyCallback fields.

* Identifies `HostKeyCallbackFunc` instances that reach `ClientConfig.HostKeyCallback` fields.

Ha, I vaguely remember thinking "Coient - what a weird word" as I read that. I didn't realise it was a typo! 😆

intrigus-lgtm · 2020-07-20T12:41:46Z

ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql

 import go
 import DataFlow::PathGraph

+/** ssh.InsecureIgnoreHostKey, which allows connecting to any host regardless of its host key. */


Suggested change

/** ssh.InsecureIgnoreHostKey, which allows connecting to any host regardless of its host key. */

/** The `ssh.InsecureIgnoreHostKey` function, which allows connecting to any host regardless of its host key. */

max-schaefer

One minor nit, otherwise LGTM. Is it intentional that this is still in experimental? If we want to move it out of there, we'll need to evaluate performance.

max-schaefer · 2020-07-28T08:01:32Z

ql/src/experimental/CWE-322/InsecureHostKeyCallback.ql

 }

+/**
+ * Identifies HostKeyCallbackFunc instances that reach `ClientConfig.HostKeyCallback` fields.


Suggested change

* Identifies HostKeyCallbackFunc instances that reach `ClientConfig.HostKeyCallback` fields.

* A data-flow configuration for identifying `HostKeyCallbackFunc` instances that reach `ClientConfig.HostKeyCallback` fields.

smowton · 2020-07-28T14:45:17Z

@max-schaefer amended and promoted out of experimental status. Also did the same to CWE-327.

max-schaefer

A few more minor things. I can't quite tell from the diff whether you added a test for the newly added logic to detect optional host checks?

Once the performance evaluation is in, this should be good to merge.

max-schaefer · 2020-07-29T06:35:17Z

ql/src/Security/CWE-322/InsecureHostKeyCallback.ql

+ * @name Use of insecure HostKeyCallback implementation
+ * @description Detects insecure SSL client configurations with an implementation of the `HostKeyCallback` that accepts all host keys.
+ * @kind path-problem
+ * @problem.severity error


Needs a @precision now.

max-schaefer · 2020-07-29T06:36:42Z

ql/src/Security/CWE-322/InsecureHostKeyCallback.ql

+    // Either a call to InsecureIgnoreHostKey(), which we know returns an insecure callback.
+    this = any(InsecureIgnoreHostKey f).getACall()
+    or
+    // Or a callback function in the source code  (named or anonymous) that always returns nil.


Suggested change

// Or a callback function in the source code (named or anonymous) that always returns nil.

// Or a callback function in the source code (named or anonymous) that always returns nil.

ql/src/Security/CWE-322/InsecureHostKeyCallback.ql

smowton · 2020-07-29T13:27:44Z

@max-schaefer all suggestions applied, and re: the test it already covered one case but not another, which I've added. It also turned out I'd partly broken the test without noticing :/ the results are now as expected, and I'm rerunning the LGTM tests to check if that's had any unexpected impact.

smowton · 2020-07-29T13:42:06Z

Btw if you look at the diff commit-by-commit you get a much nicer diff than the whole thing piled together

…to include calls with multiple return types For example, https://godoc.org/golang.org/x/crypto/ssh/knownhosts#New returns a host-key checker and an error value, and we previously didn't consider the first return value a candidate checker function.

This produces a secure host-key checker; we assume by default that an opaque function not otherwise specified returns an acceptable checker, but we need to particularly cope with its multiple return values to handle this factory function.

ql/test/query-tests/Security/CWE-322/vendor/golang.org/x/crypto/ssh/knownhosts/stub.go

smowton requested a review from a team July 17, 2020 15:27

owen-mc reviewed Jul 20, 2020

View reviewed changes

intrigus-lgtm reviewed Jul 20, 2020

View reviewed changes

smowton force-pushed the smowton/admin/cleanup-cwe-322 branch 2 times, most recently from a13a1f7 to 5101933 Compare July 20, 2020 15:45

owen-mc approved these changes Jul 21, 2020

View reviewed changes

max-schaefer suggested changes Jul 28, 2020

View reviewed changes

smowton force-pushed the smowton/admin/cleanup-cwe-322 branch from 5101933 to 1981e86 Compare July 28, 2020 14:44

max-schaefer suggested changes Jul 29, 2020

View reviewed changes

smowton force-pushed the smowton/admin/cleanup-cwe-322 branch from 1981e86 to 0a6239f Compare July 29, 2020 13:23

smowton force-pushed the smowton/admin/cleanup-cwe-322 branch from f611221 to 3fabd2e Compare July 29, 2020 13:40

smowton added 2 commits July 29, 2020 14:43

Polish CWE-322: detect and exclude cases where host-checking is optional

99f0875

Promote CWE-322 out of experimental status

f162a5b

smowton force-pushed the smowton/admin/cleanup-cwe-322 branch from 3fabd2e to f162a5b Compare July 29, 2020 13:43

smowton added 2 commits July 29, 2020 16:06

max-schaefer reviewed Jul 30, 2020

View reviewed changes

ql/test/query-tests/Security/CWE-322/vendor/golang.org/x/crypto/ssh/knownhosts/stub.go Show resolved Hide resolved

max-schaefer approved these changes Jul 30, 2020

View reviewed changes

max-schaefer merged commit 2134757 into github:master Jul 30, 2020

Feb	MAR	Apr
	05
2025	2026	2027

	* Identifies HostKeyCallbackFunc instances that reach CoientConfig.HostKeyCallback fields.
	* Identifies `HostKeyCallbackFunc` instances that reach `ClientConfig.HostKeyCallback` fields.

	/** ssh.InsecureIgnoreHostKey, which allows connecting to any host regardless of its host key. */
	/** The `ssh.InsecureIgnoreHostKey` function, which allows connecting to any host regardless of its host key. */

	* Identifies HostKeyCallbackFunc instances that reach `ClientConfig.HostKeyCallback` fields.
	* A data-flow configuration for identifying `HostKeyCallbackFunc` instances that reach `ClientConfig.HostKeyCallback` fields.

	// Or a callback function in the source code (named or anonymous) that always returns nil.
	// Or a callback function in the source code (named or anonymous) that always returns nil.

Conversation

smowton commented Jul 17, 2020

Uh oh!

owen-mc left a comment

Choose a reason for hiding this comment

Uh oh!

intrigus-lgtm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

intrigus-lgtm Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

owen-mc Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

intrigus-lgtm Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

max-schaefer left a comment

Choose a reason for hiding this comment

Uh oh!

max-schaefer Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

smowton commented Jul 28, 2020

Uh oh!

max-schaefer left a comment

Choose a reason for hiding this comment

Uh oh!

max-schaefer Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

max-schaefer Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smowton commented Jul 29, 2020

Uh oh!

smowton commented Jul 29, 2020

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

intrigus-lgtm left a comment •

edited

Loading