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

findsecbugs:UNVALIDATED_REDIRECT and context path #95

Open
trajano opened this Issue Sep 16, 2015 · 9 comments

Comments

Projects
None yet
4 participants
@trajano

trajano commented Sep 16, 2015

I have a redirect that is based on the context path of the request. The context path is not user entered and will be set to whatever the context is being used.

In addition, the URI being assembled with it has already been normalized, verified that it is not absolute and start with a path separator. As such the following code should not trigger the warning.

    final URI stateUri = URI.create(state).normalize();
    if (stateUri.isAbsolute()) {
        throw new AuthException("'state' must not be an absolute URI");
    }
    if (!stateUri.getPath().startsWith("/")) {
        throw new AuthException("'state' must start with '/'");
    }
   resp.sendRedirect(URI.create(req.getContextPath() + stateUri.toASCIIString()).normalize().toASCIIString());

Full code can be found in

https://github.com/trajano/jaspic-tester/blob/master/test-server-auth-module/src/main/java/net/trajano/auth/TestServerAuthModule.java#L136

There rule finds the error three times in the file.

@h3xstream

This comment has been minimized.

Show comment
Hide comment
@h3xstream

h3xstream Sep 16, 2015

Member

Great suggestion. It could also be the suggested code in the description for open direct.

Member

h3xstream commented Sep 16, 2015

Great suggestion. It could also be the suggested code in the description for open direct.

@trajano

This comment has been minimized.

Show comment
Hide comment
@trajano

trajano Sep 16, 2015

What is "open direct"?

trajano commented Sep 16, 2015

What is "open direct"?

@formanek

This comment has been minimized.

Show comment
Hide comment
@formanek

formanek Sep 17, 2015

Contributor

The analysis does not take conditions in conditional statements into account, so I'm afraid this will stay unsupported in the near future.

Contributor

formanek commented Sep 17, 2015

The analysis does not take conditions in conditional statements into account, so I'm afraid this will stay unsupported in the near future.

@h3xstream

This comment has been minimized.

Show comment
Hide comment
@h3xstream

h3xstream Sep 17, 2015

Member

@formanek
There is family of bugs patterns that I'm interested to cover is Injectable parameter requiring prefixes.

Considering the follow cases should be safe from Open redirect:

resp.sendRedirect(URI.create(req.getContextPath() + ...);

or

resp.sendRedirect(URI.create("/" + ...));

or

resp.sendRedirect(URI.create("https://test.com/" + ...));

It is not possible to redirect to another domain using these code because adding an external URL would create : /http://evil.com (invalid relative URL).


There is one caveat to consider.. req.getContextPath() return "" when the context is root.
Hypothetically a login page at the root http://website.com/?redirect=http://w3bsite.com could redirect to http://w3bsite.com.

/cc @trajano As @formanek mentions it will not be by looking at the condition that this case will be eliminate.

Member

h3xstream commented Sep 17, 2015

@formanek
There is family of bugs patterns that I'm interested to cover is Injectable parameter requiring prefixes.

Considering the follow cases should be safe from Open redirect:

resp.sendRedirect(URI.create(req.getContextPath() + ...);

or

resp.sendRedirect(URI.create("/" + ...));

or

resp.sendRedirect(URI.create("https://test.com/" + ...));

It is not possible to redirect to another domain using these code because adding an external URL would create : /http://evil.com (invalid relative URL).


There is one caveat to consider.. req.getContextPath() return "" when the context is root.
Hypothetically a login page at the root http://website.com/?redirect=http://w3bsite.com could redirect to http://w3bsite.com.

/cc @trajano As @formanek mentions it will not be by looking at the condition that this case will be eliminate.

@trajano

This comment has been minimized.

Show comment
Hide comment
@trajano

trajano Sep 17, 2015

ya you need to ensure somehow that the redirect URI has been checked to make it was marked as isAbsolute() and getPath().startsWith("/") however I am not sure how you can validate any of that. Maybe not possible with findbugs?

trajano commented Sep 17, 2015

ya you need to ensure somehow that the redirect URI has been checked to make it was marked as isAbsolute() and getPath().startsWith("/") however I am not sure how you can validate any of that. Maybe not possible with findbugs?

@trajano

This comment has been minimized.

Show comment
Hide comment
@trajano

trajano Sep 17, 2015

One thing I have to fix on mine but need to check as well on findsecbugs. I realize that I don't think we need to check that normalize() is called but we have to ensure that the input string must NOT have a ../ within the path or end with .. to prevent redirect outside the context of the prefix.

trajano commented Sep 17, 2015

One thing I have to fix on mine but need to check as well on findsecbugs. I realize that I don't think we need to check that normalize() is called but we have to ensure that the input string must NOT have a ../ within the path or end with .. to prevent redirect outside the context of the prefix.

@formanek

This comment has been minimized.

Show comment
Hide comment
@formanek

formanek Sep 17, 2015

Contributor

@h3xstream Nice idea, but I'm not sure how to detect prefix in bytecode - maybe something like values between the last call of StringBuilder constructor before URI.create and the first call of StringBuilder.append after that :)

Tracking what was checked probably would be possible by rewriting some methods of classes, which I'm only extending now, and creating temporary attributes removed at the end of block, but it probably wouldn't be easy and I prefer implementing other improvements earlier (I will open issues for some of them tomorrow).

Contributor

formanek commented Sep 17, 2015

@h3xstream Nice idea, but I'm not sure how to detect prefix in bytecode - maybe something like values between the last call of StringBuilder constructor before URI.create and the first call of StringBuilder.append after that :)

Tracking what was checked probably would be possible by rewriting some methods of classes, which I'm only extending now, and creating temporary attributes removed at the end of block, but it probably wouldn't be easy and I prefer implementing other improvements earlier (I will open issues for some of them tomorrow).

@h3xstream

This comment has been minimized.

Show comment
Hide comment
@h3xstream

h3xstream Jul 20, 2016

Member

@beauchef Please provide a code sample to illustrate your implementation.
Find-Sec-Bugs follow the state of variables. If an API is unknown, its return value will be threat as potential user input.
Currently, this rule need to be audit manually (mark as False positive) because, the proper implementations, as described above, are not understood by the engine.

Member

h3xstream commented Jul 20, 2016

@beauchef Please provide a code sample to illustrate your implementation.
Find-Sec-Bugs follow the state of variables. If an API is unknown, its return value will be threat as potential user input.
Currently, this rule need to be audit manually (mark as False positive) because, the proper implementations, as described above, are not understood by the engine.

@beauchef

This comment has been minimized.

Show comment
Hide comment
@beauchef

beauchef Jul 20, 2016

@h3xstream hello Philippe. We ended up ignoring the error. I use Spring Social to create the OAuth2 URL to Google. In OAuth, you have the possibility to pass a state to the OAuth provider. I bundle all the request parameters into this state, to Google, so when Google call me back, I get my state back. But I am redirecting to Google. I don't create the redirect URL myself. So basically, my redirect URL is constructed this way:

OAuth2ConnectionFactory connectionFactory =
                (OAuth2ConnectionFactory) locator.getConnectionFactory("google");
        OAuth2Operations oauthOperations = connectionFactory.getOAuthOperations();
        OAuth2Parameters params = new OAuth2Parameters();
        params.setScope(scope);
        params.setRedirectUri(callbackUrl);
        params.setState(state);
        String redirectUrl = oauthOperations.buildAuthorizeUrl(GrantType.AUTHORIZATION_CODE, params);

The locator is a Spring injected service. The scope and callbackUrl are configurations. The state is the base64 JSON of all the request parameters. So when Google calls me back, I get all the original request parameters.

beauchef commented Jul 20, 2016

@h3xstream hello Philippe. We ended up ignoring the error. I use Spring Social to create the OAuth2 URL to Google. In OAuth, you have the possibility to pass a state to the OAuth provider. I bundle all the request parameters into this state, to Google, so when Google call me back, I get my state back. But I am redirecting to Google. I don't create the redirect URL myself. So basically, my redirect URL is constructed this way:

OAuth2ConnectionFactory connectionFactory =
                (OAuth2ConnectionFactory) locator.getConnectionFactory("google");
        OAuth2Operations oauthOperations = connectionFactory.getOAuthOperations();
        OAuth2Parameters params = new OAuth2Parameters();
        params.setScope(scope);
        params.setRedirectUri(callbackUrl);
        params.setState(state);
        String redirectUrl = oauthOperations.buildAuthorizeUrl(GrantType.AUTHORIZATION_CODE, params);

The locator is a Spring injected service. The scope and callbackUrl are configurations. The state is the base64 JSON of all the request parameters. So when Google calls me back, I get all the original request parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment