Adding leadings slashes bypass security

[will421]

Describe the bug

Hi,

I'm trying to secure every /q/* path with basic auth. Configuration :

quarkus.http.auth.basic=true
quarkus.http.auth.policy.admin-policy.roles-allowed=role-admin
quarkus.http.auth.permission.role-admin.paths=/q/*
quarkus.http.auth.permission.role-admin.policy=admin-policy
quarkus.security.users.embedded.enabled=true
quarkus.security.users.embedded.plain-text=true
quarkus.security.users.embedded.users.admin=admin
quarkus.security.users.embedded.roles.admin=role-admin

It's working.

But when I add leadings slashes to my requests like curl 'http://localhost:8080//q/openapi', it will access the endpoint.
I can add more leadings slashes, it will still access the endpoint.

Expected behavior

I expect calls like
//q/openapi, //q/health/ready or custom resources like //q/hello to return 404 or 401 because of security policy

Actual behavior

Calls //q/... return the value expected by /q/...

How to Reproduce?

1. Clone https://github.com/will421/quarkus-leadingslash-security-issue/
2. mvn quarkus:dev
3. curl 'http://localhost:8080//q/openapi' or curl 'http://localhost:8080//q/health/ready' or curl 'http://localhost:8080//q/hello'

Output of uname -a or ver

MINGW64_NT-10.0-19045 PC3747 3.4.7-ea781829.x86_64 2023-07-05 12:05 UTC x86_64 Msys

Output of java -version

openjdk version "17.0.1" 2021-10-19
OpenJDK Runtime Environment Temurin-17.0.1+12 (build 17.0.1+12)
OpenJDK 64-Bit Server VM Temurin-17.0.1+12 (build 17.0.1+12, mixed mode, sharing)

Quarkus version or git rev

2.16.10 (reproduced in 3.3.2)

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.3 (ff8e977a158738155dc465c6a97ffaf31982d739)
Maven home: D:\Programs\maven
Java version: 17.0.1, vendor: Eclipse Adoptium, runtime: D:\Programs\jdk\jdk-17.0.1+12
Default locale: fr_FR, platform encoding: UTF-8
OS name: "windows 10", version: "10.0", arch: "amd64", family: "windows"

Additional information

No response

[quarkus-bot]

/cc @sberyozkin (security)

[sberyozkin]

I'm not sure what can be done with blocking the extra slashes, the simple expression expects the very first path segment start with /q, so when you have ////q, you have the very first path segment being empty so it does not match /q. HTTP policy matcher would have to become a complete reg ex matcher which will affect the performance for the majority of the flows.

@michalvavrik I wonder, instead of supporting only wildcards, may be we can introduce a convention, similar to what is used for CORS, if users want a given path be treated as a regex, then it should be surrounded by /, example, /[/q]+/ or something like that in which case the whole path component of the URI is matched against it

[sberyozkin]

@will421 You can probably request the security for /* and add exclusions for the permit policy if needed

[michalvavrik]

@michalvavrik I wonder, instead of supporting only wildcards, may be we can introduce a convention, similar to what is used for CORS, if users want a given path be treated as a regex, then it should be surrounded by /, example, /[/q]+/ or something like that in which case the whole path component of the URI is matched against it

Path matching is performance sensitive feature, I'm still not finished, but ATM I believe what I'm doing won't have impact on performance (also thanks to your suggestion to only support path segments smth/*/smth which helped me with that). But in relation to this issue, I'm also unsure for we expect exact match and I think it would take ugly addition to handle this, therefore I decided to finish path matching improvement first and then look at this. Not together.

So no regex :-)

[will421]

You can probably request the security for /* and add exclusions for the permit policy if needed

Thanks, not the most practical when adding public /* endpoints but it works.

@sberyozkin Is it expected behavior for the path resolution to ignore empty leading slashes ? I can't find informations on this or if it can be disabled.

[michalvavrik]

Ah, I just assumed this is different issue so I didn't read description. So FYI this is duplicated and we should close it as such #13756. @will421 no, this needs to be fixed or well documented, thanks for reporting and you certainly have a point. Please feel free to reopen in case you disagree. I suggest that you subscribe for the other issue.

In general, and probably not very practical either, you can implement your own global HTTP policy io.quarkus.vertx.http.runtime.security.HttpSecurityPolicy that will be applied on the all paths and then do path matching of your own choice.

[gsmet]

I think we need to discuss this more carefully. It's definitely not my expectation that, as a user, I have to handle the ///q case.
If this URL works but bypasses security, we have a problem. Security should handle this in the exact same way as our routing infrastructure.

/cc @cescoffier @geoand

[michalvavrik]

@gsmet I don't understand how does this issue differ to #13756, can you please give me a hint?

[michalvavrik]

It's definitely not my expectation that, as a user, I have to handle the ///q case.

I agree with this and I already gave it a lot of thought as I'm changing path matching for different issue. It's not like I propose to stale this, I am going to propose solution for #13756 relatively soon.

[gsmet]

Yeah, so I agree it's the same issue. What triggered me to reopen was this sentence: this needs to be fixed or well documented.

I don't think this needs to be well documented. It needs to be fixed and we also need to talk about if it's necessary to open a CVE.

And the old issue if from 2020, that's definitely a bummer.

Will your solution be backportable? My understanding is that you are also tackling a different problem that might make it harder to backport?

[michalvavrik]

Will your solution be backportable? My understanding is that you are also tackling a different problem that might make it harder to backport?

good point, it wouldn't be, okay, let's do this first

[will421]

@michalvavrik
Thanks.
With an HttpSecurityPolicy, I tried to DENY all paths starting with "//". It worked but it was too "hacky" for us.

So we choose to migrate to Quarkus 3 and use the management interface. This way we manage the security of /q/* path ( denying everying and adding needed exclusions ) without messing with our applications endpoints.

[michalvavrik]

Glad it worked out @will421 . There is going to be fix, just hold a short while. As of /q/*, I am afraid you need to secure /* or it won't work for all cases, but can't tell for sure without details. There are other options as well.

[gsmet]

This has been fixed in 2.16.11.Final, 3.2.6.Final and 3.3.3.

[mdoy179]

This has been fixed in 2.6.11.Final, 3.2.6.Final and 3.3.3.

Looks like a typo here, seems to have been fixed in 2.16.11.Final https://github.com/quarkusio/quarkus/releases/tag/2.16.11.Final

[geoand]

Indeed, that's what Guillaume meant

[gsmet]

Yeah, sorry, last week has been hard, I fixed the comment :/