Lutece 2206 #144

seboo · 2018-08-07T16:16:19Z

Avoid open redirect when modifying a theme

rzara · 2018-08-08T09:06:30Z

src/java/fr/paris/lutece/util/http/SecurityUtil.java

+
+        if ( ( strForwardUrl.startsWith("//") || strForwardUrl.contains("://") ) 
+                && !strForwardUrl.startsWith ( AppPathService.getBaseUrl( request ) ) )  {
+            return false;


You should log here as is done in the other methods of this class

rzara · 2018-08-08T09:06:56Z

src/java/fr/paris/lutece/util/http/SecurityUtil.java

+     * example with a base url "https://lutece.fr/ :
+     *      - valid : /jsp/site/Portal.jsp , Another.jsp , https://lutece.fr/jsp/site/Portal.jsp
+     *      - invalid : http://anothersite.com , https://anothersite.com , //anothersite.com , file://my.txt , ...
+     * 


These should be turned into unit tests

rzara · 2018-08-08T09:07:40Z

src/java/fr/paris/lutece/util/http/SecurityUtil.java

+        if ( StringUtils.isBlank( strForwardUrl) ) return false ;
+
+        if ( ( strForwardUrl.startsWith("//") || strForwardUrl.contains("://") ) 
+                && !strForwardUrl.startsWith ( AppPathService.getBaseUrl( request ) ) )  {


The placement of { is not per Lutece coding style

rzara · 2018-08-08T09:08:12Z

src/java/fr/paris/lutece/util/http/SecurityUtil.java

+
+        if ( StringUtils.isBlank( strForwardUrl) ) return false ;
+
+        if ( ( strForwardUrl.startsWith("//") || strForwardUrl.contains("://") ) 


Per the Lutece coding style, you should put spaces around methods args

rzara · 2018-08-08T09:08:53Z

src/java/fr/paris/lutece/util/http/SecurityUtil.java

+     * 
+     * @param strForwardUrl
+     * @param request
+     * @return 


These javadoc tags are incomplete

rzara · 2018-08-08T09:09:12Z

src/java/fr/paris/lutece/portal/web/style/ThemesJspBean.java

@@ -138,7 +138,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.isForwardUrlValid( strForwardUrl, request) )


Missing space after request

seboo · 2018-08-08T12:34:59Z

done !

________________________________ De : rzara [notifications@github.com] Envoyé : mercredi 8 août 2018 11:10 À : lutece-platform/lutece-core Cc : Seb Leridon; Author Objet : Re: [lutece-platform/lutece-core] Lutece 2206 (#144) @rzara commented on this pull request.

________________________________ In src/java/fr/paris/lutece/util/http/SecurityUtil.java<#144 (comment)>:

+ * example with a base url "https://lutece.fr/ :

+ * - valid : /jsp/site/Portal.jsp , Another.jsp , https://lutece.fr/jsp/site/Portal.jsp + * - invalid : http://anothersite.com , https://anothersite.com , //anothersite.com , file://my.txt , ... + * + * @param strForwardUrl + * @param request + * @return + */ + public static boolean isForwardUrlValid( String strForwardUrl, HttpServletRequest request ) + { + + if ( StringUtils.isBlank( strForwardUrl) ) return false ; + + if ( ( strForwardUrl.startsWith("//") || strForwardUrl.contains("://") ) + && !strForwardUrl.startsWith ( AppPathService.getBaseUrl( request ) ) ) { + return false; You should log here as is done in the other methods of this class

________________________________ In src/java/fr/paris/lutece/util/http/SecurityUtil.java<#144 (comment)>:

@@ -266,6 +267,36 @@ public static String getRealIp( HttpServletRequest request )

return strIPAddress; } + + /** + * Validate a forward URL, + * the url should : + * - not be blank (null or empty string or spaces) + * - start with the base URL, or not start with "http" or "//" + * + * example with a base url "https://lutece.fr/ : + * - valid : /jsp/site/Portal.jsp , Another.jsp , https://lutece.fr/jsp/site/Portal.jsp + * - invalid : http://anothersite.com , https://anothersite.com , //anothersite.com , file://my.txt , ... + * These should be turned into unit tests

________________________________ In src/java/fr/paris/lutece/util/http/SecurityUtil.java<#144 (comment)>:

+ *

+ * example with a base url "https://lutece.fr/ : + * - valid : /jsp/site/Portal.jsp , Another.jsp , https://lutece.fr/jsp/site/Portal.jsp + * - invalid : http://anothersite.com , https://anothersite.com , //anothersite.com , file://my.txt , ... + * + * @param strForwardUrl + * @param request + * @return + */ + public static boolean isForwardUrlValid( String strForwardUrl, HttpServletRequest request ) + { + + if ( StringUtils.isBlank( strForwardUrl) ) return false ; + + if ( ( strForwardUrl.startsWith("//") || strForwardUrl.contains("://") ) + && !strForwardUrl.startsWith ( AppPathService.getBaseUrl( request ) ) ) { The placement of { is not per Lutece coding style

________________________________ In src/java/fr/paris/lutece/util/http/SecurityUtil.java<#144 (comment)>:

+ * - start with the base URL, or not start with "http" or "//"

+ * + * example with a base url "https://lutece.fr/ : + * - valid : /jsp/site/Portal.jsp , Another.jsp , https://lutece.fr/jsp/site/Portal.jsp + * - invalid : http://anothersite.com , https://anothersite.com , //anothersite.com , file://my.txt , ... + * + * @param strForwardUrl + * @param request + * @return + */ + public static boolean isForwardUrlValid( String strForwardUrl, HttpServletRequest request ) + { + + if ( StringUtils.isBlank( strForwardUrl) ) return false ; + + if ( ( strForwardUrl.startsWith("//") || strForwardUrl.contains("://") ) Per the Lutece coding style, you should put spaces around methods args

________________________________ In src/java/fr/paris/lutece/util/http/SecurityUtil.java<#144 (comment)>:

@@ -266,6 +267,36 @@ public static String getRealIp( HttpServletRequest request )

return strIPAddress; } + + /** + * Validate a forward URL, + * the url should : + * - not be blank (null or empty string or spaces) + * - start with the base URL, or not start with "http" or "//" + * + * example with a base url "https://lutece.fr/ : + * - valid : /jsp/site/Portal.jsp , Another.jsp , https://lutece.fr/jsp/site/Portal.jsp + * - invalid : http://anothersite.com , https://anothersite.com , //anothersite.com , file://my.txt , ... + * + * @param strForwardUrl + * @param request + * @return These javadoc tags are incomplete

________________________________ In src/java/fr/paris/lutece/portal/web/style/ThemesJspBean.java<#144 (comment)>:

@@ -138,7 +138,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.isForwardUrlValid( strForwardUrl, request) ) Missing space after request — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#144 (review)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AYwWrdTqdblHF9duez2mn8euZn3Zr6fEks5uOqrtgaJpZM4VyeBT>.

rzara · 2018-08-08T17:28:20Z

src/java/fr/paris/lutece/util/http/SecurityUtil.java

+     * @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


When and why should I use that ?

rzara · 2018-08-08T17:28:37Z

src/java/fr/paris/lutece/util/http/SecurityUtil.java

+    {
+
+        if ( StringUtils.isBlank( strUrl) ) return false ;
+	AntPathMatcher pathMatcher = new AntPathMatcher();


inconsistent alignment
Plus you do not use that until the else clause. this should be moved there

rzara · 2018-08-08T17:29:47Z

src/java/fr/paris/lutece/util/http/SecurityUtil.java

+            // relative path
+            return true ;
+        }
+        else 


No need for else since you return in the if

rzara · 2018-08-08T17:30:59Z

src/java/fr/paris/lutece/util/http/SecurityUtil.java

+            if ( !pathMatcher.matchStart( strUrl, AppPathService.getBaseUrl( request ) ) )
+                    return true ;
+
+            if ( strAntPathMatcherPatterns != null )


this is not covered by tests

rzara · 2018-08-08T17:31:57Z

src/java/fr/paris/lutece/util/http/SecurityUtil.java

    private static final String [ ] XXE_TERMS = {
            "!DOCTYPE", "!ELEMENT", "!ENTITY"
    };
    private static final String [ ] PATH_MANIPULATION = {
            "..", "/", "\\"
    };

+    public static final String PROPERTY_ANTPATHMATCHER_PATTERNS = "lutece.security.antpathmatcher.patterns";
+


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.

seboo changed the base branch from master to develop August 7, 2018 16:16

rzara reviewed Aug 8, 2018

View reviewed changes

seboo force-pushed the LUTECE-2206 branch from 11daa05 to 7a0c1ad Compare August 8, 2018 12:32

seboo force-pushed the LUTECE-2206 branch 2 times, most recently from bdf1133 to ba3b6f4 Compare August 8, 2018 15:47

rzara reviewed Aug 8, 2018

View reviewed changes

seboo force-pushed the LUTECE-2206 branch 2 times, most recently from 5338579 to c66d8fc Compare August 9, 2018 16:20

seboo added 2 commits August 28, 2018 15:35

LUTECE-2206 : Add method to SecurityUtil to avoid open redirect

56b7b60

LUTECE-2206 : avoid open redirect

d546be4

seboo force-pushed the LUTECE-2206 branch from c66d8fc to d546be4 Compare August 28, 2018 13:38

AlexandreClose force-pushed the develop branch from 748174d to a582507 Compare March 26, 2019 15:17

seboo force-pushed the develop branch 3 times, most recently from 135e4f9 to e399c1d Compare July 27, 2021 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lutece 2206 #144

Lutece 2206 #144

seboo commented Aug 7, 2018

rzara Aug 8, 2018

rzara Aug 8, 2018

rzara Aug 8, 2018

rzara Aug 8, 2018

rzara Aug 8, 2018

rzara Aug 8, 2018

seboo commented Aug 8, 2018 via email •

edited

Loading

rzara Aug 8, 2018

rzara Aug 8, 2018

rzara Aug 8, 2018

rzara Aug 8, 2018

rzara Aug 8, 2018


		if ( StringUtils.isBlank( strForwardUrl) ) return false ;

		if ( ( strForwardUrl.startsWith("//") \|\| strForwardUrl.contains("://") )

Lutece 2206 #144

Are you sure you want to change the base?

Lutece 2206 #144

Conversation

seboo commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seboo commented Aug 8, 2018 via email • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seboo commented Aug 8, 2018 via email •

edited

Loading