From 28e69c377ffb10fa5a013dec070d321669dc18d8 Mon Sep 17 00:00:00 2001
From: Harry Smith <hsmith@greshamtech.com>
Date: Mon, 17 Feb 2020 08:36:39 +0000
Subject: [PATCH] DWN-31929 : mitigate open id common XSS vulnerability

---
 openid-connect-common/pom.xml                 | 44 +++++++++++++++
 .../connect/web/UserInfoInterceptor.java      | 55 ++++++++++++++++++-
 .../web/UserInfoInterceptorSpec.groovy        | 41 ++++++++++++++
 pom.xml                                       | 23 ++++++++
 4 files changed, 160 insertions(+), 3 deletions(-)
 create mode 100644 openid-connect-common/src/test/groovy/org/mitre/openid/connect/web/UserInfoInterceptorSpec.groovy

diff --git a/openid-connect-common/pom.xml b/openid-connect-common/pom.xml
index bc910d1cd..79123a854 100644
--- a/openid-connect-common/pom.xml
+++ b/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-all</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.7.0</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.18.1</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>
diff --git a/openid-connect-common/src/main/java/org/mitre/openid/connect/web/UserInfoInterceptor.java b/openid-connect-common/src/main/java/org/mitre/openid/connect/web/UserInfoInterceptor.java
index 584b435ff..43d9c8591 100644
--- a/openid-connect-common/src/main/java/org/mitre/openid/connect/web/UserInfoInterceptor.java
+++ b/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;
@@ -78,9 +81,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) {
-					request.setAttribute("userInfo", oidc.getUserInfo());
-					request.setAttribute("userInfoJson", oidc.getUserInfo().toJson());
+				UserInfo userInfo = oidc.getUserInfo();
+				if (userInfo != null) {
+					santiseUserInfo(userInfo);
+					request.setAttribute("userInfo", userInfo);
+					request.setAttribute("userInfoJson", userInfo.toJson());
 				} else {
 					request.setAttribute("userInfo", null);
 					request.setAttribute("userInfoJson", "null");
@@ -94,6 +99,7 @@ public class UserInfoInterceptor extends HandlerInterceptorAdapter {
 
 					// if we have one, inject it so views can use it
 					if (user != null) {
+						santiseUserInfo(user);
 						request.setAttribute("userInfo", user);
 						request.setAttribute("userInfoJson", user.toJson());
 					}
@@ -104,4 +110,47 @@ public class UserInfoInterceptor extends HandlerInterceptorAdapter {
 		return true;
 	}
 
+	private UserInfo santiseUserInfo(final UserInfo userInfo) {
+		userInfo.setSub(nullCheckClean(userInfo.getSub()));
+		userInfo.setPreferredUsername(nullCheckClean(userInfo.getPreferredUsername()));
+		userInfo.setName(nullCheckClean(userInfo.getName()));
+		userInfo.setGivenName(nullCheckClean(userInfo.getGivenName()));
+		userInfo.setFamilyName(nullCheckClean(userInfo.getFamilyName()));
+		userInfo.setMiddleName(nullCheckClean(userInfo.getMiddleName()));
+		userInfo.setNickname(nullCheckClean(userInfo.getNickname()));
+		userInfo.setProfile(nullCheckClean(userInfo.getProfile()));
+		userInfo.setPicture(nullCheckClean(userInfo.getPicture()));
+		userInfo.setWebsite(nullCheckClean(userInfo.getWebsite()));
+		userInfo.setEmail(nullCheckClean(userInfo.getEmail()));
+		userInfo.setGender(nullCheckClean(userInfo.getGender()));
+		userInfo.setLocale(nullCheckClean(userInfo.getLocale()));
+		userInfo.setPhoneNumber(nullCheckClean(userInfo.getPhoneNumber()));
+		userInfo.setUpdatedTime(nullCheckClean(userInfo.getUpdatedTime()));
+		userInfo.setBirthdate(nullCheckClean(userInfo.getBirthdate()));
+
+		Address userInfoAddress = userInfo.getAddress();
+		if (userInfoAddress != null) {
+			userInfoAddress.setFormatted(nullCheckClean(userInfoAddress.getFormatted()));
+			userInfoAddress.setStreetAddress(nullCheckClean(userInfoAddress.getStreetAddress()));
+			userInfoAddress.setLocality(nullCheckClean(userInfoAddress.getLocality()));
+			userInfoAddress.setRegion(nullCheckClean(userInfoAddress.getRegion()));
+			userInfoAddress.setPostalCode(nullCheckClean(userInfoAddress.getPostalCode()));
+			userInfoAddress.setCountry(nullCheckClean(userInfoAddress.getCountry()));
+			userInfo.setAddress(userInfoAddress);
+		}
+
+		return userInfo;
+	}
+	
+	private String nullCheckClean(String elementToClean) {
+		final Whitelist whitelist = Whitelist.relaxed()
+			.removeTags("a")
+			.removeProtocols("img", "src", "http", "https");
+		
+		if (elementToClean != null) {
+			return Jsoup.clean(elementToClean, whitelist);
+		}
+		return null;
+	}
+
 }
diff --git a/openid-connect-common/src/test/groovy/org/mitre/openid/connect/web/UserInfoInterceptorSpec.groovy b/openid-connect-common/src/test/groovy/org/mitre/openid/connect/web/UserInfoInterceptorSpec.groovy
new file mode 100644
index 000000000..9bea3e7c0
--- /dev/null
+++ b/openid-connect-common/src/test/groovy/org/mitre/openid/connect/web/UserInfoInterceptorSpec.groovy
@@ -0,0 +1,41 @@
+package org.mitre.openid.connect.web
+
+import org.mitre.openid.connect.model.DefaultUserInfo
+import org.mitre.openid.connect.model.UserInfo
+import spock.lang.Specification
+
+class UserInfoInterceptorSpec extends Specification {
+
+	private def userInfoInterceptor = new UserInfoInterceptor()
+
+	// CVE-2020-5497 -> https://github.com/mitreid-connect/OpenID-Connect-Java-Spring-Server/issues/1521
+	def 'User Info is sanitised before making it back to the webpage'() {
+		given: 'A user name with a malicious payload'
+
+		UserInfo userInfo = new DefaultUserInfo()
+		userInfo.setSub('12318767')
+		userInfo.setName("Test</script><script>alert(1)</script> Test")
+		userInfo.setPreferredUsername('Test')
+		userInfo.setGivenName("Test</script><script>alert(1)</script>")
+		userInfo.setFamilyName('Test')
+		userInfo.setEmail('test@test.com')
+		userInfo.setEmailVerified(true)
+
+		when: 'The user info object is passed through the sanitise method'
+
+		UserInfo sanitisedUserInfo = userInfoInterceptor.santiseUserInfo(userInfo)
+
+		then: 'The malicious names have been sanitised'
+
+		sanitisedUserInfo.getName() == 'Test Test'
+		sanitisedUserInfo.getGivenName() == 'Test'
+
+		and: 'The non malicious elements have been unaffected'
+
+		sanitisedUserInfo.getSub() == '12318767'
+		sanitisedUserInfo.getPreferredUsername() == 'Test'
+		sanitisedUserInfo.getFamilyName() == 'Test'
+		sanitisedUserInfo.getEmail() == 'test@test.com'
+	}
+
+}
diff --git a/pom.xml b/pom.xml
index 1347b72e2..a11af7e5c 100644
--- a/pom.xml
+++ b/pom.xml
@@ -520,6 +520,24 @@
 				<version>1.9.5</version>
 				<scope>test</scope>
 			</dependency>
+			<dependency>
+				<groupId>org.codehaus.groovy</groupId>
+				<artifactId>groovy-all</artifactId>
+				<version>2.4.13</version>
+				<scope>test</scope>
+			</dependency>
+			<dependency>
+				<groupId>org.spockframework</groupId>
+				<artifactId>spock-core</artifactId>
+				<version>1.1-groovy-2.4</version>
+				<scope>test</scope>
+				<exclusions>
+					<exclusion>
+						<groupId>org.codehaus.groovy</groupId>
+						<artifactId>groovy-all</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>