Fix Cors for alternate local addresses #8642

timyates · 2023-01-20T12:09:23Z

It was raised in #8560 (comment) that we have not completed the circle with CORS and localhost drive-by protection

This PR adds a check for 127.0.01 in addition to localhost.

Currently it has a couple of missing parts which are up for discussion:

For local connections, It does not take the Origin/Server port into account, which it should do to be spec compliant. I feel that adding this (whilst simple) could hamper local development of multiple services, or front-ends.
If the Origin is localhost and the request is 127.0.0.1, we currently allow this (and vice versa). Again this is not spec compliant (I think), but if we have an origin of either of the two, I'm currently happy with the risk.
We do not check all 127.0.0.0/8 addresses (which are local). To do so would require some level of parsing, and I worry that the cost to each request will outweigh the benefit to people with a 127.0.0.32 address (I've never seen anything other than 127.0.01 -- but I admit I've not been specifically looking)

Please comment with your thoughts on these 3 issues (and any others you think about)

nbrugger-tgm · 2023-01-20T14:03:54Z

We do not check all 127.0.0.0/8 addresses (which are local). To do so would require some level of parsing, and I worry that the cost to each request will outweigh the benefit to people with a 127.0.0.32 address (I've never seen anything other than 127.0.01 -- but I admit I've not been specifically looking)
@timyates

The problem is not people using 127.0.0.32 as their API host. Because 90% use 0.0.0.0 as their api host which includes all of 127.0.0.0/8. If i would create a local drive by attack i would generate a random adress fitting this IP range. If you check out a random Micronaut API on github and start is localhost 127.0.0.1as well as 127.0.0.32 will probably yield the same result.

nbrugger-tgm · 2023-01-20T14:12:14Z

http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java

+    private boolean isLocal(@NonNull String hostString) {
+        URI uri = URI.create(hostString);
+        String host = uri.getHost();
+        return "localhost".equals(host) || "127.0.0.1".equals(host);


Suggested change

return "localhost".equals(host) || "127.0.0.1".equals(host);

return "localhost".equals(host) || host.startsWith("127");

this is the most accurate you get without reading the systems network settings and it matches the whole of 127.0.0.0/8

What happens when someone builds a website 127.0.0.malicious.com?

my bad not thinking about this, it would need to be coupled with something like host.matches("\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}")

Yeah, I didn't want to cause a regular expression match to be executed for every request 🤔

Using manual splitting and parsing makes it about ~30-50% slower from my immature and very vague performance tests.

On my local machine the test takes ~2ms with regex ~4ms with custom parsing, Still adding 2ms to a request seem less than ideal (ofc this 2ms depend on the machine/CPU, JIT and runtime warmup) i dont have a better way in mind atm

The only other thing i could think about is using the NetworkInterface API to check and adress for a loopback bound

The test i used

class Main { public static void main(String[] args) { var start = System.currentTimeMillis(); var rest = Arrays.stream("127.0.0.1".split("\\.",4)) .allMatch(s -> { try{ Integer.parseInt(s); return true; }catch(Exception e){return false;} }); var end = System.currentTimeMillis(); System.out.println("Non regex : "+(end-start)+"ms"); start = System.currentTimeMillis(); rest = "127.0.0.1".matches("\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}"); end = System.currentTimeMillis(); System.out.println("Regex : "+(end-start)+"ms"); } }

Thats interesting but on my system 127.0.0.2 resolves on the default project using Arch Linux. Maybe on other systems it works different?

curl --location --request GET 'https://launch.micronaut.io/create/default/com.example.demo?lang=JAVA&build=GRADLE&test=JUNIT&javaVersion=JDK_17' --output demo.zip unzip demo.zip cd demo ./gradlew run (different terminal) curl 127.0.0.3:8080 (different terminal) curl 127.200.200.203:8080

On my machine both curl commands pass with 404 not found json template which means the server is reached

Can you add a reproducer as described in #8642 (comment)

I think osx only supports 127.0.0.1, which is why I'm not seeing it

See : sdelamo/simple-request-attacks#1

I think a simple ubuntu docker container should solve your OSX reproduction problem :)

...-server-tck/src/main/java/io/micronaut/http/server/tck/tests/cors/CorsSimpleRequestTest.java

timyates · 2023-01-20T15:34:50Z

http-server-netty/src/test/groovy/io/micronaut/http/server/netty/cors/CorsFilterSpec.groovy

-        ['.*bar$', '.*foo$']                | 'asdfasdf foo'
-        ['.*bar$', '.*foo$']                | 'asdfasdf bar'
+        ['.*bar$', '.*foo$']                | 'http://asdfasdf.foo'
+        ['.*bar$', '.*foo$']                | 'http://asdfasdf.bar'


This test was failing as the origin was not a valid Origin format of:

Origin: null Origin: <scheme>://<hostname> Origin: <scheme>://<hostname>:<port>

And I now parse it with URI to get the hostname

sdelamo · 2023-01-23T12:13:59Z

We have to protect against these two scenarios, and we should protect without incurring any performance costs if possible.

If you run this sample app locally.

https://github.com/sdelamo/simple-request-attacks

And you visit in a browser that does not protect against localhost-driven attach such as Chrome:

Either:

https://sdelamo.github.io/simple-request-attacks/stopEndpoint.html

Or
https://sdelamo.github.io/simple-request-attacks/127001.html

The former URL sends a request against http://localhost:8080/stop, and the latter sends a request against http://127.0.0.1:8080/stop

The former has been patched since 3.8.1, and the latter will be patched via this PR.

If you have another attack scenario with a reproducible example, please submit a PR against the attack demo https://sdelamo.github.io/simple-request-attacks, and I will be happy to re-evaluate and patch it.

But we have to be careful not to incur performance costs.

nbrugger-tgm · 2023-01-23T13:25:04Z

If you have another attack scenario with a reproducible example
@sdelamo

Thanks for the repo! I showed off the problem i mentioned with my PR : sdelamo/simple-request-attacks#1

timyates · 2023-01-23T13:48:20Z

Ignore me... 😿

I'm now thinking we need to check the server address rather than the HOST header address to reduce the variables...

timyates · 2023-01-23T15:16:10Z

@nbrugger-tgm Right.... I think that's it...

Tested it inside a container with

curl -H "Origin: https://example.reqbin.com" 127.200.200.203:8080/stop
curl -H "Origin: https://127.hack.com" 127.200.200.203:8080/stop
curl -H "Origin: https://127.0.0.1:10101" 127.200.200.203:8080/stop

And only the last request worked...

nbrugger-tgm · 2023-01-23T16:07:53Z

Looks good to me, but I think that your implementation using URL could be simpler with a startsWith check. Anyway since you are feature complete and the problem I pointed out is fixed you have my approval (as if you need it :P)

Also it just came to my mind that you also support websockets so maybe check ws and wss too?

timyates · 2023-01-23T22:13:09Z

Thanks for all the help with this @nbrugger-tgm 😎👍

I added a page https://sdelamo.github.io/simple-request-attacks/stopChecks.html to hopefully make testing easier

graemerocher · 2023-01-24T09:57:09Z

http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java

