Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSRF not working on POST actions that require no form parameters #29763

Closed
FroMage opened this issue Dec 8, 2022 · 12 comments · Fixed by #29977
Closed

CSRF not working on POST actions that require no form parameters #29763

FroMage opened this issue Dec 8, 2022 · 12 comments · Fixed by #29977
Labels
area/security kind/bug Something isn't working
Milestone

Comments

@FroMage
Copy link
Member

FroMage commented Dec 8, 2022

Describe the bug

I'm pretty sure this is related to the problem I had in #22444 where the body was not read if the endpoint didn't have a single @RestForm parameter.

If your endpoint doesn't have any @RestForm or @FormParam parameter, RESTEasy Reactive doesn't add the body filter that reads the form parameters (urlencoded or multipart) and so the CSRF filter can't work:

@Path("Application")
public class Application {

    @Inject
    CsrfTokenParameterProvider csrfToken;

@Path("csrfToken")
    public String csrfToken() {
        return csrfToken.getToken();
    }

@Path("csrfForm1")
    @Consumes(MediaType.APPLICATION_FORM_URLENCODED)
    @POST
    public String csrfForm1(@RestForm String name) {
        return "OK: " + name;
    }

@Path("csrfForm2")
    @Consumes(MediaType.MULTIPART_FORM_DATA)
    @POST
    public String csrfForm2(@RestForm String name) {
        return "OK: " + name;
    }

@Path("csrfForm3")
    @Consumes(MediaType.APPLICATION_FORM_URLENCODED)
    @POST
    public String csrfForm3() {
        return "OK";
    }

@Path("csrfForm4")
    @Consumes(MediaType.MULTIPART_FORM_DATA)
    @POST
    public String csrfForm4() {
        return "OK";
    }
}
@QuarkusTest
public class RenardeResourceTest {

    @Test
    public void testCsrf() {
        RenardeCookieFilter cookieFilter = new RenardeCookieFilter();
        String token = given()
                .filter(cookieFilter)
                .when().get("/Application/csrfToken")
                .then()
                .statusCode(200)
                .extract().asString();
        Assertions.assertTrue(token.length() > 0, "Empty token in form: " + htmlForm);
        // no token, no dice
        given()
                .filter(cookieFilter)
                .when()
                .param("name", "Stef")
                .post("/Application/csrfForm1")
                .then()
                .statusCode(400);
        // token: good
        given()
                .filter(cookieFilter)
                .when()
                .param("csrf-token", token)
                .param("name", "Stef")
                .post("/Application/csrfForm1")
                .then()
                .statusCode(200)
                .body(is("OK: Stef"));
        // no token, no dice
        given()
                .filter(cookieFilter)
                .when()
                .multiPart("name", "Stef")
                .post("/Application/csrfForm2")
                .then()
                .statusCode(400);
        // token: good
        given()
                .filter(cookieFilter)
                .when()
                .multiPart("csrf-token", token)
                .multiPart("name", "Stef")
                .post("/Application/csrfForm2")
                .then()
                .statusCode(200)
                .body(is("OK: Stef"));
        given()
        .filter(cookieFilter)
        .when()
        .param("csrf-token", token)
        .post("/Application/csrfForm3")
        .then()
        .statusCode(200)
        .body(is("OK: Stef"));
        given()
        .filter(cookieFilter)
        .when()
        .multiPart("csrf-token", token)
        .post("/Application/csrfForm4")
        .then()
        .statusCode(200)
        .body(is("OK: Stef"));
    }
}

This currently prevents using the CSRF filter in any application that doesn't need form parameters, except for the CSRF token one. So it should be fixed in 2.15 (because I don't think the CSRF handler fix was backported to 2.14 so it doesn't work there anyway, unless i'm mistaken).

CC @sberyozkin

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

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

No response

Additional information

No response

@FroMage FroMage added the kind/bug Something isn't working label Dec 8, 2022
@sberyozkin
Copy link
Member

Hi Steph, well, we have now removed vertx handling it, CSRF feature is now a ServerRestHandler, to get multipart/form-data supported. So for this to work RestEasy Reactive should be configured to read the body even if no form parameters are present - CSRF feature can not control it.
In principle, we could fallback to trying to read the body the Vertx way if the form map provided to CsrfHandler is empty but the content type matches and Content-Length is > 0, would it be a good idea, I don't know...

@quarkus-bot
Copy link

quarkus-bot bot commented Dec 9, 2022

/cc @sberyozkin

@FroMage
Copy link
Member Author

FroMage commented Dec 9, 2022

Well, doesn't matter how, but the CSRF handler needs to force the body being read if it's multipart/urlencoded. This can be via a build item or a general purpose annotation like we proposed in #22444.

I've added a comment to #22444 to move the discussion along.

@sberyozkin
Copy link
Member

@FroMage Sounds good, I can add a CSRF test once a fix to #22444 is merged

@FroMage
Copy link
Member Author

FroMage commented Dec 13, 2022

The ironic thing is that with @WithFormRead, we can probably turn this back into a request filter.

@FroMage
Copy link
Member Author

FroMage commented Dec 13, 2022

OK, it's merged. Hope it's enough to get this fixed quickly :)

@sberyozkin
Copy link
Member

Thanks @FroMage I'll have a look soon, should be simple enough :-) (we can always migrate back to the filter if there will be a real need for it, having it as a handler right now seems OK)

@FroMage
Copy link
Member Author

FroMage commented Dec 14, 2022

Well, there's no way to make a handler force the form being read. I could add it, but I'm not sure we have to, because we made this a handler precisely to bypass the limitation that I removed on filters.

@sberyozkin
Copy link
Member

@FroMage Are you saying we have to go back to https://github.com/quarkusio/quarkus/blob/2.14.3.Final/extensions/csrf-reactive/runtime/src/main/java/io/quarkus/csrf/reactive/runtime/CsrfRequestResponseReactiveFilter.java, and add @WithFormRead to it ? What about this code,

public static Uni<MultiMap> getFormUrlEncodedData(RoutingContext context) {
?

@sberyozkin
Copy link
Member

Steph, may be I'm missing something, please clarify what kind of fix you expect

@FroMage
Copy link
Member Author

FroMage commented Dec 14, 2022

Yes, that's what I mean, except you don't have to read the form yourself it will always be read before your filter.

@sberyozkin
Copy link
Member

@FroMage I see what you mean, I was wondering how to access form parameters, I see it there, https://github.com/quarkusio/quarkus/pull/29825/files#diff-17360eebd1bfeec18218337a7ce737d98943bd55df9d940243051645053bd5ceR108.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants