jzheaux / spring-security

Spring Security
http://spring.io/spring-security
Apache License 2.0
4 stars 3 forks source link

Submitting alg=none should not discuss class #5

Open rwinch opened 6 years ago

rwinch commented 6 years ago

Summary

Submitting an algorithm of none produces an error stating to "extend class to handle". The error message reveals too much developer information and is not well worded for a user. We should use an error message that states that an alg none is not supported. We should not discuss anything about extending a class.

> GET / HTTP/1.1
> Host: localhost:8081
> User-Agent: curl/7.47.0
> Accept: */*
> Authorization: Bearer ew0KICAiYWxnIjogIm5vbmUiLA0KICAidHlwIjogIkpXVCINCn0.ew0KICAic3ViIjogIjEyMzQ1Njc4OTAiLA0KICAibmFtZSI6ICJKb2huIERvZSIsDQogICJpYXQiOiAxNTE2MjM5MDIyDQp9.
> 
< HTTP/1.1 401 
< WWW-Authenticate: Bearer error="invalid_request", error_description="An error occurred while attempting to decode the Jwt: Unsecured (plain) JWTs are rejected, extend class to handle", error_uri="https://tools.ietf.org/html/rfc6750#section-3.1"
< X-Content-Type-Options: nosniff
< X-XSS-Protection: 1; mode=block
< Cache-Control: no-cache, no-store, max-age=0, must-revalidate
< Pragma: no-cache
< Expires: 0
< X-Frame-Options: DENY
< Content-Length: 0
< Date: Thu, 21 Jun 2018 17:27:21 GMT
<
jgrandja commented 6 years ago

For the given scenario where alg=none, the error code should be invalid_token instead of invalid_request.

As per spec:

invalid_token The access token provided is expired, revoked, malformed, or invalid for other reasons. The resource SHOULD respond with the HTTP 401 (Unauthorized) status code. The client MAY request a new access token and retry the protected resource request.

JwtAuthenticationProvider assumes invalid_request for all cases where JwtDecoder.decode() fails. This is not the case in all instances. The resulting JwtException should be evaluated to determine the appropriate error_code and error_description to set.

jzheaux commented 6 years ago

Currently, Nimbus offers no effective way to evaluate the nature of the exception short of parsing the error message itself, which would likely prove brittle over time.

To that end, I've logged an issue with Nimbus to offer a couple of ideas on how their exceptions could be more distinguishable: https://bitbucket.org/connect2id/nimbus-jose-jwt/issues/264/badjwtexception-contains-no

In the meantime, @rwinch suggested the idea of extending the process(PlainJWT) method and throw our own exception. This will address this specific leak completely, though hopefully Nimbus can be enhanced to allow us to address other exception customization in the future.

jzheaux commented 6 years ago

@jgrandja, good points - #3 is related to your point, too, I believe

jgrandja commented 6 years ago

@jzheaux Yes #3 is related