github
/
OpenID-Connect-Java-Spring-Server
mirror of https://github.com/mitreid-connect/OpenID-Connect-Java-Spring-Server
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

DWN-31929 : mitigate open id common XSS vulnerability

pull/1601/head
Harry Smith 2020-02-17 08:36:39 +00:00
parent d451075fb3
commit 28e69c377f
4 changed files with 160 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
openid-connect-common/pom.xml
View File

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

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

@ -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;
}
}

41
openid-connect-common/src/test/groovy/org/mitre/openid/connect/web/UserInfoInterceptorSpec.groovy Normal file
View File

@ -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'
}
}

23
pom.xml
View File

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