Ed,
Here are more specific comments about each of parts of the proposal:
The specification should state that the view state token must be
cryptographically strong and verified. This will handles both cross
site request forgery and session fixation attacks on POSTs without
developers needing any form of opt in. This is also backwards
compatible with existing form renderers.
The only part that developers should have to opt into is the GET support.
For the GET support:
As noted before, the secret key solution is less secure than other
alternatives and is also subject to session fixation attacks.
Why does the <cross-site-request-forgery-element> live in the
<faces-config-element>? Since the JSF specification is adding the new
element, this isn't exactly an extension.
What features of <web-resource-collection> were you planning to use? It
seems that the <url-pattern> by itself would be sufficient, in which
case you could rename <cross-site-request-forgery-element> to something
else like <protected-urls>
The specification should be specific that these elements will be merged
across all faces-configs.
There is no runtime access to this information. I suspect that this
could be a problem for the portlet bridge.
Solutions that modify the URLs require that every encoded URL be
checked. Performance testing needs to be done for realistic cases.
This is an example where "referer" checking would be significantly faster.
We no longer need the "form" and "url" sections.
Section 7.5.1:
If we are munging the URLs, why would the URL parameter be anything
other than ResponseStateManager.VIEW_TOKEN_PARAM?
Section 2.2.1:
Shouldn't we be more specific about the exception thrown? I would think
that an IllegalStateException would do the trick, but we might want an
explicit exception so that the exception handling system knows what kind
of exception this is. I don't think that anyone would do this, but why
can't the token be in the exception?
-- Blake Sullivan
On 7/26/11 9:52 AM, Ed Burns wrote:
> B> It may have been at the 24th hour, but I believe that Alexander is
> B> correct. If we are only applying CSRF protection to form POSTs, why
> B> isn't the view state token completely sufficient? If we are attempting
> B> to apply CSRF protection to GETs, there are other issues that are going
> B> to crop up.
>
> EB> We want to apply it to any views to which the user wants it applied,
> EB> regardless of GET or POST. I understand that the existing view state
> EB> token takes a lot of the way there for pages using POST, however, there
> EB> is no requirement that the view token be cryptographically strong.
>
> B> Why do you believe that the ViewState token does not have to be
> B> cryptographically strong?
>
> I'm sorry, I was unclear. When I said, "there is no requirement that
> the view token be cryptographically strong" I was intending to refute
> your assertion that our existing view state token is a sufficient
> solution for 869-CSRF. In
> that light, I should have said, "in the current spec, there is no
> requirement that the view token be cryptographically strong".
>
> B> It would seem that the argument for why it doesn't need to be
> B> cryptographically strong is that without cracking the session id, it
> B> doesn't matter. However, we should making it cryptographically
> B> strong helps prevent session fixation attacks and is easy, so why
> B> wouldn't we? Do you disagree with the statement that a
> B> cryptographically strong ViewState token handles the POST request
> B> case?
>
> I agree that a cryptographically strong ViewState token handles the POST
> case.
>
> B> This leaves the GET case and I'm wondering what kinds of GET attacks you
> B> are worried about. GETs with no behavioral side-effects should be
> B> fine.
>
>
> B> As far as implementation, adding a secret to the URL has
> B> potential problems with referer leakage. Checking the "referer" header
> B> can have problems with proxies stripping the "referer" MACing the URL
> B> parameters does work but makes client-side URL construction
> B> significantly more complicated.
>
> B> As far as controlling which pages this applies to, it seems like kind of
> B> a pain and I would prefer that we look at ways of making this simpler or
> B> possibly combining with other page-level metadata that the application
> B> developers might wish to add.
>
> I do not agree with your assertion that the only way to control the
> extent of CSRF protected pages should be page-level metadata.
>
> KM> I really think it should be easy to turn this on -- you shouldn't
> KM> have to earmark every page if you don't want to. The possible
> KM> attacks using GET requests are pretty well documented, and since JSF
> KM> apps have no restrictions on what type of actions you can perform,
> KM> we can't guarantee that people aren't doing updates during GETs.
>
> IO> I personally don't think we need GET support (if someone can come up
> IO> with a good use case, I might change my mind. I think most of the
> IO> time it should be possible to work around the rare cases with a
> IO> commandLink in a form)
>
> B> I understand that solving GET would be nice. However, the GET problem
> B> only affects GET pages with side-effects--the application developer has
> B> control over the extent to which this affects her. The POST problem
> B> affects every developer and is therefore more pressing and easily solved
> B> by making the view state token cryptographically strong and choking in
> B> restore view--in fact Trinidad and I'm sure other frameworks already do
> B> this.
>
> B> On the GET side, we have two problems--one implementation and one
> B> controlling which pages this applies to (or doesn't apply to). I laid
> B> out the implementation possibilities.
>
> Unless I missed it, you said we should exclusively do something using
> in-page metadata. I assert that is not the only implementation
> possibility.
>
> B> The problem of determining when
> B> to apply is that any page that needs to be directly accessible from
> B> outside the application can not have this check applied to it, but JSF
> B> doesn't currently keep track of valid external entry points.
>
> ...and I'm saying it should.
>
> http://javaserverfaces-spec-public.java.net/nonav/proposals/JAVASERVERFACES_SPEC_PUBLIC-869-CSRF.txt
>
> - Define a<cross-site-request-forgery-protection> element, to be
> declared in the<faces-config-extension> element. If this element
> exists, and contains one or more<web-resource-collection> elements,
> the CSRF protection is said to be enabled for those views that are
> encompassed by the<url-pattern> elements in the
> <web-resource-collection> elements. Otherwise, the CSRF feature is
> not enabled for the application.
>
> B> If we don't want the application author to turn on or off individual
> B> pages, I think we end up with either rules for when the check should be
> B> applied.
>
> I think there should be a way to do it in the view metadata *as well as*
> in the application metadata. Also, what is the "or" to your "either"?
>
> B> If we are specifying rules, should the rules be based purely on the
> B> URL?
>
> The current proposal is the author declares<web-resource-collection>
> elements. Blake, is that sufficient?
>
> B> If we are modifying the generated URLs, I worry some about the
> B> performance impact of stamped actions.
>
> This is a good point.
>
> B> Even if we end up falling back to URL manipulation for GETs, I would
> B> prefer that we use referer checking if the referer was present in the
> B> HTTP Request that initially created the session, as this doesn't
> B> require any URL manipulation.
>
> Yes, using referrer is probably a valid option.
>
> AS> My opinion to use existing viewState parameter comes from frameworks
> AS> internals; adding one more hidden parameter could break some existing
> AS> libraries that relays on javax.faces.ViewState ( the only required by
> AS> current spec ), so that's matter of backward compatibility.
> AS> Fo GETJSF 2 already has appropriate "view parameter" that can be used
> AS> to check secure token. It can be special Validator or component that
> AS> extends UIViewParam ( or coming UIViewAction ? ) and blocks access to
> AS> page without proper token. Everything already there to implement such
> AS> protection and make it configurable per page.
>
> B> I think that you are saying that for POST requests, we should fix up the
> B> view state token and that for GET requests, we can leverage the
> B> UIViewParam support for the actual checking.
>
> Blake, can you please propose changes to
> <http://javaserverfaces-spec-public.java.net/nonav/proposals/JAVASERVERFACES_SPEC_PUBLIC-869-CSRF.txt>
> to address your concerns?
>
> To summarize:
>
> GET support
> ===========
>
> Alexander, Kito and I think we should have GET
> support. Blake and Imre think it is ok to not have it.
>
> Per-view configuration
> ======================
>
> Blake favors view-metadata only. Ed, and Kito favor application
> metadata. Ed favors enabling both.
>
> Ed
>