[jsr-314-open-mirror] [jsr-314-open] [Spec-869-Specify CSRF Solution] PROPOSAL

[Roger Kitain]

  On 10/22/10 7:01 AM, Andy Schwartz wrote:
> Another though occurred to me at 3AM while trying to convince a 
> screaming baby to go back to sleep...
>
> Several frameworks (Trinidad/ADF Faces as well as others) provide 
> their own form components (ie. components that replace h:form).  How 
> do we expect these form components to participate in the CSRF 
> solution?  In order for this to work, the form components must have 
> access to the secret key/token since they must render the hidden field 
> for the token.  Do we need to provide additional APIs to better 
> support this case?  Seems like without this our existing form 
> components will not be compatible with CSRF_ALGORITHM=form.
Custom forms would need to follow the new rendering requirements for 
CSRF (described in
  renderkit docs).  In 2.2, we could further simplify this by providing 
API "hooks".
By default, CSRF is not enabled, so existing apps continue to work.


>
> Andy
>
> On 10/21/10 9:53 PM, Andy Schwartz wrote:
>> Gang -
>>
>> Apologies for being so amazingly late in reviewing this - just 
>> haven't had a chance to look until now.
>>
>> Regarding:
>>
>>> For token encoded as url parameter this proposal protects whole
>>> application, so no one can either got logged in to protected site
>>> because of circular dependencies: to open login page, visitor has to
>>> have secure token, which one he can get only from JSF login page...
>>
>> I think that this assumes that:
>>
>> 1.  We are checking GET requests for the token.
>> 2.  We need a token for the initial request into the application.
>>
>> Looking at Mojarra's current implementation, these assumptions are 
>> not true.  From RestoreViewPhase.execute():
>>
>>>             if (isPostBack) {
>>>                 if (isCSRFOptionEnabled(facesContext)) {
>>>                     String convertedViewId = 
>>> viewHandler.deriveViewId(facesContext, viewId);
>>>                     if (!TokenHelper.verifyToken(facesContext, 
>>> convertedViewId)) {
>>>                         throw new FacesException("Token verification 
>>> failed.");
>>>                     }
>>
>> Since we only verify the token for POST requests, if we arrive on the 
>> login page via a GET, we should be fine.
>>
>> However, if we only verify POST requests, I am not sure that I 
>> understand the value of CSRF_ALGORITHM=URL.  Any POSTs from an h:form 
>> will already contain the token from the hidden form field.  Including 
>> a query parameter in the form's action seems redundant/unnecessary.
>>
>> Speciyfing that ViewHandler.getActionURL() must include the token 
>> query parameter has other undesirable side effects.  Both 
>> getRedirectURL() and getBookmarkableURL() call getActionURL().  As 
>> such, these URLs will end up with the query parameter even though we 
>> won't use this query parameter when handling the GET.  We end up with 
>> noticeably uglier URLs with no gain in security.
>>
>> Earlier Kito mentioned the following justification for supporting the 
>> CSRF=URL approach:
>>
>>> I think it's actually safer to allow both: 
>>> http://www.owasp.org/index.php/Cross-Site_Request_Forgery_%28CSRF%29_Prevention_Cheat_Sheet#Disclosure_of_Token_in_URL. 
>>> According to this, appending the token works better when you can't 
>>> guarantee that a server-side operation is occurring only through 
>>> POST. Especially with the new capabilities of JSF 2 for parsing view 
>>> parameters, I don't think we can make this guarantee.
>>
>> I think I see Kito's point.  However, if we are only verifying the 
>> token on POST requests, the CSRF=URL approach doesn't address the 
>> view parameter use case.  Again, we don't gain anything here.
>>
>> I get the feeling from both Alexander and Kito's comments that there 
>> was an assumption that we would be performing token verification for 
>> both POST and GET requests.  I suppose we can consider doing this.  
>> However, as Alexander hinted, this gets tricky fast.  If we only 
>> expose an option for globally securing all GETs, this means that the 
>> application can no longer support bookmarkable URLs (since every URL 
>> must contain a token which is only valid for a very limited 
>> lifetime).  Any attempt to access the application without a valid 
>> token (such as a bookmark to the login page) will fail.  As such, I 
>> agree with Alexander's comment:
>>
>>> There should be per-page security configuration.
>>
>> At least when it comes to the URL/GET-based solution, I don't see how 
>> enabling this globally is a useful solution.
>>
>> Based on the current implementation I fail to see the benefits of the 
>> CSRF=URL approach.  I see at least one fairly significant drawback: 
>> unnecessarily uglier redirect/bookmarkable URLs.  Unless I am missing 
>> something (which might well be the case), I am -1 on including 
>> CSRF=URL in the specification.
>>
>> Andy
>>
>


-- 
roger.kitain at oracle.com
https://twitter.com/rogerk09
http://www.java.net/blogs/rogerk