сб, 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/.

2) This commit is not mentioned in changelog.

> +        "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.

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.)


5) I wonder whether it is a good idea to expose those constants as
ServletContext attributes, vs attributes of a specific request.

We already have nonce cache stored as a session attribute, thus maybe
it is not bad.

>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();

Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to