Potential Security flaw with OAuth sparkjava

[Benedikt Bünz]

Hi

is the following a security flaw or intended behavior:

StravaClient stravaClient = new StravaClient(AppConfig.getString("strava.client_id"), AppConfig.getString("strava.client_secret"));
stravaClient.setAuthorizationGenerator(profile -> profile.addRole("Strava"));
Clients clients = new Clients("http://localhost:4567/callback", stravaClient);
Config config = new Config(clients);
config.setHttpActionAdapter(new DefaultHttpActionAdapter());
config.addAuthorizer("strava", new RequireAnyRoleAuthorizer("Strava"));
CallbackRoute callback = new CallbackRoute(config, null, true, false);

get("/callback", callback);
post("/callback", callback);
get("/logout", new ApplicationLogoutRoute(config, "/"));


SecurityFilter admin = new SecurityFilter(config, "StravaClient", "strava", "", true);
before("/secure", admin);
AtomicInteger secretCount = new AtomicInteger(0);
post("/secure", (req, res) -> secretCount.incrementAndGet());
get("/secure", (req, res) -> secretCount.get());

When making a post or get request to /secure a redirect is issued but the post/get is still called:
So using curl -X POST http://localhost:4567/secure BEFORE oAuthing and then opening http://localhost:4567/secure returns 1. 
I think the reason for this is that the DefaultHTTPAdapter does not halt on redirect but I am not sure whether this should be the responsibility of the HTTPAdapter anyway and not of the securityfilter/securitylogic.
I am just using sparkjava so I have no idea what the behavior is in the other modules.

logger.debug("requires HTTP action: {}", code);
if (code == HttpConstants.UNAUTHORIZED) {
    halt(HttpConstants.UNAUTHORIZED, "authentication required");
} else if (code == HttpConstants.FORBIDDEN) {
    halt(HttpConstants.FORBIDDEN, "forbidden");
} else if (code == HttpConstants.OK) {
    halt(HttpConstants.OK, context.getBody());
} else if (code == HttpConstants.TEMP_REDIRECT) {
    context.getSparkResponse().redirect(context.getLocation());
}

[Jérôme LELEU]

Hi,

You're right, there is a security flaw here. We must halt on redirection and the DefaultHttpActionAdapter must have the appropriate behavior.

I just fixed that: https://github.com/pac4j/spark-pac4j/blob/master/src/main/java/org/pac4j/sparkjava/DefaultHttpActionAdapter.java#L31 on the 1.2.3-SNAPSHOT version.

Can you confirm the fix?

Thanks.

Best regards,

Jérôme

--
You received this message because you are subscribed to the Google Groups "pac4j-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to pac4j-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Benedikt Bünz]

Hi Jerome,

Yes this works, thanks. I still think though that the security filter should be responsible for halting. Custom HttpActionAdapters might fail to halt the request. Also passing any other code to the DefaultHttpActionAdapter will result in the same security bug. This is obviously a slightly bigger design decision. I would be happy to work on it if you agree that this would be a good thing.

Benedikt 

[Jérôme LELEU]

Hi,

The security filter delegates to the SecurityLogic which acts upon the specific web context of Spark. Though, for Spark (like for some other frameworks), this is not enough and the HttpActionAdapter is in charge of transforming the updated web context into the specific actions for Spark.

So it's really the responsibility of the HttpActionAdapter to halt the process, the SecurityLogic returns null if the user is authenticated and authorized to access the resource.

That said, you're right: when developing their custom HttpActionAdapter, developers may forget to halt and there is nothing worse than a security framework that allows you to generate security flaws.

So I think the best option is to create a specific SparkSecurityLogic (extending the DefaultSecurityLogic) to halt the process in case the HttpActionAdapater is called. I created: https://github.com/pac4j/spark-pac4j/issues/15

Would you mind giving it a try?

Thanks.

Best regards,

Jérôme

[Jérôme LELEU]

Thinking a little more about that, we may have a better solution by throwing an AccessGrantedException (instead of returning null) in case of success, catching it and return; or halt otherwise...

[Benedikt Bünz]

Explicit granting of access seems like a good idea. So SecurityLogic throws the exception and SecurityFilter catches it? I can work on that perhaps next week

[Jérôme LELEU]

Hi,

Yes, that's the idea. It seems fairly easy. Unless you have a better option. Keep us posted...

Thanks.

Best regards,

Jérôme

[Jérôme LELEU]

Hi,

I released spark-pac4j v1.2.3 with my security fix.

I'll release a new version 1.2.4 with yours later on.

Thanks.

Best regards,

Jérôme

[Jérôme LELEU]

Hi,

Have you made any progress on this?

Thanks.

Best regards,

Jérôme

[Jérôme LELEU]

Hi,

Any progress on this?

Thanks.

Best regards,

Jérôme

[Jérôme LELEU]

Hi,

Here is my proposal: https://github.com/pac4j/spark-pac4j/pull/17

Thanks.

Best regards,

Jérôme