+    private boolean isOriginLocal(@NonNull String hostString) {
+        URI uri = URI.create(hostString);
+        String host = uri.getHost();
+        return "localhost".equals(host) || "127.0.0.1".equals(host);


use SocketUtils.LOCALHOST constant

graemerocher · 2023-01-24T09:57:56Z

http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java

+        return hostString.startsWith("http://localhost")
+            || hostString.startsWith("https://localhost")
+            || hostString.startsWith("http://127.")
+            || hostString.startsWith("https://127.")
+            || hostString.startsWith("ws://localhost")
+            || hostString.startsWith("wss://localhost")
+            || hostString.startsWith("ws://127.")
+            || hostString.startsWith("wss://127.");


can we improve the performance of this with a check that the first char is w or h?

Added a pre-check for a host of length 0 and that the first char must be h or w

http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java

sonarcloud · 2023-01-24T12:01:55Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

87.1% Coverage
0.0% Duplication

timyates added 3 commits January 20, 2023 12:08

Add tests for localhost CORs check [skip-ci]

44da05b

Add cors test for 127.0.0.1 from a different remote origin [skip ci]

57aab02

WIP

b14c297

timyates changed the title ~~Add tests for localhost CORs check [skip-ci]~~ Fix Cors for alternate local addresses Jan 20, 2023

timyates mentioned this pull request Jan 20, 2023

Error 403 forbidden POST login using localhost #8560

Closed

nbrugger-tgm suggested changes Jan 20, 2023

View reviewed changes

nbrugger-tgm reviewed Jan 20, 2023

View reviewed changes

...-server-tck/src/main/java/io/micronaut/http/server/tck/tests/cors/CorsSimpleRequestTest.java Show resolved Hide resolved

timyates added 2 commits January 20, 2023 15:28

Fix second method

0f78b53

Fix tests

58aca0c

timyates marked this pull request as ready for review January 20, 2023 15:33

timyates requested review from sdelamo and graemerocher January 20, 2023 15:34

timyates commented Jan 20, 2023

View reviewed changes

timyates added this to the 3.8.3 milestone Jan 20, 2023

timyates added the type: bug Something isn't working label Jan 20, 2023

timyates self-assigned this Jan 20, 2023

timyates linked an issue Jan 20, 2023 that may be closed by this pull request

Error 403 forbidden POST login using localhost #8560

Closed

Fix CORs for the many many loopback addresses of Linux

0926ba1

sdelamo approved these changes Jan 23, 2023

View reviewed changes

sdelamo requested a review from yawkat January 23, 2023 15:45

nbrugger-tgm approved these changes Jan 23, 2023

View reviewed changes

Add ws: and wss: as prefixes

a4a8e54

Checkstyle

6a7e2b6

timyates added 3 commits January 24, 2023 09:07

Drop 17 dev

93ce86e

Fix yaml

2272bee

Remove parens for Sonar

2adb8eb

graemerocher requested changes Jan 24, 2023

View reviewed changes

yawkat reviewed Jan 24, 2023

View reviewed changes

http-server/src/main/java/io/micronaut/http/server/cors/CorsFilter.java Outdated Show resolved Hide resolved

timyates added 2 commits January 24, 2023 10:25

Check first char of host as an optimization

4f43729

Handle invalid Origins

35cc6af

yawkat approved these changes Jan 24, 2023

View reviewed changes

timyates requested review from graemerocher and yawkat January 24, 2023 10:30

yawkat approved these changes Jan 24, 2023

View reviewed changes

graemerocher approved these changes Jan 24, 2023

View reviewed changes

sdelamo merged commit 01adb21 into 3.8.x Jan 27, 2023

sdelamo deleted the local-cors-fix branch January 27, 2023 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Cors for alternate local addresses #8642

Fix Cors for alternate local addresses #8642

timyates commented Jan 20, 2023 •

edited

Loading

nbrugger-tgm commented Jan 20, 2023

nbrugger-tgm Jan 20, 2023

timyates Jan 20, 2023

nbrugger-tgm Jan 20, 2023

timyates Jan 20, 2023

nbrugger-tgm Jan 20, 2023 •

edited

Loading

nbrugger-tgm Jan 23, 2023

timyates Jan 23, 2023

timyates Jan 23, 2023

nbrugger-tgm Jan 23, 2023

nbrugger-tgm Jan 23, 2023

timyates Jan 20, 2023 •

edited

Loading

sdelamo commented Jan 23, 2023

nbrugger-tgm commented Jan 23, 2023

timyates commented Jan 23, 2023 •

edited

Loading

timyates commented Jan 23, 2023

nbrugger-tgm commented Jan 23, 2023

timyates commented Jan 23, 2023

graemerocher Jan 24, 2023

graemerocher Jan 24, 2023

timyates Jan 24, 2023

sonarcloud bot commented Jan 24, 2023

	return "localhost".equals(host) \|\| "127.0.0.1".equals(host);
	return "localhost".equals(host) \|\| host.startsWith("127");

Fix Cors for alternate local addresses #8642

Fix Cors for alternate local addresses #8642

Conversation

timyates commented Jan 20, 2023 • edited Loading

nbrugger-tgm commented Jan 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbrugger-tgm Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timyates Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

sdelamo commented Jan 23, 2023

nbrugger-tgm commented Jan 23, 2023

timyates commented Jan 23, 2023 • edited Loading

timyates commented Jan 23, 2023

nbrugger-tgm commented Jan 23, 2023

timyates commented Jan 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 24, 2023

timyates commented Jan 20, 2023 •

edited

Loading

nbrugger-tgm Jan 20, 2023 •

edited

Loading

timyates Jan 20, 2023 •

edited

Loading

timyates commented Jan 23, 2023 •

edited

Loading