[Java] CWE-348: Using a client-supplied IP address in a security check #5631

haby0 · 2021-04-08T09:15:51Z

If an application trusts an HTTP request header like X-Forwarded-For to accurately specify the remote IP address of the connecting client.

intrigus-lgtm · 2021-04-08T09:34:28Z

java/ql/test/stubs/springframework-5.2.3/org/springframework/stereotype/Controller.java

 @Documented
-@Component


What happened to @Component?
https://github.com/spring-projects/spring-framework/blob/v5.2.3.RELEASE/spring-context/src/main/java/org/springframework/stereotype/Controller.java

Thanks for reminding

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp

smowton · 2021-04-08T11:00:04Z

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll

+/** A data flow sink of method return or log output or local print. */
+class UseOfLessTrustedSink extends DataFlow::Node {
+  UseOfLessTrustedSink() {
+    exists(ReturnStmt rs | rs.getResult() = this.asExpr())


If we're treating returning as a sink we might as well flag any use of X-Forwarded-For except splitting and taking the rightmost value.

The result of return as a sink, I think the method is a util method, this usage is common.

Right, but you don't care about what is done with it subsequently? This is so general that there's almost no point checking how it is used at all.

If so, I think there is a problem as long as there is a place to call this method. If you consider what to do with the result of the client ip method, I think there will be a false negative rate. What do you think?

What uses do you actually consider dangerous, besides logging?

In addition to local output or log records, there are also user authentication based on client ip, database storage, etc., but their use is not unique, so I think it is impossible to standardize the definition.

Can we let the security-lab review it together?

E.g:

The client ip detects whether there is login permission: UserServiceImpl

Limit the number of logins for the client ip, which is common in scenarios such as blasting and ticketing: CaptchaService

Insert the database as a record: LogDB

Save it in JavaBean and insert into the database: MenuController

I can, but as this stands I would have to warn them that the query will produce really strange results:

Directly used in logging call: positive

Directly used in a rate-limiter: negative

Directly returned: positive

Returned in a field: negative

A user of this query would be very confused, because their expectation is that the way the value is used is the important thing, not trivial aspects of dataflow like is it returned from a function or used in function-local scope.

I would recommend you find some e.g. rate-limiting utility that would constitute a dangerous way to use this identifier. Alternatively you can choose to send this to the security lab with the health warning attached.

Thank you, I agree to let them know the current situation, and I also want to listen to their suggestions, so as to implement a changed query.

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll

smowton · 2021-04-08T11:01:32Z

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll

+        .getASupertype*()
+        .hasQualifiedName("javax.servlet", "ServletRequest") and
+    this.getArgument(0).toString().toLowerCase() in [
+        "\"wl-proxy-client-ip\"", "\"proxy-client-ip\"", "\"http_client_ip\"",


Perhaps provide a link regarding how you chose this list of header values?

Reference:

github

memorynotfound

smowton · 2021-04-08T11:03:47Z

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll

+}
+
+/** The first one in the method to get the ip value through `x-forwarded-for`. */
+predicate xffIsFirstGet(Node node) {


Explain what you're intending to do here? Should this suppress some warnings completely, or just prevent repeated warnings in the same function?

Ensure that x-forwarded-for is the first to be obtained in the method of obtaining client ip.

smowton · 2021-04-08T11:04:54Z

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll

+        .hasQualifiedName("javax.servlet", "ServletRequest") and
+    this.getArgument(0).toString().toLowerCase() in [
+        "\"wl-proxy-client-ip\"", "\"proxy-client-ip\"", "\"http_client_ip\"",
+        "\"http_x_forwarded_for\"", "\"x-real-ip\""


Explain why you're using these headers for xffIsFirstGet but only X-Forwarded-For in the main query?

This is related to the xffIsFirstGet predicate.

Right, but why are you checking for all these possibilities here, but only X-Forwarded-For in UseOfLessTrustedSource? What is special about that header that means you trust it less than X-Real-IP for instance?

Yes, and X-Forwarded-For has a high usage rate.

Why are you only considering X-Forwarded-For a source of error? Isn't X-Real-IP exactly the same? I don't understand why you consider all 5 for the purposes of finding out if this is the first check in this function, but only one for the source.

X-Forwarded-For is a superposition process when used, format: client, proxy1, proxy2. X-Real-IP is a process of coverage when in use.

Right, which is why you suppose that taking the right-most comma-separated piece of X-Forwarded-For might be safer than the left-most. If X-Real-IP doesn't get used that way then it's simply always dangerous to trust, right? But at the moment we treat it as always safe?

Thank you for reviewing my pr.

You are right. But I still find it weird, regarding the questions here and above, I would like to hear the opinions of security-lab. Hope you can understand.

…SourceLib.qll Co-authored-by: Chris Smowton <smowton@github.com>

…Source.qhelp Co-authored-by: Chris Smowton <smowton@github.com>

smowton · 2021-04-13T14:41:50Z

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql

+    exists(ArrayAccess aa, MethodAccess ma | aa.getArray() = ma |
+      ma.getQualifier() = node.asExpr() and
+      ma.getMethod() instanceof SplitMethod and
+      not aa.getIndexExpr().toString() = "0"


Suggested change

not aa.getIndexExpr().toString() = "0"

not aa.getIndexExpr().(CompileTimeConstantExpr).getIntValue() = 0

smowton · 2021-04-13T14:47:45Z

After discussing with colleages, I have decided I will reject this PR unless you can replace the highly-general ReturnValue sink with something more precise which you believe represents a hazardous use of an untrustworthy header. Otherwise this query is not much better than flagging any getHeader("X-Forwarded-For"), which can be achieved with much simpler tools than CodeQL.

haby0 · 2021-04-13T15:22:39Z

After discussing with colleages, I have decided I will reject this PR unless you can replace the highly-general ReturnValue sink with something more precise which you believe represents a hazardous use of an untrustworthy header. Otherwise this query is not much better than flagging any getHeader("X-Forwarded-For"), which can be achieved with much simpler tools than CodeQL.

I will try to find the accurate sink from github. In addition, I want to know if the source needs to be changed here?

smowton · 2021-04-14T12:15:18Z

I would strongly advise to use the same list of headers that specify a remote endpoint identifier (X-Forwarded-For, X-Real-IP, ...) as you used elsewhere in this PR. X-Forwarded-For only differs in that it is sometimes possible to comma-split it and so retrieve a safe(r) version of it.

haby0 · 2021-04-15T02:30:35Z

I would strongly advise to use the same list of headers that specify a remote endpoint identifier (X-Forwarded-For, X-Real-IP, ...) as you used elsewhere in this PR. X-Forwarded-For only differs in that it is sometimes possible to comma-split it and so retrieve a safe(r) version of it.

Thank you for your suggestion, I will reflect it in the next version.

haby0 · 2021-04-20T03:21:58Z

There are several changes in this revision:

Modify source, add remote endpoint identifier.
Modify sink. Including: log operation, if condition (excluding the null value judgment of client ip), sql operation (using QueryInjectionSink directly), local output.

lgtm result for reference.

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql

smowton · 2021-04-20T11:10:55Z

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll

+ * `if (remoteAddr == null || "".equals(remoteAddr)) {...` judging whether the client ip is a null value,
+ * it needs to be excluded
+ */
+private class IfConditionSink extends UseOfLessTrustedSink {


This is still a very very broad sink. I would much rather we looked for specific patterns we think are dangerous (for example, the startsWith condition you describe in the comment than to just exclude 7 or 8 cases you think are probably FPs and flag every other if statement.

If we look positively for suspicious usage, we can also avoid insisting the expression occurs directly with an if statement -- instead the sink is simply any startsWith(...) call, or an .equals with a non-blank right-hand side, or similar.

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll

…SourceLib.qll Co-authored-by: Chris Smowton <smowton@github.com>

…Source.qhelp Co-authored-by: Chris Smowton <smowton@github.com>

haby0 · 2021-04-28T11:19:39Z

It works for structr, it's just such an unprincipled discriminator that I don't really want to commit this to our codebase. It wouldn't make any sense to anyone using the query -- this one is regarded as bad, and this one as good, because you used a String or Object-typed variable somewhere?

The Object type restriction on Node is currently only for the structr project. I haven't found it in other projects for the time being.

kevinbackhouse · 2021-04-29T10:18:37Z

My recommendation for improving the query is to restrict it so that it only flags cases like this one:

https://lgtm.com/projects/g/heathcare/heathcare/snapshot/a76e38b839a1fd02a12996c07bb70d33c1a38915/files/src/main/java/com/glaf/base/filter/PermissionFilter.java?sort=name&dir=ASC&mode=heatmap#x762b35b7412f98f6:1

In other words, only flag if-conditions that contain a hard-coded IP address: "192.168.0", "127.0.0", etc. Those are the places where an attacker has a higher chance of an authentication bypass by sending a spoofed IP address in the X-Forwarded-For field.

haby0 · 2021-04-29T11:17:49Z

My recommendation for improving the query is to restrict it so that it only flags cases like this one:

https://lgtm.com/projects/g/heathcare/heathcare/snapshot/a76e38b839a1fd02a12996c07bb70d33c1a38915/files/src/main/java/com/glaf/base/filter/PermissionFilter.java?sort=name&dir=ASC&mode=heatmap#x762b35b7412f98f6:1

In other words, only flag if-conditions that contain a hard-coded IP address: "192.168.0", "127.0.0", etc. Those are the places where an attacker has a higher chance of an authentication bypass by sending a spoofed IP address in the X-Forwarded-For field.

This will have a false negative.

@smowton What is your suggestion on this?

smowton · 2021-04-29T11:20:19Z

That looks pretty precise. Implement it and let's test it?

haby0 · 2021-04-29T11:20:54Z

That looks pretty precise. Implement it and let's test it?

haby0 · 2021-04-29T11:21:16Z

That looks pretty precise. Implement it and let's test it?

ok

haby0 · 2021-04-29T13:27:28Z

That looks pretty precise. Implement it and let's test it?

Please review.

https://lgtm.com/query/4289318833195816514/

smowton · 2021-04-29T13:49:32Z

Made an all-projects run of this for the security lab to look at

haby0 · 2021-04-29T14:23:42Z

https://lgtm.com/query/2268755143253610690/

kevinbackhouse · 2021-04-29T15:40:25Z

Thanks. The results are looking much better now. There is just one false positive pattern that seems to come up a lot:

"0:0:0:0:0:0:0:1".equals(ip) ? "127.0.0.1" : ip;

Examples:

So it might be worth adding special case for that pattern, but otherwise I am happy with this now.

haby0 · 2021-04-30T01:31:45Z

Thanks. The results are looking much better now. There is just one false positive pattern that seems to come up a lot:
"0:0:0:0:0:0:0:1".equals(ip) ? "127.0.0.1" : ip;
Examples:

https://lgtm.com/projects/g/zhangyp1/demo/snapshot/62dfd4ded70fdeae6d4ed39b268cdb2b0eb04f68/files/opslabJutil-master/src/main/java/com/opslab/helper/HttpRequestHelper.java?sort=name&dir=ASC&mode=heatmap#x55ca8e92fb877ffc:1

https://lgtm.com/projects/g/lizhiguoemail/bandian/snapshot/bcfeced4557bb67db7b6894867ef3f9e437775a3/files/src/main/java/com/lhsz/bandian/utils/IpUtils.java?sort=name&dir=ASC&mode=heatmap#x3a7fdd641de011ed:1

So it might be worth adding special case for that pattern, but otherwise I am happy with this now.

I made a modification to this false positive, please review the lgtm result.

https://lgtm.com/query/2916758049910443110/
https://lgtm.com/query/2367742774803333177/

smowton

Rename "UseOfLessTrustedSource" -> "ClientSuppliedIpUsedInSecurityCheck" everywhere, including filenames and class names.

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.qhelp

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.java

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll

smowton · 2021-04-30T08:51:54Z

java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrustedSourceLib.qll

+      ma.getAnArgument()
+          .(CompileTimeConstantExpr)
+          .getStringValue()
+          .regexpMatch("^((10\\.((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?)|(192\\.168\\.)|172\\.(1[6789]|2[0-9]|3[01])\\.)((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)(\\.)?((1\\d{2})?|(2[0-4]\\d)?|(25[0-5])?|([1-9]\\d|[0-9])?)$")


Factor the regex out into something like

string getIpAddressRegex() { result = ... }

…Source.qhelp Co-authored-by: Chris Smowton <smowton@github.com>

…SourceLib.qll Co-authored-by: Chris Smowton <smowton@github.com>

…Source.java Co-authored-by: Chris Smowton <smowton@github.com>

haby0 · 2021-04-30T09:53:19Z

https://lgtm.com/query/5856626224614437538/
https://lgtm.com/query/8934149255622582580/

haby0 · 2021-05-05T09:53:07Z

@smowton @kevinbackhouse thanks.

[Java] CWE-348: Use of less trusted source

3f0a326

haby0 requested a review from a team as a code owner April 8, 2021 09:15

github-actions bot added documentation Java labels Apr 8, 2021

intrigus-lgtm reviewed Apr 8, 2021

View reviewed changes

Restore @component annotation

86ef258

smowton self-assigned this Apr 8, 2021

haby0 mentioned this pull request Apr 8, 2021

[Java] CWE-348: Use of less trusted source github/securitylab#336

Closed

1 task

smowton reviewed Apr 8, 2021

View reviewed changes

haby0 and others added 3 commits April 8, 2021 19:17

Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrusted…

2100400

…SourceLib.qll Co-authored-by: Chris Smowton <smowton@github.com>

Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrusted…

bfbfe7a

…Source.qhelp Co-authored-by: Chris Smowton <smowton@github.com>

Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrusted…

1da48ed

…Source.qhelp Co-authored-by: Chris Smowton <smowton@github.com>

smowton reviewed Apr 13, 2021

View reviewed changes

haby0 added 4 commits April 19, 2021 20:05

Merge remote-tracking branch 'upstream/main' into UseOfLessTrustedSource

23b508c

Fix Modify the ql query (the qhelp part is not modified).

8296abc

Fix Modify the ql query (the qhelp part is not modified).

0159956

update qhelp file and ql comments

0053158

haby0 requested review from smowton and removed request for a team April 20, 2021 03:22

smowton reviewed Apr 20, 2021

View reviewed changes

haby0 and others added 5 commits April 20, 2021 19:31

Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrusted…

b60bffa

…SourceLib.qll Co-authored-by: Chris Smowton <smowton@github.com>

Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrusted…

0b1637a

…Source.qhelp Co-authored-by: Chris Smowton <smowton@github.com>

Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrusted…

d82878a

…Source.qhelp Co-authored-by: Chris Smowton <smowton@github.com>

Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrusted…

9ece4da

…Source.qhelp Co-authored-by: Chris Smowton <smowton@github.com>

Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrusted…

408dd31

…Source.qhelp Co-authored-by: Chris Smowton <smowton@github.com>

haby0 closed this Apr 29, 2021

haby0 reopened this Apr 29, 2021

use hardCode

e813257

Eliminate false positives\

711a74c

smowton reviewed Apr 30, 2021

View reviewed changes

smowton changed the title ~~[Java] CWE-348: Use of less trusted source~~ [Java] CWE-348: Using a client-supplied IP address in a security check Apr 30, 2021

haby0 and others added 4 commits April 30, 2021 16:54

Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrusted…

8142810

…Source.qhelp Co-authored-by: Chris Smowton <smowton@github.com>

Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrusted…

0691cac

…SourceLib.qll Co-authored-by: Chris Smowton <smowton@github.com>

Update java/ql/src/experimental/Security/CWE/CWE-348/UseOfLessTrusted…

f41301f

…Source.java Co-authored-by: Chris Smowton <smowton@github.com>

UseOfLessTrustedSource -> ClientSuppliedIpUsedInSecurityCheck"

fdcc517

haby0 requested a review from smowton April 30, 2021 09:44

smowton approved these changes Apr 30, 2021

View reviewed changes

smowton merged commit b2c0259 into github:main Apr 30, 2021

haby0 deleted the UseOfLessTrustedSource branch May 5, 2021 09:53

yabeow mentioned this pull request Oct 13, 2021

[Javascript] CWE-348: Client supplied ip used in security check #6864

Draft

This was referenced May 26, 2022

[C#] CWE-348: Using a client-supplied IP address in a security check manunio/learn-go-with-tests#2

Closed

[C#] CWE-348: Using a client-supplied IP address in a security check #9339

Closed

	not aa.getIndexExpr().toString() = "0"
	not aa.getIndexExpr().(CompileTimeConstantExpr).getIntValue() = 0

		@Documented
		@Component

[Java] CWE-348: Using a client-supplied IP address in a security check #5631

[Java] CWE-348: Using a client-supplied IP address in a security check #5631

Uh oh!

Conversation

haby0 commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haby0 Apr 8, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented Apr 13, 2021

Uh oh!

haby0 commented Apr 13, 2021

Uh oh!

smowton commented Apr 14, 2021

Uh oh!

haby0 commented Apr 15, 2021

Uh oh!

haby0 commented Apr 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

haby0 commented Apr 8, 2021 •

edited

Loading

haby0 Apr 8, 2021 •

edited

Loading

haby0 commented Apr 29, 2021 •

edited

Loading

haby0 commented Apr 30, 2021 •

edited

Loading