-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Konstantin,
Thanks for the review. On 11/16/19 16:12, Konstantin Kolinko wrote: > сб, 16 нояб. 2019 г. в 18:55, <schu...@apache.org>: > >> >> This is an automated email from the ASF dual-hosted git >> repository. >> >> schultz pushed a commit to branch master in repository >> https://gitbox.apache.org/repos/asf/tomcat.git >> >> >> The following commit(s) were added to refs/heads/master by this >> push: new ff687b5 Improve CSRF prevention filter by exposing the >> request's current nonce to the request. ff687b5 is described >> below >> >> commit ff687b5524b2b78c449fe32e8c520da86068cd1a Author: >> Christopher Schultz <ch...@christopherschultz.net> AuthorDate: >> Sat Nov 16 10:54:36 2019 -0500 >> >> Improve CSRF prevention filter by exposing the request's current >> nonce to the request. --- >> java/org/apache/catalina/filters/Constants.java | 33 >> ++++++++++++++++++++++ >> .../catalina/filters/CsrfPreventionFilter.java | 5 ++++ >> .../catalina/filters/CsrfPreventionFilterBase.java | 10 +++++++ 3 >> files changed, 48 insertions(+) >> >> diff --git a/java/org/apache/catalina/filters/Constants.java >> b/java/org/apache/catalina/filters/Constants.java index >> 4fe41cd..87dd6c4 100644 --- >> a/java/org/apache/catalina/filters/Constants.java +++ >> b/java/org/apache/catalina/filters/Constants.java @@ -25,12 >> +25,34 @@ package org.apache.catalina.filters; */ public final >> class Constants { >> >> + /** + * The session attribute key under which the CSRF >> nonce + * cache will be stored. + */ public static final >> String CSRF_NONCE_SESSION_ATTR_NAME = >> "org.apache.catalina.filters.CSRF_NONCE"; >> >> + /** + * The request attribute key under which the >> current + * requests's CSRF nonce can be found. + */ + >> public static final String CSRF_NONCE_REQUEST_ATTR_NAME = + >> "org.apache.catalina.filters.CSRF_REQUEST_NONCE"; + + /** + >> * The name of the request parameter which carries CSRF nonces + >> * from the client to the server for validation. + */ public >> static final String CSRF_NONCE_REQUEST_PARAM = >> "org.apache.catalina.filters.CSRF_NONCE"; >> >> + /** + * The servlet context attribute key under which >> the + * CSRF request parameter name can be found. + */ + >> public static final String CSRF_NONCE_REQUEST_PARAM_NAME_KEY = + >> "org.apache.catalina.filters.CSRF_NONCE_PARAM_NAME"; + public >> static final String METHOD_GET = "GET"; >> >> public static final String CSRF_REST_NONCE_HEADER_NAME = >> "X-CSRF-Token"; @@ -39,6 +61,17 @@ public final class Constants >> { >> >> public static final String CSRF_REST_NONCE_HEADER_REQUIRED_VALUE >> = "Required"; >> >> + /** + * The session attribute key under which the CSRF >> REST nonce + * cache will be stored. + */ public static >> final String CSRF_REST_NONCE_SESSION_ATTR_NAME = >> "org.apache.catalina.filters.CSRF_REST_NONCE"; + + /** + * >> The servlet context attribute key under which the + * CSRF >> REST header name can be found. + */ + public static final >> String CSRF_REST_NONCE_HEDAER_NAME_KEY = > > 1) There is a typo in the name of the constant above, > s/HEDAER/HEADER/. ACK I will fix this. > 2) This commit is not mentioned in changelog. I will also be fixing this. I have a handful of changes for the CSRF prevention filter and I was going to file them all under "improvements", instead of individually. I can add that changelog entry sooner rather than later. >> + >> "org.apache.catalina.filters.CSRF_REST_NONCE_HEADER_NAME"; } diff >> --git >> a/java/org/apache/catalina/filters/CsrfPreventionFilter.java >> b/java/org/apache/catalina/filters/CsrfPreventionFilter.java >> index fff5c2f..d94cdec 100644 --- >> a/java/org/apache/catalina/filters/CsrfPreventionFilter.java +++ >> b/java/org/apache/catalina/filters/CsrfPreventionFilter.java @@ >> -128,6 +128,11 @@ public class CsrfPreventionFilter extends >> CsrfPreventionFilterBase { >> >> nonceCache.add(newNonce); >> >> + // Take this request's nonce and put it into the >> request + // attributes so pages can make direct use >> of it, rather than + // requiring the use of >> response.encodeURL. + >> request.setAttribute(Constants.CSRF_NONCE_REQUEST_ATTR_NAME, >> newNonce); + wResponse = new CsrfResponseWrapper(res, newNonce); >> } else { wResponse = response; diff --git >> a/java/org/apache/catalina/filters/CsrfPreventionFilterBase.java >> b/java/org/apache/catalina/filters/CsrfPreventionFilterBase.java >> index c0083f0..8d401af 100644 --- >> a/java/org/apache/catalina/filters/CsrfPreventionFilterBase.java >> +++ >> b/java/org/apache/catalina/filters/CsrfPreventionFilterBase.java >> @@ -78,6 +78,16 @@ public abstract class CsrfPreventionFilterBase >> extends FilterBase { // Set the parameters >> super.init(filterConfig); >> >> + // Put the expected request parameter name into the >> application scope + >> filterConfig.getServletContext().setAttribute( + >> Constants.CSRF_NONCE_REQUEST_PARAM_NAME_KEY, + >> Constants.CSRF_NONCE_REQUEST_PARAM); + + // Put the >> expected request header name into the application scope + >> filterConfig.getServletContext().setAttribute( + >> Constants.CSRF_REST_NONCE_HEDAER_NAME_KEY, + >> Constants.CSRF_REST_NONCE_HEADER_NAME); > > The typo that I mentioned above is visible here. > > 3) Why this code is here, in a *Base class? > > The CsrfPreventionFilterBase class does not use those constants. > They are used by specific subclasses only. Good question. I chose to use the base class because there was an existing init() method. I can move those to the subclass instead. The base class is really just a utility class anyway. The subclass does not actually need to subclass the base -- this is more of an "implementation inheritance" than an "interface inheritance". I'm happy to move to the subclass. > 4) I think that one possible improvement to CsrfPreventionFilter > may be to make the parameter name configurable. > > (I think that is implemented by adding a field and public > getter/setter to CsrfPreventionFilter class. If so, the base class > will have no access to the new field.) Yes, this was an improvement I was planning to make. The default parameter name is unnecessarily verbose. > 5) I wonder whether it is a good idea to expose those constants as > ServletContext attributes, vs attributes of a specific request. Hmm, that's a good question. The parameter name should not change for the life of the application, otherwise CSRF prevention would cause continuous request failures. > We already have nonce cache stored as a session attribute, thus > maybe it is not bad. Honestly, this value shouldn't be changing very much, which is why I chose to put it into the application scope. I'm happy to move it if there is a compelling reason to do so, but I do not see any such a reason at this point. > From other view, Servlet 4.0 spec (chapter 6.2.1 Filter Lifecycle) > allows to initialize filters lazily, before processing a request. > Currently Tomcat does not do so. It initializes all filters when a > web application starts. (Code: - StandardContext.filterStart() -> > ApplicationFilterConfig(...) constructor -> > ApplicationFilterConfig.initFilter() ) > > If filters were initialized lazily, presence of those attributes > in ServletContext would depend on whether the filter has started > yet. > >> try { Class<?> clazz = Class.forName(randomClass); randomSource = >> (Random) clazz.getConstructor().newInstance(); It's not possible to get around this problem. Either the request has gone through the Filter (in which case, the init() method has been called and the context attributes are present) or the request has not traversed the Filter and the CSRF nonce value is not available. In either case, the request is either 100% okay or 100% NOT okay and CSRF prevention would fail for reasons other than this attribute being missing from the application scope. - -chris -----BEGIN PGP SIGNATURE----- Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/ iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAl3UJBYACgkQHPApP6U8 pFjVyA/+Ier0XNeM+xZVXFTH9hWntpRpBsOmQpMuirM2IWVv64l97Dz47HpUK38y pF/rdc2aQ6f32Ra4xxNQCeATbJ79p0/Pe81P19GGNG9UFSjXQ9DL+514wq5LZMcf BHJh9YDnb1izOyB2aEIaBuf0G8lHWZZa2Yf04YXqbrSFkFm9oB3XP65NsXZxS2MU WkIQeJXKEGTa5g2lkyrDQPblGCJplU1Wd0qQAeGt46tgJdvRrnxSg/NWCf/SMLKJ d012AYXTVp2xQNGSvQWMcfmXfTRpAR0akSsI0iz1VM7Iae29RnOtmHdLbsWdUJCI I5F99ETJQ4rwNLB33Wmkv70doyeb0wYt5mgCwooHFgpaPa5i+Td/g1SWoNACn6Xy djt+wyy1tehzIOYYqmOhiJMceQ+PekDR2p29OQXoXmmI8Wb8tpAyQUnzcQv++z/n wkM7o9S4RwkN4YyoJD199hINZXCsgLEHxXU0N5+LsrTIy6c8lbuGkBH7XiqKhT4a C3cARkpbo3bTpkJVoJxPuojkJwRUuNaludXQTade87g/WnZo1ifhxr+ufdIZgT34 RyPkOkB5sc2uwv4RWvrzrwtiLDxoYUoH6eW/pWK/dcVLRHpr/9E1HcMbYRf7wyI6 UWE/j3LPXK2wSuXJ7bAChWYTsx/SBOB67yLzU/4hCyycj2vyNOo= =e6PP -----END PGP SIGNATURE----- --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
