You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@continuum.apache.org by oc...@apache.org on 2011/04/15 12:01:19 UTC
svn commit: r1092648 - in /continuum/branches/continuum-1.3.x: ./
continuum-webapp-test/src/test/testng/org/apache/continuum/web/test/
continuum-webapp/
continuum-webapp/src/main/java/org/apache/continuum/web/interceptor/
continuum-webapp/src/main/java...
Author: oching
Date: Fri Apr 15 10:01:18 2011
New Revision: 1092648
URL: http://svn.apache.org/viewvc?rev=1092648&view=rev
Log:
[CONTINUUM-2622]
o do an explicit check for a random generated value in the action on remove project group (built-in token session interceptor doesn't work for projectGroupSummary page because
the <s:action> tag (which executes result) for getting the projects in the group in the page causes a double submit
o enabled selenium test for remove project group csrf check
Added:
continuum/branches/continuum-1.3.x/continuum-webapp/src/main/java/org/apache/continuum/web/interceptor/
continuum/branches/continuum-1.3.x/continuum-webapp/src/main/java/org/apache/continuum/web/interceptor/ContinuumTokenSessionInterceptor.java
Modified:
continuum/branches/continuum-1.3.x/continuum-webapp-test/src/test/testng/org/apache/continuum/web/test/CSRFSecurityTest.java
continuum/branches/continuum-1.3.x/continuum-webapp/pom.xml
continuum/branches/continuum-1.3.x/continuum-webapp/src/main/java/org/apache/maven/continuum/web/action/ProjectGroupAction.java
continuum/branches/continuum-1.3.x/continuum-webapp/src/main/resources/localization/Continuum.properties
continuum/branches/continuum-1.3.x/continuum-webapp/src/main/resources/struts.xml
continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/confirmGroupRemoval.jsp
continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/groupSummary.jsp
continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/projectGroupSummary.jsp
continuum/branches/continuum-1.3.x/pom.xml
Modified: continuum/branches/continuum-1.3.x/continuum-webapp-test/src/test/testng/org/apache/continuum/web/test/CSRFSecurityTest.java
URL: http://svn.apache.org/viewvc/continuum/branches/continuum-1.3.x/continuum-webapp-test/src/test/testng/org/apache/continuum/web/test/CSRFSecurityTest.java?rev=1092648&r1=1092647&r2=1092648&view=diff
==============================================================================
--- continuum/branches/continuum-1.3.x/continuum-webapp-test/src/test/testng/org/apache/continuum/web/test/CSRFSecurityTest.java (original)
+++ continuum/branches/continuum-1.3.x/continuum-webapp-test/src/test/testng/org/apache/continuum/web/test/CSRFSecurityTest.java Fri Apr 15 10:01:18 2011
@@ -53,14 +53,13 @@ public class CSRFSecurityTest
assertTextPresent( "Possible CSRF attack detected! Invalid token found in the request." );
}
- /*
public void testCSRFRemoveProjectGroup()
{
getSelenium().open( baseUrl );
getSelenium().open( baseUrl + "/removeProjectGroup.action?projectGroupId=2" );
assertTextPresent( "Security Alert - Invalid Token Found" );
assertTextPresent( "Possible CSRF attack detected! Invalid token found in the request." );
- } */
+ }
public void testCSRFRemoveBuildResult()
{
Modified: continuum/branches/continuum-1.3.x/continuum-webapp/pom.xml
URL: http://svn.apache.org/viewvc/continuum/branches/continuum-1.3.x/continuum-webapp/pom.xml?rev=1092648&r1=1092647&r2=1092648&view=diff
==============================================================================
--- continuum/branches/continuum-1.3.x/continuum-webapp/pom.xml (original)
+++ continuum/branches/continuum-1.3.x/continuum-webapp/pom.xml Fri Apr 15 10:01:18 2011
@@ -448,6 +448,10 @@ under the License.
<artifactId>commons-io</artifactId>
</dependency>
<dependency>
+ <groupId>commons-codec</groupId>
+ <artifactId>commons-codec</artifactId>
+ </dependency>
+ <dependency>
<groupId>commons-fileupload</groupId>
<artifactId>commons-fileupload</artifactId>
<version>1.2.1</version>
Added: continuum/branches/continuum-1.3.x/continuum-webapp/src/main/java/org/apache/continuum/web/interceptor/ContinuumTokenSessionInterceptor.java
URL: http://svn.apache.org/viewvc/continuum/branches/continuum-1.3.x/continuum-webapp/src/main/java/org/apache/continuum/web/interceptor/ContinuumTokenSessionInterceptor.java?rev=1092648&view=auto
==============================================================================
--- continuum/branches/continuum-1.3.x/continuum-webapp/src/main/java/org/apache/continuum/web/interceptor/ContinuumTokenSessionInterceptor.java (added)
+++ continuum/branches/continuum-1.3.x/continuum-webapp/src/main/java/org/apache/continuum/web/interceptor/ContinuumTokenSessionInterceptor.java Fri Apr 15 10:01:18 2011
@@ -0,0 +1,76 @@
+package org.apache.continuum.web.interceptor;
+
+/*
+ * 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.
+ */
+
+import com.opensymphony.xwork2.ActionInvocation;
+import org.apache.struts2.ServletActionContext;
+import org.apache.struts2.interceptor.TokenInterceptor;
+import org.apache.struts2.interceptor.TokenSessionStoreInterceptor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.http.HttpServletRequest;
+
+/**
+ * ContinuumTokenSessionInterceptor allows the action to pass through if <i>explicitCSRFCheck</i> is
+ * enabled even if the TokenSessionStoreInterceptor returned invalid token error. If the <i>explicitCSRFCheck</i>
+ * parameter is <i>true</i>, it means that the CSRF check will be done in the action itself. Otherwise,
+ * whatever result is returned by the TokenSessionStoreInterceptor will just be propagated.
+ *
+ * This is to handle cases in Continuum where an <s:action> tag is also present in the page (example is projectGroupSummary.jsp)
+ * causing a double submit and the TokenSessionStoreInterceptor to fail even if the request was actually valid.
+ *
+ * @author: Maria Odea Ching <oc...@apache.org>
+ * @version:
+ * @plexus.component role="com.opensymphony.xwork2.interceptor.Interceptor" role-hint="continuumTokenSessionInterceptor"
+ */
+public class ContinuumTokenSessionInterceptor
+ extends TokenSessionStoreInterceptor
+{
+ private static final String EXPLICIT_CSRF_CHECK_PARAM = "explicitCSRFCheck";
+
+ private static final Logger logger = LoggerFactory.getLogger( ContinuumTokenSessionInterceptor.class );
+
+ @Override
+ protected String doIntercept( ActionInvocation invocation )
+ throws Exception
+ {
+ String returnedString = super.doIntercept( invocation );
+
+ logger.debug( "TokenSessionStoreInterceptor returned '" + returnedString + "'." );
+
+ if ( TokenInterceptor.INVALID_TOKEN_CODE.equalsIgnoreCase( returnedString ) )
+ {
+ HttpServletRequest request =
+ ( HttpServletRequest ) invocation.getInvocationContext().get( ServletActionContext.HTTP_REQUEST );
+
+ String explicitCSRFCheck = request.getParameter( EXPLICIT_CSRF_CHECK_PARAM );
+
+ if ( explicitCSRFCheck != null && Boolean.parseBoolean( explicitCSRFCheck ) )
+ {
+ logger.debug( "Explicit CSRF check flag set to true. Passing on the invocation.." );
+ return invocation.invoke();
+ }
+ }
+
+ return returnedString;
+ }
+
+}
Modified: continuum/branches/continuum-1.3.x/continuum-webapp/src/main/java/org/apache/maven/continuum/web/action/ProjectGroupAction.java
URL: http://svn.apache.org/viewvc/continuum/branches/continuum-1.3.x/continuum-webapp/src/main/java/org/apache/maven/continuum/web/action/ProjectGroupAction.java?rev=1092648&r1=1092647&r2=1092648&view=diff
==============================================================================
--- continuum/branches/continuum-1.3.x/continuum-webapp/src/main/java/org/apache/maven/continuum/web/action/ProjectGroupAction.java (original)
+++ continuum/branches/continuum-1.3.x/continuum-webapp/src/main/java/org/apache/maven/continuum/web/action/ProjectGroupAction.java Fri Apr 15 10:01:18 2011
@@ -19,6 +19,7 @@ package org.apache.maven.continuum.web.a
* under the License.
*/
+import java.security.SecureRandom;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -26,9 +27,12 @@ import java.util.Comparator;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedHashMap;
+import java.util.LinkedList;
import java.util.List;
import java.util.Map;
+import java.util.Random;
+import org.apache.commons.codec.binary.Base64;
import org.apache.commons.collections.ComparatorUtils;
import org.apache.commons.lang.StringUtils;
import org.apache.continuum.buildmanager.BuildManagerException;
@@ -46,6 +50,7 @@ import org.apache.maven.continuum.model.
import org.apache.maven.continuum.project.ContinuumProjectState;
import org.apache.maven.continuum.web.bean.ProjectGroupUserBean;
import org.apache.maven.continuum.web.exception.AuthorizationRequiredException;
+import org.apache.struts2.interceptor.TokenInterceptor;
import org.codehaus.plexus.redback.rbac.RBACManager;
import org.codehaus.plexus.redback.rbac.RbacManagerException;
import org.codehaus.plexus.redback.rbac.RbacObjectNotFoundException;
@@ -144,6 +149,21 @@ public class ProjectGroupAction
private List<ProjectScmRoot> projectScmRoots;
+ private Random randomizer;
+
+ private String encodedRandomVal;
+
+ private static List<String> encodedRandomValCache = new LinkedList();
+
+ private boolean explicitCSRFCheck = false;
+
+ private static final int CACHE_MAX_SIZE = 30;
+
+ public ProjectGroupAction()
+ {
+ randomizer = new SecureRandom();
+ }
+
public String summary()
throws ContinuumException
{
@@ -244,6 +264,19 @@ public class ProjectGroupAction
projectScmRoots = getContinuum().getProjectScmRootByProjectGroup( projectGroup.getId() );
}
+ // explicit csrf check for CONTINUUM-2622
+ encodedRandomVal = generateEncodedRandomVal();
+
+ synchronized( encodedRandomValCache )
+ {
+ // check size of cache first before adding anything in the cache
+ if( encodedRandomValCache.size() == CACHE_MAX_SIZE )
+ {
+ ( ( LinkedList ) encodedRandomValCache ).removeFirst();
+ }
+ encodedRandomValCache.add( decodeRandomVal( encodedRandomVal ) );
+ }
+
return SUCCESS;
}
@@ -315,6 +348,24 @@ public class ProjectGroupAction
}
else
{
+ // explicit CSRF check for CONTINUUM-2622 - need to explicitly implement for remove project group because <s:token/> doesn't work
+ // in project group summary as there is a <s:action> whose result is being executed in the page causing a double submission
+ if( explicitCSRFCheck )
+ {
+ if( StringUtils.isEmpty( encodedRandomVal ) || !encodedRandomValCache.contains( decodeRandomVal( encodedRandomVal ) ) )
+ {
+ logger.error( "Token not found in cache!" );
+ addActionError( getText( "projectGroup.remove.invalid.token", "Action not allowed to continue - invalid token found!" ) );
+ return TokenInterceptor.INVALID_TOKEN_CODE;
+ }
+ else
+ {
+ logger.info( "Token found in cache.." );
+ // remove it from the cache if found and let the action continue
+ encodedRandomValCache.remove( decodeRandomVal( encodedRandomVal ) );
+ }
+ }
+
name = getProjectGroupName();
return CONFIRM;
}
@@ -834,6 +885,42 @@ public class ProjectGroupAction
} );
}
+ protected String generateEncodedRandomVal()
+ {
+ String encodedRandomVale;
+
+ byte[] random = new byte[16];
+ randomizer.nextBytes( random );
+ byte[] all = new byte[17];
+
+ for( int i = 0; i < random.length; i++ )
+ {
+ all[i] = random[i];
+ }
+
+ // include time to ensure uniqueness
+ byte time = ( byte ) System.currentTimeMillis();
+ all[16] = time;
+
+ // encode as string
+ encodedRandomVale = Base64.encodeBase64String( all );
+
+ return encodedRandomVale;
+ }
+
+ protected String decodeRandomVal( String encodedRandomVal )
+ {
+ byte[] randomValInBytes = Base64.decodeBase64( encodedRandomVal );
+
+ String decodedRandomVal = "";
+ if( randomValInBytes != null )
+ {
+ decodedRandomVal = new String( randomValInBytes );
+ }
+
+ return decodedRandomVal;
+ }
+
public int getProjectGroupId()
{
return projectGroupId;
@@ -1101,4 +1188,24 @@ public class ProjectGroupAction
{
this.sorterProperty = sorterProperty;
}
+
+ public String getEncodedRandomVal()
+ {
+ return encodedRandomVal;
+ }
+
+ public void setEncodedRandomVal( String encodedRandomVal )
+ {
+ this.encodedRandomVal = encodedRandomVal;
+ }
+
+ public boolean isExplicitCSRFCheck()
+ {
+ return explicitCSRFCheck;
+ }
+
+ public void setExplicitCSRFCheck( boolean explicitCSRFCheck )
+ {
+ this.explicitCSRFCheck = explicitCSRFCheck;
+ }
}
Modified: continuum/branches/continuum-1.3.x/continuum-webapp/src/main/resources/localization/Continuum.properties
URL: http://svn.apache.org/viewvc/continuum/branches/continuum-1.3.x/continuum-webapp/src/main/resources/localization/Continuum.properties?rev=1092648&r1=1092647&r2=1092648&view=diff
==============================================================================
--- continuum/branches/continuum-1.3.x/continuum-webapp/src/main/resources/localization/Continuum.properties (original)
+++ continuum/branches/continuum-1.3.x/continuum-webapp/src/main/resources/localization/Continuum.properties Fri Apr 15 10:01:18 2011
@@ -198,6 +198,7 @@ projectGroup.scmRoot.title = Project Gro
projectGroup.scmRoot.label = Scm Root URL
projectGroup.cancelGroupBuild = Cancel Group Build
projectGroup.invalid.id = Invalid Project Group Id: {0}
+projectGroup.remove.invalid.token = Possible CSRF attack! Invalid token found in remove project group request.
# ----------------------------------------------------------------------
# Page: Project Group - Members
Modified: continuum/branches/continuum-1.3.x/continuum-webapp/src/main/resources/struts.xml
URL: http://svn.apache.org/viewvc/continuum/branches/continuum-1.3.x/continuum-webapp/src/main/resources/struts.xml?rev=1092648&r1=1092647&r2=1092648&view=diff
==============================================================================
--- continuum/branches/continuum-1.3.x/continuum-webapp/src/main/resources/struts.xml (original)
+++ continuum/branches/continuum-1.3.x/continuum-webapp/src/main/resources/struts.xml Fri Apr 15 10:01:18 2011
@@ -31,6 +31,7 @@
<interceptors>
<interceptor name="continuumConfigurationCheck" class="forceContinuumConfigurationInterceptor"/>
+ <interceptor name="continuumTokenSessionCheck" class="continuumTokenSessionInterceptor"/>
<interceptor name="redbackForceAdminUser" class="redbackForceAdminUserInterceptor"/>
<interceptor name="redbackSecureActions" class="redbackSecureActionInterceptor"/>
<interceptor name="redbackAutoLogin" class="redbackAutoLoginInterceptor"/>
@@ -47,7 +48,7 @@
</interceptor-ref>
<interceptor-ref name="redbackPolicyEnforcement"/>
<interceptor-ref name="continuumConfigurationCheck"/>
- <interceptor-ref name="tokenSession">
+ <interceptor-ref name="continuumTokenSessionCheck">
<param name="excludeMethods">*</param>
</interceptor-ref>
<interceptor-ref name="validation">
@@ -67,6 +68,9 @@
<interceptor-ref name="redbackSecureActions">
<param name="enableReferrerCheck">false</param>
</interceptor-ref>
+ <interceptor-ref name="continuumTokenSessionCheck">
+ <param name="excludeMethods">*</param>
+ </interceptor-ref>
<interceptor-ref name="validation">
<param name="excludeMethods">input,back,cancel,browse,edit</param>
</interceptor-ref>
@@ -366,7 +370,9 @@
</action>
<action name="removeProjectGroup" class="projectGroup" method="remove">
- <interceptor-ref name="storeStack"/>
+ <interceptor-ref name="storeStack">
+ <param name="continuumTokenSessionCheck.includeMethods">remove</param>
+ </interceptor-ref>
<result name="confirm">/WEB-INF/jsp/confirmGroupRemoval.jsp</result>
<result name="success" type="redirect-action">
<param name="actionName">groupSummary</param>
@@ -493,7 +499,7 @@
<result name="success" type="chain">schedules</result>
<result name="error" type="chain">schedule</result>
<interceptor-ref name="configuredContinuumStack">
- <param name="tokenSession.includeMethods">remove</param>
+ <param name="continuumTokenSessionCheck.includeMethods">remove</param>
</interceptor-ref>
</action>
@@ -977,7 +983,7 @@
<action name="removeRepository" class="localRepository" method="remove">
<interceptor-ref name="storeStack">
- <param name="tokenSession.includeMethods">remove</param>
+ <param name="continuumTokenSessionCheck.includeMethods">remove</param>
</interceptor-ref>
<result name="confirm">/WEB-INF/jsp/admin/confirmDeleteLocalRepository.jsp</result>
<result name="success" type="redirect-action">
@@ -1022,7 +1028,7 @@
<param name="actionName">purgeConfigList</param>
</result>
<interceptor-ref name="configuredContinuumStack">
- <param name="tokenSession.includeMethods">remove</param>
+ <param name="continuumTokenSessionCheck.includeMethods">remove</param>
</interceptor-ref>
</action>
Modified: continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/confirmGroupRemoval.jsp
URL: http://svn.apache.org/viewvc/continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/confirmGroupRemoval.jsp?rev=1092648&r1=1092647&r2=1092648&view=diff
==============================================================================
--- continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/confirmGroupRemoval.jsp (original)
+++ continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/confirmGroupRemoval.jsp Fri Apr 15 10:01:18 2011
@@ -31,6 +31,7 @@
<s:form action="removeProjectGroup" method="post">
<s:hidden name="projectGroupId"/>
<s:hidden name="confirmed" value="true"/>
+ <s:hidden name="explicitCSRFCheck" value="false"/>
<s:token/>
<s:actionerror/>
Modified: continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/groupSummary.jsp
URL: http://svn.apache.org/viewvc/continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/groupSummary.jsp?rev=1092648&r1=1092647&r2=1092648&view=diff
==============================================================================
--- continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/groupSummary.jsp (original)
+++ continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/groupSummary.jsp Fri Apr 15 10:01:18 2011
@@ -101,6 +101,7 @@
<s:param name="projectGroupId">${group.id}</s:param>
<s:param name="struts.token.name">struts.token</s:param>
<s:param name="struts.token"><s:property value="struts.token"/></s:param>
+ <s:param name="explicitCSRFCheck">false</s:param>
</s:url>
<s:a href="%{removeProjectGroupUrl}">
<img src="<s:url value='/images/delete.gif'/>" alt="<s:text name="projectGroup.deleteGroup"/>" title="<s:text name="projectGroup.deleteGroup"/>" border="0">
Modified: continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/projectGroupSummary.jsp
URL: http://svn.apache.org/viewvc/continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/projectGroupSummary.jsp?rev=1092648&r1=1092647&r2=1092648&view=diff
==============================================================================
--- continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/projectGroupSummary.jsp (original)
+++ continuum/branches/continuum-1.3.x/continuum-webapp/src/main/webapp/WEB-INF/jsp/projectGroupSummary.jsp Fri Apr 15 10:01:18 2011
@@ -171,6 +171,9 @@
<td>
<redback:ifAuthorized permission="continuum-remove-group" resource="${projectGroup.name}">
<form action="removeProjectGroup.action" method="post">
+ <s:token/>
+ <input type="hidden" name="encodedRandomVal" value="<s:property value="encodedRandomVal"/>"/>
+ <input type="hidden" name="explicitCSRFCheck" value="true"/>
<input type="hidden" name="projectGroupId" value="<s:property value="projectGroupId"/>"/>
<input type="submit" name="remove" value="<s:text name="projectGroup.deleteGroup"/>"/>
</form>
Modified: continuum/branches/continuum-1.3.x/pom.xml
URL: http://svn.apache.org/viewvc/continuum/branches/continuum-1.3.x/pom.xml?rev=1092648&r1=1092647&r2=1092648&view=diff
==============================================================================
--- continuum/branches/continuum-1.3.x/pom.xml (original)
+++ continuum/branches/continuum-1.3.x/pom.xml Fri Apr 15 10:01:18 2011
@@ -1087,7 +1087,12 @@ under the License.
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>1.4</version>
- </dependency>
+ </dependency>
+ <dependency>
+ <groupId>commons-codec</groupId>
+ <artifactId>commons-codec</artifactId>
+ <version>1.4</version>
+ </dependency>
<dependency>
<groupId>org.apache.struts</groupId>
<artifactId>struts2-core</artifactId>
Re: svn commit: r1092648 - in /continuum/branches/continuum-1.3.x: ./ continuum-webapp-test/src/test/testng/org/apache/continuum/web/test/ continuum-webapp/ continuum-webapp/src/main/java/org/apache/continuum/web/interceptor/ continuum-webapp/src/main/java...
Posted by Brett Porter <br...@apache.org>.
This seems like a complicated solution, and I don't quite understand the reason the problem existed with the previous solution. Was there a way the projectGroupSummary page could be restructured so that this wasn't an issue?
- Brett
On 15/04/2011, at 8:01 PM, oching@apache.org wrote:
> Author: oching
> Date: Fri Apr 15 10:01:18 2011
> New Revision: 1092648
>
> URL: http://svn.apache.org/viewvc?rev=1092648&view=rev
> Log:
> [CONTINUUM-2622]
> o do an explicit check for a random generated value in the action on remove project group (built-in token session interceptor doesn't work for projectGroupSummary page because
> the <s:action> tag (which executes result) for getting the projects in the group in the page causes a double submit
> o enabled selenium test for remove project group csrf check
--
Brett Porter
brett@apache.org
http://brettporter.wordpress.com/