From dev-return-203446-archive-asf-public=cust-asf.ponee.io@tomcat.apache.org Sat Nov 16 21:12:45 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 35581180648 for ; Sat, 16 Nov 2019 22:12:45 +0100 (CET) Received: (qmail 96754 invoked by uid 500); 16 Nov 2019 21:12:43 -0000 Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: "Tomcat Developers List" Delivered-To: mailing list dev@tomcat.apache.org Received: (qmail 96743 invoked by uid 99); 16 Nov 2019 21:12:43 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd1-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 16 Nov 2019 21:12:43 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd1-us-west.apache.org (ASF Mail Server at spamd1-us-west.apache.org) with ESMTP id 32101C01D7 for ; Sat, 16 Nov 2019 21:12:43 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd1-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: -0.2 X-Spam-Level: X-Spam-Status: No, score=-0.2 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=disabled Authentication-Results: spamd1-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mx1-ec2-va.apache.org ([10.40.0.8]) by localhost (spamd1-us-west.apache.org [10.40.0.7]) (amavisd-new, port 10024) with ESMTP id 7-licMcoCOAz for ; Sat, 16 Nov 2019 21:12:41 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=209.85.167.53; helo=mail-lf1-f53.google.com; envelope-from=knst.kolinko@gmail.com; receiver= Received: from mail-lf1-f53.google.com (mail-lf1-f53.google.com [209.85.167.53]) by mx1-ec2-va.apache.org (ASF Mail Server at mx1-ec2-va.apache.org) with ESMTPS id D44D6BC509 for ; Sat, 16 Nov 2019 21:12:40 +0000 (UTC) Received: by mail-lf1-f53.google.com with SMTP id j14so10696385lfb.8 for ; Sat, 16 Nov 2019 13:12:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :content-transfer-encoding; bh=FBV56j2L2stcLF/GLrEXeXlCVOjOPpfCfeJONq6dRrI=; b=msVKmiubgEYxHgUnWrk/WR8TIJjYiRL3rVEw746QmWORm7GNPHDEDM/ePRL3+FuJfv yFcLIdpzfg8SG9uOxK0kdYwkUC5NVkc7rSTytPhxPbknRKlHa2hRllzs7k1dfjJ99xuI 8noj2mfR9e2j6Sa1kkUobxR3CrGVMAopmtY+SoQP74Tpx0xisULsrmKJAa4OMXmklT/7 7amB6GSKpwN8zepBOCjGXqc09Cf/Y2cJEoX6i+PeT8O0UvDiDaftqqEHAWCySJH6Phph oxEB3RFrO1O2LRi0v/QMnRbt/vEpArry2kc4m3JCC0mAe32IqfGAqGzNesL+X53UVF3W DBMg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:content-transfer-encoding; bh=FBV56j2L2stcLF/GLrEXeXlCVOjOPpfCfeJONq6dRrI=; b=XAjxyac+gHoIH8uTI9i1BZMV6z1nn3WerjZPfRzMPj/xD1UPaFlRgbkbgB/9i0Ygz+ o3SSpJhHDu6omaIP9AqzInnEzs3em5m9bZCjGLQOSP1DRM9EwPuqJyk8AXL+tREml1wD Cok0Uiq9LII8a6Z9cpGgM/HL8+jZDH7BoNQO7jdp7A3r0VtmPyiU0GClXnwKz7hUw9wQ LAEPrnoGDB7whleMPE1S1lF645YeghsaR1HBh9SiGGD3wds0L5mFc4tPcxwNMcJ3WIW2 Xov+1FZrYvsSlqqj3Me8B3ucpMY4nwNBQoag/bf18oPx7S19PGjP1Yu8Y49IZVKEEZIF ryNg== X-Gm-Message-State: APjAAAWLK1SwRh2nZNNh3CBY0Mxe5yiauyRc/NFl1JCNGc4OY8jaQFQL 6uirlgqSLBgWqOe2qgCcZcGhKjESWI121lUsZvF+uOtDMx4= X-Google-Smtp-Source: APXvYqxeYTQ8I4olJT1vALu2m1hkpLyGhDd88FYwBtuqsVYW+totgzLl5VBnFK4k48F5du4AmUhoAzPBewumf6VFRYk= X-Received: by 2002:a19:6503:: with SMTP id z3mr15217427lfb.192.1573938753716; Sat, 16 Nov 2019 13:12:33 -0800 (PST) MIME-Version: 1.0 References: <157391973276.21709.15825124078093645550@gitbox.apache.org> In-Reply-To: <157391973276.21709.15825124078093645550@gitbox.apache.org> From: Konstantin Kolinko Date: Sun, 17 Nov 2019 00:12:23 +0300 Message-ID: Subject: Re: [tomcat] branch master updated: Improve CSRF prevention filter by exposing the request's current nonce to the request. To: Tomcat Developers List Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable =D1=81=D0=B1, 16 =D0=BD=D0=BE=D1=8F=D0=B1. 2019 =D0=B3. =D0=B2 18:55, : > > 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 > AuthorDate: Sat Nov 16 10:54:36 2019 -0500 > > Improve CSRF prevention filter by exposing the request's current nonc= e 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/a= pache/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 =3D > "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 =3D > + "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 =3D > "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 =3D > + "org.apache.catalina.filters.CSRF_NONCE_PARAM_NAME"; > + > public static final String METHOD_GET =3D "GET"; > > public static final String CSRF_REST_NONCE_HEADER_NAME =3D "X-CSRF-T= oken"; > @@ -39,6 +61,17 @@ public final class Constants { > > public static final String CSRF_REST_NONCE_HEADER_REQUIRED_VALUE =3D= "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 =3D > "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 =3D 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 CsrfPreven= tionFilterBase { > > nonceCache.add(newNonce); > > + // Take this request's nonce and put it into the request > + // attributes so pages can make direct use of it, rather tha= n > + // requiring the use of response.encodeURL. > + request.setAttribute(Constants.CSRF_NONCE_REQUEST_ATTR_NAME,= newNonce); > + > wResponse =3D new CsrfResponseWrapper(res, newNonce); > } else { > wResponse =3D response; > diff --git a/java/org/apache/catalina/filters/CsrfPreventionFilterBase.ja= va 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 sco= pe > + 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 =3D Class.forName(randomClass); > randomSource =3D (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