6:01 a.m.
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.
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_Prev....
> 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