With attachments. On Tue, May 04, 2021 at 09:15:50PM -0400, Roberto C. Sánchez wrote: > I never received a reply to the below request. Is it possible for > someone to review the CVE-2020-13933 patch? > > Additionally, I have prepared two patches for CVE-2020-17510, which I > have attached to this email. The adaptations were fairly minor, but a > review would still be appreciated. > > As far as CVE-2020-17523, it looks the affected file > (support/spring-boot/spring-boot-web-starter/src/main/resources/META-INF/spring.factories) > was introduced in 1.4.x. If that is the case, then CVE-2020-17523 would > not affect 1.3.x versions of shiro. Do I have that right? > > I'd like to get these changes wrapped up and a review would increase my > confidence in the adaptations I have made. > > Once these are nailed down (fixes for CVE-2020-13933 and CVE-2020-17510, > and confirmation that CVE-2020-17523 is not applicable in 1.3.x), then I > will be in a position to address getting a more recent upstream release > into Debian. > > Regards, > > -Roberto > > On Wed, Apr 07, 2021 at 03:41:25PM -0400, Roberto C. Sánchez wrote: > > Hi Brian, Ben, et al., > > > > I have what I believe to be a correct backport of commit dc194fc977ab to > > address CVE-2020-13933. The adaptations/changes include: > > > > - replacing the diamond operator <> with type specifications (the 1.3.x > > build uses source compatibility level 1.6) > > - replacing stream constructs with loops > > - omitting changes to non-existent tests > > - adjusting for the absence of the FilterConfig type (which seems to > > have been introduced in 1.4) > > > > If someone familiar with the code, the vulnerability, and the fix could > > review the attached patch that would be very much appreciated. Also, it > > would be quite helpful if there was a reproducer or a description of how > > to exploit the vulnerability to help with validating the fix. If that > > cannot be disclosed, then perhaps someone could confirm that this patch > > does in fact address the vulnerability; that would be helpful. > > > > Next I will start working on CVE-2020-17510 and CVE-2020-17523. Once I > > have those sorted out for the existing 1.3.x packages I will turn my > > attention to seeing about updating to a newer upstream release (which > > could be a lengthy process). > > > > Regards, > > > > -Roberto > > > > On Wed, Mar 31, 2021 at 03:21:55PM -0400, Roberto C. Sánchez wrote: > > > Hi Brian, > > > > > > Thanks for your help. I am working to backport dc194fc977ab to address > > > CVE-2020-13933 and after that I will move on to the fixes for > > > CVE-2020-17510 and CVE-2020-17523. > > > > > > As far as the maintainability of a 1.3.x package, upgrading to a newer > > > version is not an option for two reasons. First, I am working on > > > addressing these vulnerabilities in Debian Stretch, which is in the LTS > > > stage of its lifecycle, making an update to a new upstream release very > > > unlikely. Second, the shiro package in Debian was last updated about 2 > > > years ago and even in Debian unstable 1.3.x is the highest available > > > version. Any update to a new upstream version would need to start in > > > the unstable distribution. > > > > > > As an additional complication, the activemq package in Debian depends on > > > the shiro package, so a shiro update would need to be coordinated in a > > > that ensures it doesn't break activemq. > > > > > > That said, I will bring the issue up to see if I can get an update > > > started so that at the least the next Debian stable release has > > > something more recent. > > > > > > Regards, > > > > > > -Roberto > > > > > > On Tue, Mar 16, 2021 at 05:50:50PM -0400, Brian Demers wrote: > > > > Hey Roberto, > > > > > > > > Sorry about the delay on this one, I originally thought we had answered > > > > your question. > > > > > > > > The commit you are looking for is > > > > https://github.com/apache/shiro/commit/dc194fc977ab6cfbf3c1ecb085e2bac5db14af6d > > > > > > > > If you are maintaining a 1.3.x package this is going to become more > > > > difficult, is it possible to deprecate it and move to a recent version? > > > > > > > > > > > > > > > > On Sat, Jan 30, 2021 at 4:45 PM Roberto C. Sánchez <[email protected]> > > > > wrote: > > > > > > > > > Bump. > > > > > > > > > > On Tue, Dec 22, 2020 at 09:30:47AM -0500, Roberto C. Sánchez wrote: > > > > > > On Mon, Dec 21, 2020 at 09:33:44PM +0100, Benjamin Marwell wrote: > > > > > > > Hi Roberto, > > > > > > > > > > > > > > after talking to the PMC chair, I can give you three commit links. > > > > > > > > > > > > > > > > > > > https://github.com/apache/shiro/commit/042c59356cc6442345a9f935aed3e7603cb4dfad > > > > > > > > > > > > https://github.com/apache/shiro/commit/5b1add9a4c4ed046b52cf2132ed0f264a22caf1d > > > > > > > > > > > > https://github.com/apache/shiro/commit/1b9d8d99cd6d50d7114916508a13677a0fe6f345 > > > > > > > > > > > > > > I guess it is quite obvious what is inside these commits. > > > > > > > > > > > > > Hi Ben, > > > > > > > > > > > > This commits seem to have been made after the 1.6.0 release and > > > > > > before > > > > > > the 1.7.0 release. My belief is that they address CVE-2020-17510. > > > > > > Can > > > > > > you tell me if dc194fc977ab6cfbf3c1ecb085e2bac5db14af6d is the > > > > > > commit > > > > > > that addresses CVE-2020-13933? Are there other commits that go > > > > > > along > > > > > > with it to completely remedy CVE-2020-13933? > > > > > > > > > > > > Regards, > > > > > > > > > > > > -Roberto > > > > > > > > > > > > -- > > > > > > Roberto C. Sánchez > > > > > > > > > > -- > > > > > Roberto C. Sánchez > > > > > > > > > > > -- > > > Roberto C. Sánchez > > > http://people.connexer.com/~roberto > > > http://www.connexer.com > > > > -- > > Roberto C. Sánchez > > http://people.connexer.com/~roberto > > http://www.connexer.com > > > From dc194fc977ab6cfbf3c1ecb085e2bac5db14af6d Mon Sep 17 00:00:00 2001 > > From: Brian Demers <[email protected]> > > Date: Tue, 7 Jul 2020 21:06:35 -0400 > > Subject: [PATCH] Add a feature to allow for global filters > > > > Adds new filter to block invalid requests > > --- > > .../shiro/guice/web/ShiroWebModule.java | 25 ++- > > .../shiro/guice/web/ShiroWebModuleTest.java | 153 ++++++++++++++++++ > > .../ShiroWebFilterConfiguration.java | 8 + > > .../web/ConfiguredGlobalFiltersTest.groovy | 104 ++++++++++++ > > .../web/DisabledGlobalFiltersTest.groovy | 64 ++++++++ > > ...ShiroWebSpringAutoConfigurationTest.groovy | 30 +++- > > ...roWebAutoConfigurationTestApplication.java | 4 +- > > .../spring/web/ShiroFilterFactoryBean.java | 23 +++ > > .../config/AbstractShiroWebConfiguration.java | 3 - > > .../AbstractShiroWebFilterConfiguration.java | 9 +- > > .../config/ShiroWebFilterConfiguration.java | 6 + > > .../ShiroWebFilterConfigurationTest.groovy | 3 +- > > .../web/ShiroFilterFactoryBeanTest.java | 8 +- > > .../config/IniFilterChainResolverFactory.java | 18 +++ > > .../web/filter/InvalidRequestFilter.java | 124 ++++++++++++++ > > .../shiro/web/filter/mgt/DefaultFilter.java | 4 +- > > .../filter/mgt/DefaultFilterChainManager.java | 37 ++++- > > .../web/filter/mgt/FilterChainManager.java | 22 +++ > > .../web/servlet/AbstractShiroFilter.java | 1 + > > .../IniFilterChainResolverFactoryTest.groovy | 26 +++ > > .../web/env/IniWebEnvironmentTest.groovy | 69 ++++++++ > > .../filter/InvalidRequestFilterTest.groovy | 106 ++++++++++++ > > .../mgt/DefaultFilterChainManagerTest.groovy | 52 ++++++ > > .../org/apache/shiro/web/env/FilterStub.java | 45 ++++++ > > 24 files changed, 925 insertions(+), 19 deletions(-) > > create mode 100644 > > support/spring-boot/spring-boot-web-starter/src/test/groovy/org/apache/shiro/spring/boot/autoconfigure/web/ConfiguredGlobalFiltersTest.groovy > > create mode 100644 > > support/spring-boot/spring-boot-web-starter/src/test/groovy/org/apache/shiro/spring/boot/autoconfigure/web/DisabledGlobalFiltersTest.groovy > > create mode 100644 > > web/src/main/java/org/apache/shiro/web/filter/InvalidRequestFilter.java > > create mode 100644 > > web/src/test/groovy/org/apache/shiro/web/filter/InvalidRequestFilterTest.groovy > > create mode 100644 > > web/src/test/java/org/apache/shiro/web/env/FilterStub.java > > > > --- > > a/support/guice/src/main/java/org/apache/shiro/guice/web/ShiroWebModule.java > > +++ > > b/support/guice/src/main/java/org/apache/shiro/guice/web/ShiroWebModule.java > > @@ -18,9 +18,13 @@ > > */ > > package org.apache.shiro.guice.web; > > > > +import java.util.ArrayList; > > +import java.util.Arrays; > > import java.util.Collection; > > +import java.util.Collections; > > import java.util.HashMap; > > import java.util.LinkedHashMap; > > +import java.util.List; > > import java.util.Map; > > > > import javax.servlet.Filter; > > @@ -32,6 +36,7 @@ > > import org.apache.shiro.mgt.SecurityManager; > > import org.apache.shiro.session.mgt.SessionManager; > > import org.apache.shiro.web.env.WebEnvironment; > > +import org.apache.shiro.web.filter.InvalidRequestFilter;; > > import org.apache.shiro.web.filter.PathMatchingFilter; > > import org.apache.shiro.web.filter.authc.AnonymousFilter; > > import org.apache.shiro.web.filter.authc.BasicHttpAuthenticationFilter; > > @@ -86,7 +91,8 @@ > > public static final Key<SslFilter> SSL = Key.get(SslFilter.class); > > @SuppressWarnings({"UnusedDeclaration"}) > > public static final Key<UserFilter> USER = Key.get(UserFilter.class); > > - > > + @SuppressWarnings({"UnusedDeclaration"}) > > + public static final Key<InvalidRequestFilter> INVALID_REQUEST = > > Key.get(InvalidRequestFilter.class); > > > > static final String NAME = "SHIRO"; > > > > @@ -123,6 +129,12 @@ > > }; > > } > > > > + public List<Key<? extends Filter>> globalFilters() { > > + ArrayList<Key<? extends Filter>> filters = new ArrayList<Key<? > > extends Filter>>(); > > + filters.add(INVALID_REQUEST); > > + return Collections.unmodifiableList(filters); > > + } > > + > > @Override > > protected final void configureShiro() { > > bindBeanType(TypeLiteral.get(ServletContext.class), > > Key.get(ServletContext.class, Names.named(NAME))); > > @@ -134,6 +146,12 @@ > > > > this.configureShiroWeb(); > > > > + // add default matching route if not already set > > + if (!filterChains.containsKey("/**")) { > > + // no config, this will add only the global filters > > + this.addFilterChain("/**", new Key[0]); > > + } > > + > > setupFilterChainConfigs(); > > > > bind(FilterChainResolver.class).toProvider(new > > FilterChainResolverProvider(filterChains)); > > @@ -143,8 +161,15 @@ > > Map<Key<? extends PathMatchingFilter>, Map<String, String>> > > configs = new HashMap<Key<? extends PathMatchingFilter>, Map<String, > > String>>(); > > > > for (Map.Entry<String, Key<? extends Filter>[]> filterChain : > > filterChains.entrySet()) { > > - for (int i = 0; i < filterChain.getValue().length; i++) { > > - Key<? extends Filter> key = filterChain.getValue()[i]; > > + List<Key<? extends Filter>> globalFilters = > > this.globalFilters(); > > + Key<? extends Filter>[] pathFilters = filterChain.getValue(); > > + > > + // merge the global filters and the path specific filters > > + List<Key<? extends Filter>> filterConfigs = new > > ArrayList<Key<? extends Filter>>(globalFilters.size() + pathFilters.length); > > + filterConfigs.addAll(globalFilters); > > + filterConfigs.addAll(Arrays.asList(pathFilters)); > > + for (int i = 0; i < filterConfigs.size(); i++) { > > + Key<? extends Filter> key = filterConfigs.get(i); > > if (key instanceof FilterConfigKey) { > > FilterConfigKey<? extends PathMatchingFilter> > > configKey = (FilterConfigKey<? extends PathMatchingFilter>) key; > > key = configKey.getKey(); > > --- > > a/support/spring/src/main/java/org/apache/shiro/spring/web/ShiroFilterFactoryBean.java > > +++ > > b/support/spring/src/main/java/org/apache/shiro/spring/web/ShiroFilterFactoryBean.java > > @@ -25,8 +25,10 @@ > > import org.apache.shiro.util.StringUtils; > > import org.apache.shiro.web.config.IniFilterChainResolverFactory; > > import org.apache.shiro.web.filter.AccessControlFilter; > > +import org.apache.shiro.web.filter.InvalidRequestFilter; > > import org.apache.shiro.web.filter.authc.AuthenticationFilter; > > import org.apache.shiro.web.filter.authz.AuthorizationFilter; > > +import org.apache.shiro.web.filter.mgt.DefaultFilter; > > import org.apache.shiro.web.filter.mgt.DefaultFilterChainManager; > > import org.apache.shiro.web.filter.mgt.FilterChainManager; > > import org.apache.shiro.web.filter.mgt.FilterChainResolver; > > @@ -41,7 +43,9 @@ > > import org.springframework.beans.factory.config.BeanPostProcessor; > > > > import javax.servlet.Filter; > > +import java.util.ArrayList; > > import java.util.LinkedHashMap; > > +import java.util.List; > > import java.util.Map; > > > > /** > > @@ -121,6 +125,8 @@ > > > > private Map<String, Filter> filters; > > > > + private List<String> globalFilters; > > + > > private Map<String, String> filterChainDefinitionMap; > > //urlPathExpression_to_comma-delimited-filter-chain-definition > > > > private String loginUrl; > > @@ -131,6 +137,8 @@ > > > > public ShiroFilterFactoryBean() { > > this.filters = new LinkedHashMap<String, Filter>(); > > + this.globalFilters = new ArrayList<String>(); > > + this.globalFilters.add(DefaultFilter.invalidRequest.name()); > > this.filterChainDefinitionMap = new LinkedHashMap<String, > > String>(); //order matters! > > } > > > > @@ -332,6 +340,14 @@ > > } > > > > /** > > + * Sets the list of filters that will be executed against every > > request. Defaults to the {@link InvalidRequestFilter} which will block > > known invalid request attacks. > > + * @param globalFilters the list of filters to execute before specific > > path filters. > > + */ > > + public void setGlobalFilters(List<String> globalFilters) { > > + this.globalFilters = globalFilters; > > + } > > + > > + /** > > * Lazily creates and returns a {@link AbstractShiroFilter} concrete > > instance via the > > * {@link #createInstance} method. > > * > > @@ -388,6 +404,9 @@ > > } > > } > > > > + // set the global filters > > + manager.setGlobalFilters(this.globalFilters); > > + > > //build up the chains: > > Map<String, String> chains = getFilterChainDefinitionMap(); > > if (!CollectionUtils.isEmpty(chains)) { > > @@ -398,6 +417,9 @@ > > } > > } > > > > + // create the default chain, to match anything the path matching > > would have missed > > + manager.createDefaultChain("/**"); // TODO this assumes ANT path > > matching, which might be OK here > > + > > return manager; > > } > > > > --- > > a/web/src/main/java/org/apache/shiro/web/config/IniFilterChainResolverFactory.java > > +++ > > b/web/src/main/java/org/apache/shiro/web/config/IniFilterChainResolverFactory.java > > @@ -24,6 +24,7 @@ > > import org.apache.shiro.config.ReflectionBuilder; > > import org.apache.shiro.util.CollectionUtils; > > import org.apache.shiro.util.Factory; > > +import org.apache.shiro.web.filter.mgt.DefaultFilter; > > import org.apache.shiro.web.filter.mgt.FilterChainManager; > > import org.apache.shiro.web.filter.mgt.FilterChainResolver; > > import org.apache.shiro.web.filter.mgt.PathMatchingFilterChainResolver; > > @@ -32,7 +33,9 @@ > > > > import javax.servlet.Filter; > > import javax.servlet.FilterConfig; > > +import java.util.Collections; > > import java.util.LinkedHashMap; > > +import java.util.List; > > import java.util.Map; > > > > /** > > @@ -49,6 +52,8 @@ > > > > private FilterConfig filterConfig; > > > > + private List<String> globalFilters = > > Collections.singletonList(DefaultFilter.invalidRequest.name()); > > + > > private Map<String, ?> defaultBeans; > > > > public IniFilterChainResolverFactory() { > > @@ -72,6 +77,14 @@ > > this.filterConfig = filterConfig; > > } > > > > + public List<String> getGlobalFilters() { > > + return globalFilters; > > + } > > + > > + public void setGlobalFilters(List<String> globalFilters) { > > + this.globalFilters = globalFilters; > > + } > > + > > protected FilterChainResolver createInstance(Ini ini) { > > FilterChainResolver filterChainResolver = createDefaultInstance(); > > if (filterChainResolver instanceof > > PathMatchingFilterChainResolver) { > > @@ -122,9 +135,14 @@ > > //add the filters to the manager: > > registerFilters(filters, manager); > > > > + manager.setGlobalFilters(getGlobalFilters()); > > + > > //urls section: > > section = ini.getSection(URLS); > > createChains(section, manager); > > + > > + // create the default chain, to match anything the path matching > > would have missed > > + manager.createDefaultChain("/**"); // TODO this assumes ANT path > > matching > > } > > > > protected void registerFilters(Map<String, Filter> filters, > > FilterChainManager manager) { > > --- /dev/null > > +++ > > b/web/src/main/java/org/apache/shiro/web/filter/InvalidRequestFilter.java > > @@ -0,0 +1,136 @@ > > +/* > > + * Licensed to the Apache Software Foundation (ASF) under one > > + * or more contributor license agreements. See the NOTICE file > > + * distributed with this work for additional information > > + * regarding copyright ownership. The ASF licenses this file > > + * to you under the Apache License, Version 2.0 (the > > + * "License"); you may not use this file except in compliance > > + * with the License. You may obtain a copy of the License at > > + * > > + * http://www.apache.org/licenses/LICENSE-2.0 > > + * > > + * Unless required by applicable law or agreed to in writing, > > + * software distributed under the License is distributed on an > > + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY > > + * KIND, either express or implied. See the License for the > > + * specific language governing permissions and limitations > > + * under the License. > > + */ > > + > > +package org.apache.shiro.web.filter; > > + > > +import org.apache.shiro.web.util.WebUtils; > > + > > +import javax.servlet.ServletRequest; > > +import javax.servlet.ServletResponse; > > +import java.util.Arrays; > > +import java.util.Collections; > > +import java.util.List; > > + > > +/** > > + * A request filter that blocks malicious requests. Invalid request will > > respond with a 400 response code. > > + * <p> > > + * This filter checks and blocks the request if the following characters > > are found in the request URI: > > + * <ul> > > + * <li>Semicolon - can be disabled by setting {@code blockSemicolon = > > false}</li> > > + * <li>Backslash - can be disabled by setting {@code blockBackslash = > > false}</li> > > + * <li>Non-ASCII characters - can be disabled by setting {@code > > blockNonAscii = false}, the ability to disable this check will be removed > > in future version.</li> > > + * </ul> > > + * > > + * @see <a > > href="https://docs.spring.io/spring-security/site/docs/current/api/org/springframework/security/web/firewall/StrictHttpFirewall.html";>This > > class was inspired by Spring Security StrictHttpFirewall</a> > > + * @since 1.6 > > + */ > > +public class InvalidRequestFilter extends AccessControlFilter { > > + > > + private static final List<String> SEMICOLON = > > Collections.unmodifiableList(Arrays.asList(";", "%3b", "%3B")); > > + > > + private static final List<String> BACKSLASH = > > Collections.unmodifiableList(Arrays.asList("\\", "%5c", "%5C")); > > + > > + private boolean blockSemicolon = true; > > + > > + private boolean blockBackslash = true; > > + > > + private boolean blockNonAscii = true; > > + > > + @Override > > + protected boolean isAccessAllowed(ServletRequest request, > > ServletResponse response, Object mappedValue) throws Exception { > > + String uri = WebUtils.toHttp(request).getRequestURI(); > > + return !containsSemicolon(uri) > > + && !containsBackslash(uri) > > + && !containsNonAsciiCharacters(uri); > > + } > > + > > + @Override > > + protected boolean onAccessDenied(ServletRequest request, > > ServletResponse response) throws Exception { > > + WebUtils.toHttp(response).sendError(400, "Invalid request"); > > + return false; > > + } > > + > > + private boolean containsSemicolon(String uri) { > > + if (isBlockSemicolon()) { > > + int length = uri.length(); > > + for (int i = 0; i < length; i++) { > > + char c = uri.charAt(i); > > + if (c == ';') { > > + return true; > > + } > > + } > > + } > > + return false; > > + } > > + > > + private boolean containsBackslash(String uri) { > > + if (isBlockBackslash()) { > > + int length = uri.length(); > > + for (int i = 0; i < length; i++) { > > + char c = uri.charAt(i); > > + if (c == '\\') { > > + return true; > > + } > > + } > > + } > > + return false; > > + } > > + > > + private boolean containsNonAsciiCharacters(String uri) { > > + if (isBlockNonAscii()) { > > + return !containsOnlyPrintableAsciiCharacters(uri); > > + } > > + return false; > > + } > > + > > + private static boolean containsOnlyPrintableAsciiCharacters(String > > uri) { > > + int length = uri.length(); > > + for (int i = 0; i < length; i++) { > > + char c = uri.charAt(i); > > + if (c < '\u0020' || c > '\u007e') { > > + return false; > > + } > > + } > > + return true; > > + } > > + > > + public boolean isBlockSemicolon() { > > + return blockSemicolon; > > + } > > + > > + public void setBlockSemicolon(boolean blockSemicolon) { > > + this.blockSemicolon = blockSemicolon; > > + } > > + > > + public boolean isBlockBackslash() { > > + return blockBackslash; > > + } > > + > > + public void setBlockBackslash(boolean blockBackslash) { > > + this.blockBackslash = blockBackslash; > > + } > > + > > + public boolean isBlockNonAscii() { > > + return blockNonAscii; > > + } > > + > > + public void setBlockNonAscii(boolean blockNonAscii) { > > + this.blockNonAscii = blockNonAscii; > > + } > > +} > > --- a/web/src/main/java/org/apache/shiro/web/filter/mgt/DefaultFilter.java > > +++ b/web/src/main/java/org/apache/shiro/web/filter/mgt/DefaultFilter.java > > @@ -19,6 +19,7 @@ > > package org.apache.shiro.web.filter.mgt; > > > > import org.apache.shiro.util.ClassUtils; > > +import org.apache.shiro.web.filter.InvalidRequestFilter; > > import org.apache.shiro.web.filter.authc.*; > > import org.apache.shiro.web.filter.authz.*; > > import org.apache.shiro.web.filter.session.NoSessionCreationFilter; > > @@ -47,7 +48,8 @@ > > rest(HttpMethodPermissionFilter.class), > > roles(RolesAuthorizationFilter.class), > > ssl(SslFilter.class), > > - user(UserFilter.class); > > + user(UserFilter.class), > > + invalidRequest(InvalidRequestFilter.class); > > > > private final Class<? extends Filter> filterClass; > > > > --- > > a/web/src/main/java/org/apache/shiro/web/filter/mgt/DefaultFilterChainManager.java > > +++ > > b/web/src/main/java/org/apache/shiro/web/filter/mgt/DefaultFilterChainManager.java > > @@ -30,8 +30,10 @@ > > import javax.servlet.FilterChain; > > import javax.servlet.FilterConfig; > > import javax.servlet.ServletException; > > +import java.util.ArrayList; > > import java.util.Collections; > > import java.util.LinkedHashMap; > > +import java.util.List; > > import java.util.Map; > > import java.util.Set; > > > > @@ -52,17 +54,21 @@ > > > > private Map<String, Filter> filters; //pool of filters available for > > creating chains > > > > + private List<String> globalFilterNames; // list of filters to prepend > > to every chain > > + > > private Map<String, NamedFilterList> filterChains; //key: chain name, > > value: chain > > > > public DefaultFilterChainManager() { > > this.filters = new LinkedHashMap<String, Filter>(); > > this.filterChains = new LinkedHashMap<String, NamedFilterList>(); > > + this.globalFilterNames = new ArrayList<String>(); > > addDefaultFilters(false); > > } > > > > public DefaultFilterChainManager(FilterConfig filterConfig) { > > this.filters = new LinkedHashMap<String, Filter>(); > > this.filterChains = new LinkedHashMap<String, NamedFilterList>(); > > + this.globalFilterNames = new ArrayList<String>(); > > setFilterConfig(filterConfig); > > addDefaultFilters(true); > > } > > @@ -115,6 +121,17 @@ > > addFilter(name, filter, init, true); > > } > > > > + public void createDefaultChain(String chainName) { > > + // only create the defaultChain if we don't have a chain with this > > name already > > + // (the global filters will already be in that chain) > > + if (!getChainNames().contains(chainName) && > > !CollectionUtils.isEmpty(globalFilterNames)) { > > + // add each of global filters > > + for (String filterName : globalFilterNames) { > > + addToChain(chainName, filterName); > > + } > > + } > > + } > > + > > public void createChain(String chainName, String chainDefinition) { > > if (!StringUtils.hasText(chainName)) { > > throw new NullPointerException("chainName cannot be null or > > empty."); > > @@ -124,7 +141,14 @@ > > } > > > > if (log.isDebugEnabled()) { > > - log.debug("Creating chain [" + chainName + "] from String > > definition [" + chainDefinition + "]"); > > + log.debug("Creating chain [" + chainName + "] with global > > filters " + globalFilterNames + " and from String definition [" + > > chainDefinition + "]"); > > + } > > + > > + // first add each of global filters > > + if (!CollectionUtils.isEmpty(globalFilterNames)) { > > + for (String filterName : globalFilterNames) { > > + addToChain(chainName, filterName); > > + } > > } > > > > //parse the value by tokenizing it to get the resulting > > filter-specific config entries > > @@ -273,6 +297,21 @@ > > chain.add(filter); > > } > > > > + public void setGlobalFilters(List<String> globalFilterNames) throws > > ConfigurationException { > > + // validate each filter name > > + if (!CollectionUtils.isEmpty(globalFilterNames)) { > > + for (String filterName : globalFilterNames) { > > + Filter filter = filters.get(filterName); > > + if (filter == null) { > > + throw new ConfigurationException("There is no filter > > with name '" + filterName + > > + "' to apply to the > > global filters in the pool of available Filters. Ensure a " + > > + "filter with that > > name/path has first been registered with the addFilter method(s)."); > > + } > > + this.globalFilterNames.add(filterName); > > + } > > + } > > + } > > + > > protected void applyChainConfig(String chainName, Filter filter, > > String chainSpecificFilterConfig) { > > if (log.isDebugEnabled()) { > > log.debug("Attempting to apply path [" + chainName + "] to > > filter [" + filter + "] " + > > --- > > a/web/src/main/java/org/apache/shiro/web/filter/mgt/FilterChainManager.java > > +++ > > b/web/src/main/java/org/apache/shiro/web/filter/mgt/FilterChainManager.java > > @@ -22,6 +22,7 @@ > > > > import javax.servlet.Filter; > > import javax.servlet.FilterChain; > > +import java.util.List; > > import java.util.Map; > > import java.util.Set; > > > > @@ -165,6 +166,14 @@ > > void createChain(String chainName, String chainDefinition); > > > > /** > > + * Creates a chain that should match any non-matched request paths, > > typically {@code /**} assuming an {@link AntPathMatcher} I used. > > + * @param chainName The name of the chain to create, likely {@code > > /**}. > > + * @since 1.6 > > + * @see org.apache.shiro.lang.util.AntPathMatcher AntPathMatcher > > + */ > > + void createDefaultChain(String chainName); > > + > > + /** > > * Adds (appends) a filter to the filter chain identified by the given > > {@code chainName}. If there is no chain > > * with the given name, a new one is created and the filter will be > > the first in the chain. > > * > > @@ -195,4 +204,17 @@ > > * interface). > > */ > > void addToChain(String chainName, String filterName, String > > chainSpecificFilterConfig) throws ConfigurationException; > > + > > + /** > > + * Configures the set of named filters that will match all paths. > > These filters will match BEFORE explicitly > > + * configured filter chains i.e. by calling {@link > > #createChain(String, String)}, {@link #addToChain(String, String)}, etc. > > + * <br> > > + * <strong>Filters configured in this list wll apply to ALL > > requests.</strong> > > + * > > + * @param globalFilterNames the list of filter names to match > > ALL paths. > > + * @throws ConfigurationException if one of the filter names is > > invalid and cannot be loaded from the set of > > + * configured filters {@link > > #getFilters()}}. > > + * @since 1.6 > > + */ > > + void setGlobalFilters(List<String> globalFilterNames) throws > > ConfigurationException; > > } > > > -- > Roberto C. Sánchez
-- Roberto C. Sánchez
>From a28300448ae6c4bb78a8ba626b0cacb00f82d5f8 Mon Sep 17 00:00:00 2001 From: Brian Demers <[email protected]> Date: Thu, 3 Sep 2020 14:58:45 -0400 Subject: [PATCH] Adds configuration to toggle the normalization of backslashes This is normally handled by the container Update the InvalidRequestFilter to use WebUtils.ALLOW_BACKSLASH (new system property: org.apache.shiro.web.ALLOW_BACKSLASH) Fixes: SHIRO-794 --- .../web/filter/InvalidRequestFilter.java | 22 ++++-- .../org/apache/shiro/web/util/WebUtils.java | 4 +- .../filter/InvalidRequestFilterTest.groovy | 48 +++++++++++-- .../apache/shiro/web/util/WebUtilsTest.groovy | 52 ++++++++++++++ .../shiro/web/RestoreSystemProperties.java | 69 +++++++++++++++++++ 5 files changed, 182 insertions(+), 13 deletions(-) create mode 100644 web/src/test/java/org/apache/shiro/web/RestoreSystemProperties.java --- a/web/src/main/java/org/apache/shiro/web/filter/InvalidRequestFilter.java +++ b/web/src/main/java/org/apache/shiro/web/filter/InvalidRequestFilter.java @@ -19,10 +19,12 @@ package org.apache.shiro.web.filter; +import org.apache.shiro.util.StringUtils; import org.apache.shiro.web.util.WebUtils; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -48,16 +50,24 @@ private boolean blockSemicolon = true; - private boolean blockBackslash = true; + private boolean blockBackslash = !Boolean.getBoolean(WebUtils.ALLOW_BACKSLASH); private boolean blockNonAscii = true; @Override - protected boolean isAccessAllowed(ServletRequest request, ServletResponse response, Object mappedValue) throws Exception { - String uri = WebUtils.toHttp(request).getRequestURI(); - return !containsSemicolon(uri) - && !containsBackslash(uri) - && !containsNonAsciiCharacters(uri); + protected boolean isAccessAllowed(ServletRequest req, ServletResponse response, Object mappedValue) throws Exception { + HttpServletRequest request = WebUtils.toHttp(req); + // check the original and decoded values + return isValid(request.getRequestURI()) // user request string (not decoded) + && isValid(request.getServletPath()) // decoded servlet part + && isValid(request.getPathInfo()); // decoded path info (may be null) + } + + private boolean isValid(String uri) { + return !StringUtils.hasText(uri) + || ( !containsSemicolon(uri) + && !containsBackslash(uri) + && !containsNonAsciiCharacters(uri)); } @Override --- a/web/src/main/java/org/apache/shiro/web/util/WebUtils.java +++ b/web/src/main/java/org/apache/shiro/web/util/WebUtils.java @@ -56,6 +56,8 @@ public static final String SERVLET_REQUEST_KEY = ServletRequest.class.getName() + "_SHIRO_THREAD_CONTEXT_KEY"; public static final String SERVLET_RESPONSE_KEY = ServletResponse.class.getName() + "_SHIRO_THREAD_CONTEXT_KEY"; + public static final String ALLOW_BACKSLASH = "org.apache.shiro.web.ALLOW_BACKSLASH"; + /** * {@link org.apache.shiro.session.Session Session} key used to save a request and later restore it, for example when redirecting to a * requested page after login, equal to {@code shiroSavedRequest}. @@ -162,7 +164,7 @@ * @return normalized path */ public static String normalize(String path) { - return normalize(path, true); + return normalize(path, Boolean.getBoolean(ALLOW_BACKSLASH)); } /** --- a/web/src/test/groovy/org/apache/shiro/web/util/WebUtilsTest.groovy +++ b/web/src/test/groovy/org/apache/shiro/web/util/WebUtilsTest.groovy @@ -18,12 +18,15 @@ */ package org.apache.shiro.web.util +import org.apache.shiro.web.RestoreSystemProperties +import org.hamcrest.CoreMatchers import org.junit.Test import javax.servlet.http.HttpServletRequest import static org.easymock.EasyMock.* import static org.junit.Assert.* +import static org.hamcrest.CoreMatchers.* /** * Tests for {@link WebUtils}. @@ -162,6 +165,55 @@ } + @Test + void testNormalize() { + doNormalizeTest"/foobar", "/foobar" + doNormalizeTest "/foobar/", "/foobar/" + doNormalizeTest"", "/" + doNormalizeTest"foobar", "/foobar" + doNormalizeTest"//foobar", "/foobar" + doNormalizeTest"//foobar///", "/foobar/" + doNormalizeTest"/context-path/foobar", "/context-path/foobar" + doNormalizeTest"/context-path/foobar/", "/context-path/foobar/" + doNormalizeTest"//context-path/foobar", "/context-path/foobar" + doNormalizeTest"//context-path//foobar" ,"/context-path/foobar" + doNormalizeTest"//context-path/remove-one/remove-two/../../././/foobar", "/context-path/foobar" + doNormalizeTest"//context-path//../../././/foobar", null + doNormalizeTest"/context path/foobar", "/context path/foobar" + + doNormalizeTest"/context path/\\foobar", "/context path/\\foobar" + doNormalizeTest"//context-path\\..\\../.\\.\\foobar", "/context-path\\..\\../.\\.\\foobar" + doNormalizeTest"//context-path\\..\\..\\.\\.\\foobar", "/context-path\\..\\..\\.\\.\\foobar" + doNormalizeTest"\\context-path\\..\\foobar", "/\\context-path\\..\\foobar" + } + + @Test + void testNormalize_allowBackslashes() { + RestoreSystemProperties.withProperties(["org.apache.shiro.web.ALLOW_BACKSLASH": "true"]) { + doNormalizeTest"/foobar", "/foobar" + doNormalizeTest "/foobar/", "/foobar/" + doNormalizeTest"", "/" + doNormalizeTest"foobar", "/foobar" + doNormalizeTest"//foobar", "/foobar" + doNormalizeTest"//foobar///", "/foobar/" + doNormalizeTest"/context-path/foobar", "/context-path/foobar" + doNormalizeTest"/context-path/foobar/", "/context-path/foobar/" + doNormalizeTest"//context-path/foobar", "/context-path/foobar" + doNormalizeTest"//context-path//foobar" ,"/context-path/foobar" + doNormalizeTest"//context-path/remove-one/remove-two/../../././/foobar", "/context-path/foobar" + doNormalizeTest"//context-path//../../././/foobar", null + doNormalizeTest"/context path/foobar", "/context path/foobar" + doNormalizeTest"/context path/\\foobar", "/context path/foobar" + doNormalizeTest"//context-path\\..\\..\\.\\.\\foobar", null + doNormalizeTest"\\context-path\\..\\foobar", "/foobar" + + } + } + + void doNormalizeTest(String path, String expected) { + assertThat WebUtils.normalize(path), equalTo(expected) + } + void doTestGetPathWithinApplication(String contextPath, String requestUri, String expectedValue) { def request = createMock(HttpServletRequest) --- /dev/null +++ b/web/src/test/java/org/apache/shiro/web/RestoreSystemProperties.java @@ -0,0 +1,69 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.shiro.web; + +import groovy.lang.Closure; + +import java.io.Closeable; +import java.util.Collections; +import java.util.Map; +import java.util.Properties; + +/** + * Wrapper that will restore System properties after test methods. + * + * Based on: https://github.com/stefanbirkner/system-rules/blob/master/src/main/java/org/junit/contrib/java/lang/system/RestoreSystemProperties.java + */ +public class RestoreSystemProperties implements Closeable { + + private final Properties originalProperties; + + public RestoreSystemProperties() { + originalProperties = System.getProperties(); + System.setProperties(copyOf(originalProperties)); + } + + public void restore() { + System.setProperties(originalProperties); + } + + private Properties copyOf(Properties source) { + Properties copy = new Properties(); + copy.putAll(source); + return copy; + } + + public static <T> T withProperties(Closure<T> closure) { + return withProperties(Collections.emptyMap(), closure); + } + + public static <T> T withProperties(Map<String, String> properties, Closure<T> closure) { + + try (RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties()) { + properties.forEach(System::setProperty); + + return closure.call(); + } + } + + @Override + public void close() { + restore(); + } +}
>From 74d4cb6aee9aa1af4b098edc526a1e5630743f9b Mon Sep 17 00:00:00 2001 From: Brian Demers <[email protected]> Date: Tue, 29 Sep 2020 17:59:29 -0400 Subject: [PATCH] Disable jsessionid URL rewriting by default This matches the default of the InvalidRequestFilter Fixes: SHIRO-795 --- .../spring/web/config/AbstractShiroWebConfiguration.java | 2 +- .../shiro/web/session/mgt/DefaultWebSessionManager.java | 2 +- .../web/session/mgt/DefaultWebSessionManagerTest.groovy | 5 ++++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/web/src/main/java/org/apache/shiro/web/session/mgt/DefaultWebSessionManager.java b/web/src/main/java/org/apache/shiro/web/session/mgt/DefaultWebSessionManager.java index eb7eda1f..9aa275a9 100644 --- a/web/src/main/java/org/apache/shiro/web/session/mgt/DefaultWebSessionManager.java +++ b/web/src/main/java/org/apache/shiro/web/session/mgt/DefaultWebSessionManager.java @@ -58,7 +58,7 @@ public DefaultWebSessionManager() { cookie.setHttpOnly(true); //more secure, protects against XSS attacks this.sessionIdCookie = cookie; this.sessionIdCookieEnabled = true; - this.sessionIdUrlRewritingEnabled = true; + this.sessionIdUrlRewritingEnabled = false; } public Cookie getSessionIdCookie() { diff --git a/web/src/test/groovy/org/apache/shiro/web/session/mgt/DefaultWebSessionManagerTest.groovy b/web/src/test/groovy/org/apache/shiro/web/session/mgt/DefaultWebSessionManagerTest.groovy index 841569fc..35b31204 100644 --- a/web/src/test/groovy/org/apache/shiro/web/session/mgt/DefaultWebSessionManagerTest.groovy +++ b/web/src/test/groovy/org/apache/shiro/web/session/mgt/DefaultWebSessionManagerTest.groovy @@ -127,7 +127,7 @@ public class DefaultWebSessionManagerTest { ShiroHttpServletRequest.COOKIE_SESSION_ID_SOURCE); request.setAttribute(ShiroHttpServletRequest.REFERENCED_SESSION_ID, id); request.setAttribute(ShiroHttpServletRequest.REFERENCED_SESSION_ID_IS_VALID, Boolean.TRUE); - request.setAttribute(ShiroHttpServletRequest.SESSION_ID_URL_REWRITING_ENABLED, Boolean.TRUE); + request.setAttribute(ShiroHttpServletRequest.SESSION_ID_URL_REWRITING_ENABLED, Boolean.FALSE); replay(cookie); replay(request); @@ -147,6 +147,7 @@ public class DefaultWebSessionManagerTest { Cookie cookie = createMock(Cookie.class); mgr.setSessionIdCookie(cookie); mgr.setSessionIdCookieEnabled(false); + mgr.setSessionIdUrlRewritingEnabled(true) //we should not have any reads from the cookie fields - if we do, this test case will fail. @@ -182,6 +183,7 @@ public class DefaultWebSessionManagerTest { Cookie cookie = createMock(Cookie.class); mgr.setSessionIdCookie(cookie); mgr.setSessionIdCookieEnabled(false); + mgr.setSessionIdUrlRewritingEnabled(true) //we should not have any reads from the cookie fields - if we do, this test case will fail. @@ -218,6 +220,7 @@ public class DefaultWebSessionManagerTest { public void testGetSessionIdFromRequestUriPathSegmentParam() { mgr.setSessionIdCookieEnabled(false); + mgr.setSessionIdUrlRewritingEnabled(true) HttpServletRequest request = createMock(HttpServletRequest.class); HttpServletResponse response = createMock(HttpServletResponse.class); -- 2.20.1
