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

Prevent HTTP response splitting #3938

Closed
wants to merge 1 commit into from

Conversation

@eddumelendez
Copy link
Contributor

commented Jun 19, 2016

Evaluate if http header value contains CR/LF.

Reference: https://www.owasp.org/index.php/HTTP_Response_Splitting

Fixes gh-3910

@pivotal-issuemaster

This comment has been minimized.

Copy link

commented Jun 19, 2016

@eddumelendez Thank you for signing the Contributor License Agreement!

@eddumelendez eddumelendez changed the title Prevent HTTP responde splitting Prevent HTTP response splitting Jun 19, 2016
@eddumelendez eddumelendez force-pushed the eddumelendez:gh-3910 branch 2 times, most recently from 77e605e to f47c6f2 Jun 19, 2016
@rwinch rwinch added this to the 4.2.0 M1 milestone Jun 20, 2016
*/
public abstract class OnCommittedResponseWrapper extends HttpServletResponseWrapper {

private static final String CR_OR_LF = "\r\n";

This comment has been minimized.

Copy link
@rwinch

rwinch Jun 21, 2016

Member

Is there a reason you made the change here? I think the best place for the change is in FirewalledResponse. You would override both setHeader and addHeader methods of HttpServletResponseWrapper

@eddumelendez eddumelendez force-pushed the eddumelendez:gh-3910 branch from f47c6f2 to cb92790 Jun 21, 2016
@eddumelendez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2016

@rwinch PR updated

@rwinch

This comment has been minimized.

Copy link
Member

commented Jun 21, 2016

@eddumelendez Thanks for the fast response. I had something like this in mind:

You would override both setHeader and addHeader methods of HttpServletResponseWrapper

@Override
public void setHeader(String name, String value) {
    if (CR_OR_LF.matcher(value).find()) {
        throw new IllegalArgumentException(
                "Invalid characters (CR/LF) in header");
    }
    super.setHeader(name, value);
}

@Override
public void addHeader(String name, String value) {
    if (CR_OR_LF.matcher(value).find()) {
        throw new IllegalArgumentException(
                "Invalid characters (CR/LF) in header");
    }
    super.addHeader(name, value);
}

You might even extract out the validation logic.

@eddumelendez eddumelendez force-pushed the eddumelendez:gh-3910 branch from cb92790 to b967475 Jun 21, 2016
@eddumelendez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2016

@rwinch Thanks for the hint. PR updated.

@eddumelendez eddumelendez force-pushed the eddumelendez:gh-3910 branch from b967475 to 9e27440 Jun 21, 2016
}

private void validateCRLF(String value) {
if (CR_OR_LF.matcher(value).find()) {
throw new IllegalArgumentException(
"Invalid characters (CR/LF) in redirect location");

This comment has been minimized.

Copy link
@rwinch

rwinch Jun 21, 2016

Member

We need a different error message since these are not redirect locations. You could probably just change this to header since redirect location is a header.

Evaluate if http header value contains CR/LF.

Reference: https://www.owasp.org/index.php/HTTP_Response_Splitting

Fixes gh-3910
@eddumelendez eddumelendez force-pushed the eddumelendez:gh-3910 branch from 9e27440 to 302dede Jun 21, 2016
@eddumelendez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2016

@rwinch PR updated. I am also printing the header in the exception message.

@rwinch

This comment has been minimized.

Copy link
Member

commented Jul 7, 2016

Thanks for the PR! This is merged via 26fa4a4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.