tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantin Kolinko <knst.koli...@gmail.com>
Subject Re: [tomcat] branch master updated: Improve CSRF prevention filter by exposing the request's current nonce to the request.
Date Sat, 16 Nov 2019 21:12:23 GMT
сб, 16 нояб. 2019 г. в 18:55, <schultz@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 <chris@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message