From 5703a029efbbd476742e51466817e130415fd20d Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Mon, 27 Nov 2023 09:03:54 +0100 Subject: [PATCH] Fix XSS vulnerability in property handling. Sanitize property values in the sanitizeInput method to prevent XSS attacks. Special characters in property values are now replaced with their corresponding HTML entities. This change enhances the security of the application by mitigating the risk of script injection through property values. --- ChangeLog.md | 6 + .../java/org/apache/wiki/api/Release.java | 2 +- .../java/org/apache/wiki/ui/Installer.java | 58 +++++++-- .../org/apache/wiki/ui/InstallerTest.java | 112 ++++++++++++++++++ 4 files changed, 166 insertions(+), 12 deletions(-) create mode 100644 jspwiki-main/src/test/java/org/apache/wiki/ui/InstallerTest.java diff --git a/ChangeLog.md b/ChangeLog.md index 4c8c882d0d..79e76b393f 100644 --- a/ChangeLog.md +++ b/ChangeLog.md @@ -17,6 +17,12 @@ specific language governing permissions and limitations under the License. --> +**2023-11-26 Arturo Bernal (juanpablo AT apache DOT org)** + +* _2.12.2-git-10_ + +* [JSPWIKI-1138](https://issues.apache.org/jira/browse/JSPWIKI-1138) Fix XSS vulnerability in property handling + **2023-11-25 Juan Pablo Santos (juanpablo AT apache DOT org)** * _2.12.2-git-09_ diff --git a/jspwiki-api/src/main/java/org/apache/wiki/api/Release.java b/jspwiki-api/src/main/java/org/apache/wiki/api/Release.java index 97c8502516..ff567b0493 100644 --- a/jspwiki-api/src/main/java/org/apache/wiki/api/Release.java +++ b/jspwiki-api/src/main/java/org/apache/wiki/api/Release.java @@ -69,7 +69,7 @@ public final class Release { *

* If the build identifier is empty, it is not added. */ - public static final String BUILD = "09"; + public static final String BUILD = "10"; /** * This is the generic version string you should use when printing out the version. It is of diff --git a/jspwiki-main/src/main/java/org/apache/wiki/ui/Installer.java b/jspwiki-main/src/main/java/org/apache/wiki/ui/Installer.java index 570d9722e0..28059a9b61 100644 --- a/jspwiki-main/src/main/java/org/apache/wiki/ui/Installer.java +++ b/jspwiki-main/src/main/java/org/apache/wiki/ui/Installer.java @@ -18,6 +18,7 @@ Licensed to the Apache Software Foundation (ASF) under one */ package org.apache.wiki.ui; +import org.apache.commons.io.FilenameUtils; import org.apache.wiki.api.core.Engine; import org.apache.wiki.api.core.Session; import org.apache.wiki.api.providers.AttachmentProvider; @@ -181,17 +182,18 @@ public void parseProperties () { // Get application name String nullValue = m_props.getProperty( APP_NAME, rb.getString( "install.installer.default.appname" ) ); - parseProperty( APP_NAME, nullValue ); + parseProperty( APP_NAME, nullValue ); + sanitizeInput( APP_NAME ); // Get/sanitize page directory nullValue = m_props.getProperty( PAGE_DIR, rb.getString( "install.installer.default.pagedir" ) ); parseProperty( PAGE_DIR, nullValue ); - sanitizePath( PAGE_DIR ); + sanitizeAndNormalizePath( PAGE_DIR ); // Get/sanitize work directory nullValue = m_props.getProperty( WORK_DIR, TMP_DIR ); parseProperty( WORK_DIR, nullValue ); - sanitizePath( WORK_DIR ); + sanitizeAndNormalizePath( WORK_DIR ); // Set a few more default properties, for easy setup m_props.setProperty( STORAGE_DIR, m_props.getProperty( PAGE_DIR ) ); @@ -239,17 +241,34 @@ private void parseProperty( final String param, final String defaultValue ) { } m_props.put( param, value ); } - + /** - * Simply sanitizes any path which contains backslashes (sometimes Windows users may have them) by expanding them to double-backslashes + * Sanitizes and normalizes the file path associated with the specified property key. + *

+ * This method performs a two-step process on the property value: + *

    + *
  1. Sanitization: It first sanitizes the input to mitigate the risk of Cross-Site Scripting (XSS) attacks. + * This is achieved by replacing specific characters known to be used in XSS attacks with their corresponding + * HTML entity codes. The characters '<', '>', '"', ''', and '/' are replaced with "<", ">", """, + * "'", and "/", respectively.
    2. + *
  2. Normalization: After sanitization, the method normalizes the file path using + * {@link org.apache.commons.io.FilenameUtils#normalizeNoEndSeparator(String)}. This normalization process + * ensures that the file path is in a consistent format, which is particularly important for file system + * operations, especially in environments like Windows where backslashes are used.
    3. + *
+ * If the property key is null or the property value is not set, the method performs no action. + *

* - * @param key the key of the property to sanitize + * @param key The key of the property whose value represents a file path. This key is used to retrieve, + * sanitize, and normalize the property value. If the key is null, no action is taken. */ - private void sanitizePath( final String key ) { - String s = m_props.getProperty( key ); - s = TextUtil.replaceString(s, "\\", "\\\\" ); - s = s.trim(); - m_props.put( key, s ); + private void sanitizeAndNormalizePath( final String key ) { + sanitizeInput( key ); // Sanitize the input for XSS protection + final String sanitizedValue = m_props.getProperty( key ); + if( sanitizedValue != null ) { + final String normalizedPath = FilenameUtils.normalizeNoEndSeparator( sanitizedValue ); + m_props.put( key, normalizedPath ); + } } private void validateNotNull( final String key, final String message ) { @@ -258,5 +277,22 @@ private void validateNotNull( final String key, final String message ) { m_session.addMessage( INSTALL_ERROR, message ); } } + + /** + * Sanitizes the property value associated with the specified key to prevent Cross-Site Scripting (XSS) attacks. + * This method retrieves the property value for the given key from the properties map, then replaces certain + * characters with their corresponding HTML entity codes to mitigate the risk of XSS vulnerabilities. + * The characters '<', '>', '"', ''', and '/' are replaced with "<", ">", """, "'", and "/", respectively. + * If the key is null, the method returns without making any changes. + * + * @param key The key of the property to be sanitized. If the key is null, no action is taken. + */ + private void sanitizeInput( final String key ) { + m_props.put( key, m_props.getProperty( key ).replaceAll( "<", "<" ) + .replaceAll( ">", ">" ) + .replaceAll( "\"", """ ) + .replaceAll( "'", "'" ) + .replaceAll( "/", "/" ) ); + } } diff --git a/jspwiki-main/src/test/java/org/apache/wiki/ui/InstallerTest.java b/jspwiki-main/src/test/java/org/apache/wiki/ui/InstallerTest.java new file mode 100644 index 0000000000..8e99d1f4dc --- /dev/null +++ b/jspwiki-main/src/test/java/org/apache/wiki/ui/InstallerTest.java @@ -0,0 +1,112 @@ +/* + 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. + */ +/* + * (C) Janne Jalkanen 2005 + * + */ + +package org.apache.wiki.ui; + +import net.sourceforge.stripes.mock.MockHttpServletRequest; +import net.sourceforge.stripes.mock.MockServletConfig; +import org.apache.wiki.TestEngine; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import javax.servlet.ServletException; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + + +public class InstallerTest { + + private Installer installer; + + + @BeforeEach + public void setUp() throws ServletException { + + + final IntallerMockHttpServletRequest req = new IntallerMockHttpServletRequest( "/JSPWiki", "/wiki/Wiki.jsp" ); + + + req.setParameter( "jspwiki.applicationName", "" ); + req.setParameter( "jspwiki.workDir", "C:\\Users\\Example\\Path" ); + req.setParameter( "jspwiki.fileSystemProvider.pageDir", "" ); + + + final MockServletConfig config = new MockServletConfig(); + config.setServletContext( TestEngine.createServletContext( "/JSPWiki" ) ); + + + installer = new Installer( req, config ); + } + + @Test + public void testSanitizeInput() { + // Assuming there's a method in Installer that processes the request and sets properties + installer.parseProperties(); + + // Verify the property is sanitized + String sanitizedValue = installer.getProperty( "jspwiki.applicationName" ); + assertEquals( "<script>alert('xss');</script>", sanitizedValue ); + } + + @Test + public void testSanitizePath() { + // Assuming there's a method in Installer that processes the request and sets properties + installer.parseProperties(); + + // Verify the path is normalized + String normalizedPath = installer.getProperty( "jspwiki.workDir" ); + assertEquals( "C:/Users/Example/Path", normalizedPath ); // Adjust expected value based on normalization behavior + } + + @Test + public void testSanitizePageDir() { + // Assuming there's a method in Installer that processes the request and sets properties + installer.parseProperties(); + + // Verify the path is normalized + String sanitizedValue = installer.getProperty( "jspwiki.fileSystemProvider.pageDir" ); + assertEquals( "<script>alert2('xss');</script>", sanitizedValue ); + } + + + static class IntallerMockHttpServletRequest extends MockHttpServletRequest { + private final Map< String, String > parameters = new HashMap<>(); + + public IntallerMockHttpServletRequest( String contextPath, String servletPath ) { + super( contextPath, servletPath ); + } + + @Override + public String getParameter( String name ) { + return parameters.get( name ); + } + + public void setParameter( String name, String value ) { + parameters.put( name, value ); + } + } + +} +