Merge pull request #5 from gresham-computing/sanitseUserName

Sanitse user name

Jenkinsfile

@@ -23,7 +23,7 @@ pipeline {
                 branch "1.3.x"
             }
             steps {
-                sh "mvn versions:set -DnewVersion=1.3.3.GRESHAM-18"
+                sh "mvn versions:set -DnewVersion=1.3.3.GRESHAM-19"
                 sh "mvn -N versions:update-child-modules"
 
                 timeout(time: 10, unit: 'MINUTES') {

openid-connect-common/pom.xml

@@ -87,6 +87,19 @@
 			<groupId>org.bouncycastle</groupId>
 			<artifactId>bcprov-jdk15on</artifactId>
 		</dependency>
+		<dependency>
+			<groupId>org.jsoup</groupId>
+			<artifactId>jsoup</artifactId>
+		</dependency>
+
+		<dependency>
+			<groupId>org.codehaus.groovy</groupId>
+			<artifactId>groovy</artifactId>
+		</dependency>
+		<dependency>
+			<groupId>org.spockframework</groupId>
+			<artifactId>spock-core</artifactId>
+		</dependency>
 	</dependencies>
 
 	<packaging>jar</packaging>
@@ -101,6 +114,37 @@
 					<target>${java-version}</target>
 				</configuration>
 			</plugin>
+			<plugin>
+				<groupId>org.codehaus.gmavenplus</groupId>
+				<artifactId>gmavenplus-plugin</artifactId>
+				<version>1.8.1</version>
+				<executions>
+					<execution>
+						<goals>
+							<goal>addTestStubSources</goal>
+							<goal>compileTests</goal>
+							<goal>removeTestStubs</goal>
+						</goals>
+					</execution>
+				</executions>
+				<configuration>
+					<stubsOutputDirectory>${project.build.directory}/generated-groovy-stubs</stubsOutputDirectory>
+					<testStubsOutputDirectory>${project.build.directory}/generated-groovy-test-stubs</testStubsOutputDirectory>
+				</configuration>
+			</plugin>
+			<plugin>
+				<groupId>org.apache.maven.plugins</groupId>
+				<artifactId>maven-surefire-plugin</artifactId>
+				<version>2.22.2</version>
+				<configuration>
+					<excludedGroups combine.self="override"/>
+					<testClassesDirectory>${project.build.testOutputDirectory}</testClassesDirectory>
+					<includes>
+						<include>**/*Test.java</include>
+						<include>**/*Spec.java</include>
+					</includes>
+				</configuration>
+			</plugin>
 			<!--&lt;!&ndash; BUILD SOURCE FILES &ndash;&gt;
 			<plugin>
 				<groupId>org.apache.maven.plugins</groupId>

openid-connect-common/src/main/java/org/mitre/openid/connect/web/UserInfoInterceptor.java

@@ -25,6 +25,9 @@ import java.lang.reflect.Type;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import org.jsoup.Jsoup;
+import org.jsoup.safety.Whitelist;
+import org.mitre.openid.connect.model.Address;
 import org.mitre.openid.connect.model.OIDCAuthenticationToken;
 import org.mitre.openid.connect.model.UserInfo;
 import org.mitre.openid.connect.service.UserInfoService;
@@ -51,6 +54,8 @@ import com.google.gson.JsonSerializer;
  */
 public class UserInfoInterceptor extends HandlerInterceptorAdapter {
 
+	private final Whitelist whitelist = Whitelist.none();
+
 	private Gson gson = new GsonBuilder()
 			.registerTypeHierarchyAdapter(GrantedAuthority.class, new JsonSerializer<GrantedAuthority>() {
 				@Override
@@ -78,9 +83,11 @@ public class UserInfoInterceptor extends HandlerInterceptorAdapter {
 			if (auth instanceof OIDCAuthenticationToken) {
 				// if they're logging into this server from a remote OIDC server, pass through their user info
 				OIDCAuthenticationToken oidc = (OIDCAuthenticationToken) auth;
-				if (oidc.getUserInfo() != null) {
+				UserInfo userInfo = oidc.getUserInfo();
-					request.setAttribute("userInfo", oidc.getUserInfo());
+				if (userInfo != null) {
-					request.setAttribute("userInfoJson", oidc.getUserInfo().toJson());
+					sanitiseUserInfo(userInfo);
+					request.setAttribute("userInfo", userInfo);
+					request.setAttribute("userInfoJson", userInfo.toJson());
 				} else {
 					request.setAttribute("userInfo", null);
 					request.setAttribute("userInfoJson", "null");
@@ -94,6 +101,7 @@ public class UserInfoInterceptor extends HandlerInterceptorAdapter {
 
 					// if we have one, inject it so views can use it
 					if (user != null) {
+						sanitiseUserInfo(user);
 						request.setAttribute("userInfo", user);
 						request.setAttribute("userInfoJson", user.toJson());
 					}
@@ -104,4 +112,42 @@ public class UserInfoInterceptor extends HandlerInterceptorAdapter {
 		return true;
 	}
 
+	private void sanitiseUserInfo(final UserInfo userInfo) {
+		userInfo.setSub(sanitise(userInfo.getSub()));
+		userInfo.setPreferredUsername(sanitise(userInfo.getPreferredUsername()));
+		userInfo.setName(sanitise(userInfo.getName()));
+		userInfo.setGivenName(sanitise(userInfo.getGivenName()));
+		userInfo.setFamilyName(sanitise(userInfo.getFamilyName()));
+		userInfo.setMiddleName(sanitise(userInfo.getMiddleName()));
+		userInfo.setNickname(sanitise(userInfo.getNickname()));
+		userInfo.setProfile(sanitise(userInfo.getProfile()));
+		userInfo.setPicture(sanitise(userInfo.getPicture()));
+		userInfo.setWebsite(sanitise(userInfo.getWebsite()));
+		userInfo.setEmail(sanitise(userInfo.getEmail()));
+		userInfo.setGender(sanitise(userInfo.getGender()));
+		userInfo.setLocale(sanitise(userInfo.getLocale()));
+		userInfo.setPhoneNumber(sanitise(userInfo.getPhoneNumber()));
+		userInfo.setUpdatedTime(sanitise(userInfo.getUpdatedTime()));
+		userInfo.setBirthdate(sanitise(userInfo.getBirthdate()));
+
+		Address userInfoAddress = userInfo.getAddress();
+		if (userInfoAddress != null) {
+			userInfoAddress.setFormatted(sanitise(userInfoAddress.getFormatted()));
+			userInfoAddress.setStreetAddress(sanitise(userInfoAddress.getStreetAddress()));
+			userInfoAddress.setLocality(sanitise(userInfoAddress.getLocality()));
+			userInfoAddress.setRegion(sanitise(userInfoAddress.getRegion()));
+			userInfoAddress.setPostalCode(sanitise(userInfoAddress.getPostalCode()));
+			userInfoAddress.setCountry(sanitise(userInfoAddress.getCountry()));
+			userInfo.setAddress(userInfoAddress);
+		}
+
+	}
+	
+	private String sanitise(String elementToClean) {
+		if (elementToClean != null) {
+			return Jsoup.clean(elementToClean, whitelist);
+		}
+		return null;
+	}
+
 }

openid-connect-common/src/test/groovy/org/mitre/openid/connect/web/UserInfoInterceptorSpec.groovy

@@ -0,0 +1,97 @@
+package org.mitre.openid.connect.web
+
+import org.mitre.openid.connect.model.DefaultUserInfo
+import org.mitre.openid.connect.model.UserInfo
+import spock.lang.Specification
+import spock.lang.Unroll
+
+class UserInfoInterceptorSpec extends Specification {
+
+	private def userInfoInterceptor = new UserInfoInterceptor()
+
+	// CVE-2020-5497 -> https://github.com/mitreid-connect/OpenID-Connect-Java-Spring-Server/issues/1521
+	@Unroll
+	def 'User Info is sanitised before making it back to the webpage with payload #payload'() {
+		given: 'A user name with a malicious payload'
+
+		UserInfo userInfo = new DefaultUserInfo()
+		userInfo.setSub('12318767')
+		userInfo.setName("Test" + payload + " Test")
+		userInfo.setPreferredUsername('Test')
+		userInfo.setGivenName("Test" + payload)
+		userInfo.setFamilyName('Test')
+		userInfo.setEmail('test@test.com')
+		userInfo.setEmailVerified(true)
+
+		when: 'The user info object is passed through the sanitise method'
+
+		userInfoInterceptor.sanitiseUserInfo(userInfo)
+
+		then: 'The malicious names have been sanitised'
+
+		userInfo.getName() == 'Test Test'
+		userInfo.getGivenName() == 'Test'
+
+		and: 'The non malicious elements have been unaffected'
+
+		userInfo.getSub() == '12318767'
+		userInfo.getPreferredUsername() == 'Test'
+		userInfo.getFamilyName() == 'Test'
+		userInfo.getEmail() == 'test@test.com'
+
+		where:
+
+		payload | _
+		"</script><script>alert(1)</script>" | _
+		"<body src=1 href=1 onerror=\"javascript:alert(1)\"></body>" | _
+		"<html onMouseWheel html onMouseWheel=\"javascript:javascript:alert(1)\"></html onMouseWheel>" | _
+		"<IMG SRC=`javascript:javascript:alert(1)`>" | _
+		"<script ~~~>alert(0%0)</script ~~~>" | _
+		"<IMG SRC=x onload=\"alert(String.fromCharCode(88,83,83))\">" | _
+		"<div STYLE=\"background-image: url(&#1;javascript:document.vulnerable=true;)\">" | _
+		"<BODY ONLOAD=javascript:alert(1)>" | _
+		"<iframe src=\"vbscript:document.vulnerable=true;\">" | _
+		"<br SIZE=\"&{document.vulnerable=true}\">" | _
+		"<img src=\"Mario Heiderich says that svg SHOULD not be executed trough image tags\" onerror=\"javascript:document.write('\\u003c\\u0069\\u0066\\u0072\\u0061\\u006d\\u0065\\u0020\\u0073\\u0072\\u0063\\u003d\\u0022\\u0064\\u0061\\u0074\\u0061\\u003a\\u0069\\u006d\\u0061\\u0067\\u0065\\u002f\\u0073\\u0076\\u0067\\u002b\\u0078\\u006d\\u006c\\u003b\\u0062\\u0061\\u0073\\u0065\\u0036\\u0034\\u002c\\u0050\\u0048\\u004e\\u0032\\u005a\\u0079\\u0042\\u0034\\u0062\\u0057\\u0078\\u0075\\u0063\\u007a\\u0030\\u0069\\u0061\\u0048\\u0052\\u0030\\u0063\\u0044\\u006f\\u0076\\u004c\\u0033\\u0064\\u0033\\u0064\\u0079\\u0035\\u0033\\u004d\\u0079\\u0035\\u0076\\u0063\\u006d\\u0063\\u0076\\u004d\\u006a\\u0041\\u0077\\u004d\\u0043\\u0039\\u007a\\u0064\\u006d\\u0063\\u0069\\u0050\\u0069\\u0041\\u0067\\u0043\\u0069\\u0041\\u0067\\u0049\\u0044\\u0078\\u0070\\u0062\\u0057\\u0046\\u006e\\u005a\\u0053\\u0042\\u0076\\u0062\\u006d\\u0078\\u0076\\u0059\\u0057\\u0051\\u0039\\u0049\\u006d\\u0046\\u0073\\u005a\\u0058\\u004a\\u0030\\u004b\\u0044\\u0045\\u0070\\u0049\\u006a\\u0034\\u0038\\u004c\\u0032\\u006c\\u0074\\u0059\\u0057\\u0064\\u006c\\u0050\\u0069\\u0041\\u0067\\u0043\\u0069\\u0041\\u0067\\u0049\\u0044\\u0078\\u007a\\u0064\\u006d\\u0063\\u0067\\u0062\\u0032\\u0035\\u0073\\u0062\\u0032\\u0046\\u006b\\u0050\\u0053\\u004a\\u0068\\u0062\\u0047\\u0056\\u0079\\u0064\\u0043\\u0067\\u0079\\u004b\\u0053\\u0049\\u002b\\u0050\\u0043\\u0039\\u007a\\u0064\\u006d\\u0063\\u002b\\u0049\\u0043\\u0041\\u004b\\u0049\\u0043\\u0041\\u0067\\u0050\\u0048\\u004e\\u006a\\u0063\\u006d\\u006c\\u0077\\u0064\\u0044\\u0035\\u0068\\u0062\\u0047\\u0056\\u0079\\u0064\\u0043\\u0067\\u007a\\u004b\\u0054\\u0077\\u0076\\u0063\\u0032\\u004e\\u0079\\u0061\\u0058\\u0042\\u0030\\u0050\\u0069\\u0041\\u0067\\u0043\\u0069\\u0041\\u0067\\u0049\\u0044\\u0078\\u006b\\u005a\\u0057\\u005a\\u007a\\u0049\\u0047\\u0039\\u0075\\u0062\\u0047\\u0039\\u0068\\u005a\\u0044\\u0030\\u0069\\u0059\\u0057\\u0078\\u006c\\u0063\\u006e\\u0051\\u006f\\u004e\\u0043\\u006b\\u0069\\u0050\\u006a\\u0077\\u0076\\u005a\\u0047\\u0056\\u006d\\u0063\\u007a\\u0034\\u0067\\u0049\\u0041\\u006f\\u0067\\u0049\\u0043\\u0041\\u0038\\u005a\\u0079\\u0042\\u0076\\u0062\\u006d\\u0078\\u0076\\u0059\\u0057\\u0051\\u0039\\u0049\\u006d\\u0046\\u0073\\u005a\\u0058\\u004a\\u0030\\u004b\\u0044\\u0055\\u0070\\u0049\\u006a\\u0034\\u0067\\u0049\\u0041\\u006f\\u0067\\u0049\\u0043\\u0041\\u0067\\u0049\\u0043\\u0041\\u0067\\u0050\\u0047\\u004e\\u0070\\u0063\\u006d\\u004e\\u0073\\u005a\\u0053\\u0042\\u0076\\u0062\\u006d\\u0078\\u0076\\u0059\\u0057\\u0051\\u0039\\u0049\\u006d\\u0046\\u0073\\u005a\\u0058\\u004a\\u0030\\u004b\\u0044\\u0059\\u0070\\u0049\\u0069\\u0041\\u0076\\u0050\\u0069\\u0041\\u0067\\u0043\\u0069\\u0041\\u0067\\u0049\\u0043\\u0041\\u0067\\u0049\\u0043\\u0041\\u0038\\u0064\\u0047\\u0056\\u0034\\u0064\\u0043\\u0042\\u0076\\u0062\\u006d\\u0078\\u0076\\u0059\\u0057\\u0051\\u0039\\u0049\\u006d\\u0046\\u0073\\u005a\\u0058\\u004a\\u0030\\u004b\\u0044\\u0063\\u0070\\u0049\\u006a\\u0034\\u0038\\u004c\\u0033\\u0052\\u006c\\u0065\\u0048\\u0051\\u002b\\u0049\\u0043\\u0041\\u004b\\u0049\\u0043\\u0041\\u0067\\u0050\\u0043\\u0039\\u006e\\u0050\\u0069\\u0041\\u0067\\u0043\\u006a\\u0077\\u0076\\u0063\\u0033\\u005a\\u006e\\u0050\\u0069\\u0041\\u0067\\u0022\\u003e\\u003c\\u002f\\u0069\\u0066\\u0072\\u0061\\u006d\\u0065\\u003e');\"></img>" | _
+	}
+
+	@Unroll
+	def 'User Info is santised to an extent to not produce an XSS with payload #payload'() {
+		given: 'A user name with a malicious payload'
+
+		UserInfo userInfo = new DefaultUserInfo()
+		userInfo.setSub('12318767')
+		userInfo.setName("Test" + payload + " Test")
+		userInfo.setPreferredUsername('Test')
+		userInfo.setGivenName("Test" + payload)
+		userInfo.setFamilyName('Test')
+		userInfo.setEmail('test@test.com')
+		userInfo.setEmailVerified(true)
+
+		when: 'The user info object is passed through the sanitise method'
+
+		userInfoInterceptor.sanitiseUserInfo(userInfo)
+
+		then: 'The malicious names have been sanitised'
+
+		userInfo.getName() == 'Test' + expectedResponse + ' Test'
+		userInfo.getGivenName() == 'Test' + expectedResponse
+
+		and: 'The non malicious elements have been unaffected'
+
+		userInfo.getSub() == '12318767'
+		userInfo.getPreferredUsername() == 'Test'
+		userInfo.getFamilyName() == 'Test'
+		userInfo.getEmail() == 'test@test.com'
+
+		where:
+
+		payload																										| expectedResponse
+		"'\"></title><script>alert(1111)</script>"																	| "'\"&gt;"
+		"'>//\\\\,<'>\">\">\"*\""																					| "'&gt;//\\\\,&lt;'&gt;\"&gt;\"&gt;\"*\""
+		"'\"\"><script language=\"JavaScript\"> alert('X \\nS \\nS');</script>"										| "'\"\"&gt;"
+		"!--\" /><script>alert('xss');</script>"																	| "!--\" /&gt;"
+		"\">/XaDoS/><script>alert(document.cookie)</script><script src=\"http://www.site.com/XSS.js\"></script>"	| "\"&gt;/XaDoS/&gt;"
+	}
+
+}

pom.xml

@@ -520,6 +520,24 @@
 				<version>1.9.5</version>
 				<scope>test</scope>
 			</dependency>
+			<dependency>
+				<groupId>org.codehaus.groovy</groupId>
+				<artifactId>groovy</artifactId>
+				<version>2.5.9</version>
+				<scope>test</scope>
+			</dependency>
+			<dependency>
+				<groupId>org.spockframework</groupId>
+				<artifactId>spock-core</artifactId>
+				<version>1.3-groovy-2.5</version>
+				<scope>test</scope>
+				<exclusions>
+					<exclusion>
+						<groupId>org.codehaus.groovy</groupId>
+						<artifactId>*</artifactId>
+					</exclusion>
+				</exclusions>
+			</dependency>
 			<!-- MITREid Connect components -->
 			<dependency>
 				<groupId>org.mitre</groupId>
@@ -607,6 +625,11 @@
 				<artifactId>wro4j-extensions</artifactId>
 				<version>1.8.0</version>
 			</dependency>
+			<dependency>
+				<groupId>org.jsoup</groupId>
+				<artifactId>jsoup</artifactId>
+				<version>1.10.3</version>
+			</dependency>
 		</dependencies>
 	</dependencyManagement>