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

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 On 10/1/10 1:22 PM, Alexander Smirnov wrote: > I disagree with suggested proposal. There are two pitfalls: > a) additional parameter just increases size of generated html code and > number of request parameters. JSF forms always contain viewId parameter, > so adding encoded token to that field will have minimal side effects. > That token should be checked by StateManager before restoring view. > b) URL parameter does not make sense for forms, but more important for > 'GET' access. To make this parameter compatible with portal environment, > it can be passed trough ExternalContext.encodeNamespace method ( while I > think that it should be portal vendor concern, where secure token has to > protect whole portal page ). > c) 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... > There should be per-page security configuration. > > On 09/30/2010 07:59 PM, Roger Kitain wrote: >> Folks - >> >> I have combined the previous two proposals into one based on valuable >> feed back from >> Kito Mann and Neil Griffin: >> >> On 9/27/10 5:12 PM, Kito Mann wrote: >>> On Tue, Sep 21, 2010 at 1:15 PM, Roger Kitain <roger.kitain(a)oracle.com >>> <mailto:roger.kitain@oracle.com>> wrote: >>> >>> >>> There are two proposals for enhancing CSRF attacks in JSF. We >>> need to pick one. >>> >>> >>> Why? 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. >>> >>> Also, the rewriting approach is what they're using for the generic >>> filter in Tomcat 7: >>> http://www.owasp.org/index.php/Cross-Site_Request_Forgery_%28CSRF%29_Prev.... >>> >>> I was talking to Mark Thomas, the guy working on this feature, about >>> this at JavaOne. As you can see, it doesn't even look like they're >>> using the hidden field approach (probably because it'd be very hard to >>> implement in a generic fashion). >>> >>> So, I think we should provide both features, but give the users to >>> turn off either algorithm, with a context parameter such as: >>> >>> javax.faces.CSRF_ALGORITHM=NONE | FORM | URL | BOTH (or ALL) >>> >> On 9/21/10 4:20 PM, Neil Griffin wrote: >>> #1 is probably not compatible with portlets. Portals are in full >>> control of creation of URLs in general, and it is not possible to >>> simply append "&javax.faces.Token=XYZ" to a portal's ActionURL and >>> expect it to work. >>> >>> #2 is compatible with portlets, but it would be best to have the >>> hidden field namespaced. Otherwise it would be like >>> javax.faces.VIEW_ID which is not namespaced, and can cause problems in >>> portals. >>> Neil >> ============================================== >> >> This proposal does the following: >> >> - Token is generated on the server consisting (minimally) of a >> randomly generated "secret key" (stored in session). The token >> can be generated >> upon session creation, or, at the time the token hidden field and/or >> token Url parameter is produced. >> >> - Standard context parameter javax.faces.CSRF_ALGORITHM controls >> where the token is produced by the following values: >> a. "form" : token is produced as the value of the hidden field >> with the name >> <form client id>:javax.faces.Token where <form client id> is the >> enclosing form's client identifier (produced with >> getClientId) - i.e. >> the hidden field is namespaced using the form's client >> identifier. >> b. "url" : token is produced and appended as a parameter to >> the form's action Url with the same name as in [a]. >> c. "all" : token is produced as in [a] *and* [b] >> d. "none" : no token is produced The default is [a] "form". >> >> - After render time, a subsequent action will send the token to the >> server. >> - Restore View Phase processing compares the incoming token request >> parameter value with the token value generated from the secret key >> in the session. >> >> Spec Document Modifications: >> >> Section 7.5.1: >> >> getActionURL: >> >> The returned URL must contain the >> parameter with a name consisting of the form's fully namespaced >> client identifier, the NamingContainer.SEPARATOR_CHAR and the constant >> defined by ResponseStateManager.VIEW_TOKEN_PARAM, >> if the configuration option javax.faces.CSRF_ALGORITHM >> is set to "url" or "all". The value of this parameter, known >> as the "token value" must be a cryptographically produced random >> generated >> value (known as the "secret key") retrieved from the session. If the >> "secret key" does not already exist in the session, create the >> random value >> and store it in the session. Implementations may choose to produce >> a more >> complex token value by combining the random "secret key" with other >> values. >> >> >> Section 2.2.1 >> >> "If the value of the configuration option javax.faces.CSRF_ALGORITHM >> is not "none" verify the "javax.faces.Token" request parameter >> value is the same as the token value generated from the "secret >> key" stored in the session. If the values do not match, throw a >> meaningful exception. The exception message must not include the >> token value." >> >> Standard RenderKit Docs >> >> - Form Rendering >> >> Render a hidden field named javax.faces.Token using the >> ResponseStateManager.VIEW_TOKEN_PARAM constant if the configuration >> option javax.faces.CSRF_ALGORITHM is set to the key words "form" or >> "all". The name must be namespaced with the enclosing form's client >> identifier. Render the value of this hidden field, known as the >> "token value", as a cryptographically produced random generated >> value (known as the "secret key") retrieved from the >> session. If the "secret key" does not already exist in the session, >> create the random value and >> store it in the session. Implementations may choose to produce a >> more complex token value by >> combining the random "secret key" with other values. >> Implementation Changes are attached to this issue: >> >> [1] >> https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id=869 >> >> >> -roger >> >> >> >> >> >> >> >> >> >> >> >> -- >> roger.kitain(a)oracle.com >> https://twitter.com/rogerk09 >> http://www.java.net/blogs/rogerk