Skip to content

Lutece 2206 #144

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/java/fr/paris/lutece/portal/web/style/ThemesJspBean.java
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import fr.paris.lutece.portal.service.portal.ThemesService;
import fr.paris.lutece.portal.service.template.AppTemplateService;
import fr.paris.lutece.portal.service.util.AppPathService;
import fr.paris.lutece.portal.service.util.AppPropertiesService;
import fr.paris.lutece.portal.web.admin.AdminFeaturesPageJspBean;
import fr.paris.lutece.portal.web.constants.Messages;
import fr.paris.lutece.util.html.HtmlTemplate;
Expand Down Expand Up @@ -138,7 +139,8 @@ public String doModifyUserTheme( HttpServletRequest request, HttpServletResponse
String strTheme = request.getParameter( PARAMETER_THEME );
String strForwardUrl = request.getParameter( PARAMETER_URL );

if ( !SecurityUtil.containsCleanParameters( request ) )
if ( !SecurityUtil.containsCleanParameters( request )
|| !SecurityUtil.isInternalRedirectUrlSafe( strForwardUrl, request ) )
{
return AppPathService.getBaseUrl( request );
}
Expand Down
81 changes: 81 additions & 0 deletions src/java/fr/paris/lutece/util/http/SecurityUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
*/
package fr.paris.lutece.util.http;

import fr.paris.lutece.portal.service.util.AppPathService;
import fr.paris.lutece.portal.service.util.AppPropertiesService;
import fr.paris.lutece.portal.web.LocalVariables;
import fr.paris.lutece.util.string.StringUtil;

Expand All @@ -43,6 +45,7 @@
import java.util.Enumeration;

import javax.servlet.http.HttpServletRequest;
import org.springframework.util.AntPathMatcher;

/**
* Security utils
Expand All @@ -61,6 +64,8 @@ public final class SecurityUtil
"..", "/", "\\"
};

public static final String PROPERTY_REDIRECT_URL_SAFE_PATTERNS = "lutece.security.redirectUrlSafePatterns";

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a commented out entry in the default propertied file, with an explanation as to what this property does, when to use it, etc.

// private static final String PATTERN_CLEAN_PARAMETER = "^[\\w/]+$+";

/**
Expand Down Expand Up @@ -266,6 +271,82 @@ public static String getRealIp( HttpServletRequest request )

return strIPAddress;
}

/**
* Validate a forward URL to avoid open redirect
* with url safe patterns found in properties
*
* @see SecurityUtil#isInternalRedirectUrlSafe(java.lang.String, javax.servlet.http.HttpServletRequest, java.lang.String)
*
* @param strUrl
* @param request
* @return true if valid
*/
public static boolean isInternalRedirectUrlSafe( String strUrl, HttpServletRequest request )
{
String strAntPathMatcherPatternsValues = AppPropertiesService.getProperty( SecurityUtil.PROPERTY_REDIRECT_URL_SAFE_PATTERNS );

return isInternalRedirectUrlSafe( strUrl, request, strAntPathMatcherPatternsValues );
}


/**
* Validate an internal redirect URL to avoid internal open redirect.
* (Use this function only if the use of internal url redirect keys is not possible.
* For external url redirection control, use the plugin plugin-verifybackurl)
*
* the url should :
* - not be blank (null or empty string or spaces)
* - not start with "http://" or "https://" or "//" OR match the base URL or any URL in the pattern list
*
* example with a base url "https://lutece.fr/ :
* - valid : myapp/jsp/site/Portal.jsp , Another.jsp , https://lutece.fr/myapp/jsp/site/Portal.jsp
* - invalid : http://anothersite.com , https://anothersite.com , //anothersite.com , file://my.txt , ...
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be turned into unit tests

*
* @param strUrl the Url to validate
* @param request the current request (containing the baseUrl)
* @param strAntPathMatcherPatterns a comma separated list of AntPathMatcher patterns, as "http://**.lutece.com,https://**.lutece.com"
* @return true if valid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When and why should I use that ?

*/
public static boolean isInternalRedirectUrlSafe( String strUrl, HttpServletRequest request, String strAntPathMatcherPatterns )
{

if ( StringUtils.isBlank( strUrl ) ) return true ; // this is not a valid redirect Url, but it is not unsafe

// filter schemes
if ( !strUrl.startsWith( "//" )
&& !strUrl.startsWith("http:" )
&& !strUrl.startsWith("https:" )
&& !strUrl.contains( "://" )
&& !strUrl.startsWith("javascript:" ) )
return true ; // should be a relative path

// compare with current baseUrl
if ( strUrl.startsWith( AppPathService.getBaseUrl( request ) ) )
return true ;

// compare with allowed url patterns
if ( !StringUtils.isBlank( strAntPathMatcherPatterns ) )
{
AntPathMatcher pathMatcher = new AntPathMatcher();

String[] strAntPathMatcherPatternsTab = strAntPathMatcherPatterns.split( CONSTANT_COMMA ) ;
for ( String pattern : strAntPathMatcherPatternsTab )
{
if ( pattern != null && pathMatcher.match( pattern , strUrl ) )
return true ;
}
}


// the Url does not match the allowed patterns
Logger logger = Logger.getLogger( LOGGER_NAME );
logger.warn( "SECURITY WARNING : OPEN_REDIRECT DETECTED : " + dumpRequest( request ) );

return false;

}

/**
* Identify user data saved in log files to prevent Log Forging attacks
Expand Down
63 changes: 63 additions & 0 deletions src/test/java/fr/paris/lutece/util/http/SecurityUtilTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,67 @@ public void testContainsCleanParameters( )
request.setParameter( "param4", "}" );
assertTrue( SecurityUtil.containsCleanParameters( request ) );
}


/**
* Test of isRedirectUrlSafe method, of class SecurityUtil, to avoid open redirect
*/
@Test
public void testIsRedirectUrlSafe( )
{
System.out.println( "isRedirectUrlSafe" );

MockHttpServletRequest request = new MockHttpServletRequest( );

// Assert False
String strUrl = "http://anothersite.com";
request.setParameter( "url", strUrl );
assertFalse( SecurityUtil.isRedirectUrlSafe( strUrl, request ) );

strUrl = "//anothersite.com";
request.setParameter( "url", strUrl );
assertFalse( SecurityUtil.isRedirectUrlSafe( strUrl, request ) );

strUrl = "file://my.txt";
request.setParameter( "url", strUrl );
assertFalse( SecurityUtil.isRedirectUrlSafe( strUrl, request ) );

strUrl = "javascript:alert('hello');";
request.setParameter( "url", strUrl );
assertFalse( SecurityUtil.isRedirectUrlSafe( strUrl, request ) );

strUrl = "opera-http://anothersite.com";
request.setParameter( "url", strUrl );
assertFalse( SecurityUtil.isRedirectUrlSafe( strUrl, request ) );

strUrl = "http://another.subdomain.mylutece.com";
request.setParameter( "url", strUrl );
String strUrlPatterns="http://**.lutece.com,https://**.lutece.com";
assertFalse( SecurityUtil.isRedirectUrlSafe( strUrl, request, strUrlPatterns ) );

// Assert True
strUrl = null;
assertTrue( SecurityUtil.isRedirectUrlSafe( strUrl, request) );

strUrl = "";
assertTrue( SecurityUtil.isRedirectUrlSafe( strUrl, request) );

strUrl = "/jsp/site/Portal.jsp";
request.setParameter( "url", strUrl );
assertTrue( SecurityUtil.isRedirectUrlSafe( strUrl, request ) );

strUrl = "Another.jsp";
request.setParameter( "url", strUrl );
assertTrue( SecurityUtil.isRedirectUrlSafe( strUrl, request ) );

strUrl = "http://localhost/myapp/jsp/site/Portal.jsp";
request.setParameter( "url", strUrl );
assertTrue( SecurityUtil.isRedirectUrlSafe( strUrl, request ) );

strUrl = "http://another.subdomain.lutece.com";
request.setParameter( "url", strUrl );
strUrlPatterns="http://**.lutece.com/**,https://**.lutece.com/**";
assertTrue( SecurityUtil.isRedirectUrlSafe( strUrl, request, strUrlPatterns ) );

}
}
9 changes: 9 additions & 0 deletions webapp/WEB-INF/conf/lutece.properties
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,15 @@ askPasswordReinitialization.admin.level=0
input.xss.characters=<>#"
xss.error.message= Les caract\u00e8res &lt; &gt; # &amp; et &quot; sont interdits dans le contenu de votre message.

################################################################################
# Internal Redirect URL safe patterns
#
# The list of "safe" AntPathMatcher patterns used by SecurityUtil.isReturnURLSafe function
# to avoid Open Redirect attacks.
# This should be a comma separated list of AntPathMatcher patterns,
# ex:
#lutece.security.redirectUrlSafePatterns=http://**.lutece.com/**,https://**.lutece.com/**

################################################################################
# Paginators
#
Expand Down