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

[Blake Sullivan]

On 10/22/10 1:32 PM, Martin Marinschek wrote:
> Hi guys,
>
> so my two cents on this issue, which were written on the train today,
> and do not reflect that latest discussion:
>
> @session token: can this be a unique token per request? I also don't
> see the need for an extra-token, it might be enough to keep this in
> the view-state key which is sent to the client. However, if we change
> the derivation of the view-key, we should additionally provide an API
> to access this.
>
> With this, we could implement a basic server-side double-submit
> tracking and back-button handling which is much better than we have
> right now. It would actually be quite easy to do so: we could have a
> phase-listener registered which knows about the list of processed
> request tokens, and will identify which request is coming in relative
> to this list. This of course will only work if the browser is not
> allowed to cache the pages, but that would almost never make sense in
> JSF anyways. All we would than need is a faces-artifact (name
> HistoryHandler?) which gets notified if this was a refresh, a
> back-button or a forward which was executed, also with the number of
> relative steps in between the current page and the corresponding
> back-button or forward. The application could then decide what to do
> with this information (we could possibly provide a default
> implementation). I believe this is one functionality which we should
> have had since day one, and as it is so easy to implement with this
> added request token, I think we should go for it. I would be willing
> to sponsor whatever is necessary to get this in.
Martin,

This is a pile of new functionality and the question is, and I'm curious 
what the application would do with this functionality.  What is the 
problem that the application is trying to avoid?
> I agree with other commenters that the CSRF protection really makes
> only sense per view - or actually, and this is hopefully a right
> assumption, per what the view does.
But I don't think it is a per-view.  Isn't it potentially per action?  
It's what the application logic does in response to the request that 
determines whether the application is idempotent with regards to that 
particular request.
>   The idea is: if the view is
> idempotent in the sense that it doesn't create any permanent changes
> to the app-data (whatever that is), then it is not necessary to check
> for the CSRF token - the CSRF attack will never get through to
> anything useful then. So I would envision a configuration per view (in
> the view metadata?) which would allow to mark the view as idempotent,
> and hence exclude it from the CSRF checking. Does this make sense to
> you guys?
>
> best regards,
>
> Martin
On a side note.  If the Mojarra server side state saving token isn't 
cryptographically strong, that's a security bug and needs to be fixed 
(and probably backported) regardless.

Unless we are talking about adding additional functionality, and it 
seems late to be doing so for this release, this handles the POST case 
for server side state saving.  The POST case for client side state 
saving is already handled by the fact that the state is carried in the 
POST and can't be forged.  The main open issues in these cases have to 
do with how to handles cases where we get and invalid request and 
thinking about how a user who has backed up off of the backbutton stack 
would like this to behave versus how a token we have never seen before 
should behave.

This leaves the ever popular GETs.  I'm probably being lazy, but at this 
point I'm willing to punt on GETs because of potential problems with:

1) Worries about referer leakage if the secret is encoded in the URL
2) How to deal with bookmarking
3) General dislike for ugly URLs

Admittedly, I think that 2) is the only one that really requires more 
thought, since I think that the solution to 1) is to a) Only worry about 
CSRF for pages served through a secure channel b) Require that pages 
served to authenticated users be served through a secure channel.  For 
3), I think it's gross but, that's just me :)



-- Blake Sullivan




> On 10/22/10, Blake Sullivan<blake.sullivan at oracle.com>  wrote:
>>    On 10/22/10 11:10 AM, Roger Kitain wrote:
>>>   Hey Blake -
>>>
>>> Thanks for responses..
>>>
>>> On 10/22/10 12:51 PM, Blake Sullivan wrote:
>>>>   Roger,
>>>>
>>>> Di you answer Alexander's question regarding whether this feature is
>>>> even necessary?  In the current releases, with no explicit CSRF
>>>> defenses, we have attacks against:
>>>> 1) GET attack
>>>> 2) POST attack against a page with token state saving
>>>> 3) POST attack against a page with client state saving
>>> Yes.  By number 2 - I presume you are talking about server side state
>>> saving.
>>>> 1) For GET attacks, it appears that we are saying that we aren't
>>>> going to defend against these anyway for this release.
>>> Yes. It's a bit trickier as we may need to do this on a page by page
>>> basis.
>> I think that it is trickier than that--you need to worry about things
>> like your parameter leaking out through the referer header.  At this
>> point, I don't see how you would get the spec work done in time.  Plus,
>> a well written web application won't have side-effects on the GET
>> anyway.  The CSRF attacker can GET all day and not change the
>> application state.
>>>> 2) For POST attacks against a page with token state saving.  As long
>>>> as the view state token is cryptographically strong, the attacker
>>>> can't guess the token anyway.  At that point, it comes down to what
>>>> the behavior is for a request that has an invalid token--we can
>>>> either return an error page or we can treat the request as a GET.  If
>>>> we treat the request as a GET, we are in 1
>>> For server side state saving by default, the view state is not as strong.
>>>    So we were looking at
>>> this CSRF "extra token" approach as facilitating the view state for
>>> CSRF solution.  However, there are ways to make the server side view
>>> state stronger - in which case there may not
>>> be a need for an extra token to augment the view state.  For now it
>>> may be as simple as a clarification
>>> (recommendation) in the spec about the server side view state being
>>> cryptographically strong - I'd have
>>> to check the spec to see what is there.
>> Yep.  Make it cryptographically strong, as it is in Trinidad and you are
>> done with the server-side storage POST except for figuring out what to
>> do if the token isn't valid.  I see no need to add yet another token
>> when the existing one will do.
>>>> 3) For POST attacks against a page with client state saving, as long
>>>> as attackers can't forge the client state, they can't do anything.
>>>> Since, they can't, we're safe here as well.
>>> Yes, client side view state is cryptographically strong.
>> Yep
>>>> Therefore, it isn't clear what the feature is buying us.  If there is
>>>> no clear need for the feature, it should not be added to the
>>>> specification.
>>> If we are deferring GET CSRF handling, and possibly adding a spec
>>> clarification for server side view state, then I agree.
>> Cool
>>
>> -- Blake Sullivan
>>
>>>> -- Blake Sullivan
>>>>
>>>>
>>>> On 9/21/10 10:15 AM, Roger Kitain wrote:
>>>>> There are two proposals for enhancing CSRF attacks in JSF.  We need
>>>>> to pick one.
>>>>>
>>>>> Proposal 1: Form Action URL Approach (Approach provided by Kito Mann)
>>>>>
>>>>> This approach does the following:      - Token is generated on the
>>>>> server consisting (minimally) of a randomly generated "secret key
>>>>>       (stored in session).
>>>>>     - ViewHandler.getActionURL method must include the token parameter
>>>>>       named "javax.faces.Token", and whose value is the token value.
>>>>>     - At render time this token will be included in Form's action URL
>>>>> - and it will be
>>>>>       posted back 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 URL must contain the parameter constant defined by
>>>>> ResponseStateManager.VIEW_TOKEN_PARAM
>>>>> The value of this parameter must be a cryptographically produced
>>>>> value minimally consisting
>>>>> of a "secret key". The "secret key" is a random generated value that
>>>>> was stored in the session
>>>>> (preferably around session creation time).  Implementations may also
>>>>> choose to combine other
>>>>> values with the secret key to produce a more complex token."
>>>>>
>>>>> Section 2.2.1
>>>>>
>>>>>   "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."
>>>>>
>>>>>
>>>>> Proposal 2: Form Hidden Field Approach
>>>>>
>>>>> This approach is similar to Approach 1, except a Form hidden field
>>>>> "javax.faces.Token"
>>>>> is used instead of appending to the Form's Action URL.
>>>>>
>>>>> Spec Document Modifications:
>>>>>
>>>>> Standard RenderKit Docs
>>>>>
>>>>> - Form Rendering
>>>>>
>>>>> "Render a hidden field named "javax.faces.Token" using the
>>>>> ResponseStateManager.VIEW_TOKEN_PARAM
>>>>>   constant.  The value of this hidden field is a cryptographically
>>>>> produced value that must at least
>>>>>   consist of a "secret key".  The "secret key" is a random generated
>>>>> value that was stored in the
>>>>>   session (preferably around session creation time).  Implementations
>>>>> may also choose to combine
>>>>>   other values with the secret key to produce a more complex token."
>>>>>
>>>>> Specification Document
>>>>>
>>>>> Section 2.2.1
>>>>>    "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 FacesException.
>>>>>
>>>>>
>>>>> For both approaches see:
>>>>>
>>>>>
>>>>> [1]
>>>>> https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id=869
>>>>>
>>>>> Look at the two latest change bundles attached to the issue.
>>>>>
>>>>> Please review by COB Friday as we have no time left for 2.1.
>>>>>
>>>>> Kudos to Kito Mann for helping out with the implementation.
>>>>>
>>>>> -roger
>>>
>>
>