From a253ebc9083f2d348be520274a65844a4386a2f2 Mon Sep 17 00:00:00 2001
From: Justin Richer <jricher@mit.edu>
Date: Tue, 13 May 2014 09:27:02 -0400
Subject: [PATCH] added CSRF protection to approval page

---
 .../src/main/webapp/WEB-INF/views/approve.jsp | 33 ++++++++++---------
 .../web/OAuthConfirmationController.java      |  5 +++
 .../token/TofuUserApprovalHandler.java        | 26 ++++++++++-----
 3 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/openid-connect-server-webapp/src/main/webapp/WEB-INF/views/approve.jsp b/openid-connect-server-webapp/src/main/webapp/WEB-INF/views/approve.jsp
index 2c97c143a..e632b916f 100644
--- a/openid-connect-server-webapp/src/main/webapp/WEB-INF/views/approve.jsp
+++ b/openid-connect-server-webapp/src/main/webapp/WEB-INF/views/approve.jsp
@@ -225,23 +225,24 @@
 			</div>
 
 			<div class="row">
-			<h3>
-					Do you authorize 
-					"<c:choose>
-						<c:when test="${empty client.clientName}">
-							<c:out value="${client.clientId}" />
-						</c:when>
-						<c:otherwise>
-							<c:out value="${client.clientName}" />
-						</c:otherwise>
-					</c:choose>"?
-			</h3>
+				<h3>
+						Do you authorize 
+						"<c:choose>
+							<c:when test="${empty client.clientName}">
+								<c:out value="${client.clientId}" />
+							</c:when>
+							<c:otherwise>
+								<c:out value="${client.clientName}" />
+							</c:otherwise>
+						</c:choose>"?
+				</h3>
 				<input id="user_oauth_approval" name="user_oauth_approval" value="true" type="hidden" /> 
-					<input name="authorize" value="Authorize" type="submit"
-					onclick="$('#user_oauth_approval').attr('value',true)" class="btn btn-success btn-large" /> 
-					&nbsp; 
-					<input name="deny" value="Deny" type="submit" onclick="$('#user_oauth_approval').attr('value',false)"
-					class="btn btn-secondary btn-large" />
+				<input name="csrf" value="${ csrf }" type="hidden" />
+				<input name="authorize" value="Authorize" type="submit"
+				onclick="$('#user_oauth_approval').attr('value',true)" class="btn btn-success btn-large" /> 
+				&nbsp; 
+				<input name="deny" value="Deny" type="submit" onclick="$('#user_oauth_approval').attr('value',false)"
+				class="btn btn-secondary btn-large" />
 			</div>
 
 		</form>
diff --git a/openid-connect-server/src/main/java/org/mitre/oauth2/web/OAuthConfirmationController.java b/openid-connect-server/src/main/java/org/mitre/oauth2/web/OAuthConfirmationController.java
index 057ce1cad..5f75648da 100644
--- a/openid-connect-server/src/main/java/org/mitre/oauth2/web/OAuthConfirmationController.java
+++ b/openid-connect-server/src/main/java/org/mitre/oauth2/web/OAuthConfirmationController.java
@@ -26,6 +26,7 @@ import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.UUID;
 
 import org.mitre.oauth2.model.ClientDetailsEntity;
 import org.mitre.oauth2.model.SystemScope;
@@ -192,6 +193,10 @@ public class OAuthConfirmationController {
 			model.put("gras", false);
 		}
 		
+		// inject a random value for CSRF purposes
+		String csrf = UUID.randomUUID().toString();
+		model.put("csrf", csrf);
+		authRequest.getExtensions().put("csrf", csrf);		
 		
 		return "approve";
 	}
diff --git a/openid-connect-server/src/main/java/org/mitre/openid/connect/token/TofuUserApprovalHandler.java b/openid-connect-server/src/main/java/org/mitre/openid/connect/token/TofuUserApprovalHandler.java
index 9b87dfca7..59239cf7d 100644
--- a/openid-connect-server/src/main/java/org/mitre/openid/connect/token/TofuUserApprovalHandler.java
+++ b/openid-connect-server/src/main/java/org/mitre/openid/connect/token/TofuUserApprovalHandler.java
@@ -96,11 +96,21 @@ public class TofuUserApprovalHandler implements UserApprovalHandler {
 			return true;
 		} else {
 			// if not, check to see if the user has approved it
-
-			// TODO: make parameter name configurable?
-			boolean approved = Boolean.parseBoolean(authorizationRequest.getApprovalParameters().get("user_oauth_approval"));
-
-			return userAuthentication.isAuthenticated() && approved;
+			if (Boolean.parseBoolean(authorizationRequest.getApprovalParameters().get("user_oauth_approval"))) {			// TODO: make parameter name configurable?
+				
+				// check the value of the CSRF parameter
+				
+				if (authorizationRequest.getExtensions().get("csrf") != null) {
+					if (authorizationRequest.getExtensions().get("csrf").equals(authorizationRequest.getApprovalParameters().get("csrf"))) {
+						
+						// make sure the user is actually authenticated
+						return userAuthentication.isAuthenticated();						
+					}
+				}
+			}
+			
+			// if the above doesn't pass, it's not yet approved
+			return false;
 		}
 
 	}
@@ -182,9 +192,9 @@ public class TofuUserApprovalHandler implements UserApprovalHandler {
 		ClientDetails client = clientDetailsService.loadClientByClientId(clientId);
 
 		// This must be re-parsed here because SECOAUTH forces us to call things in a strange order
-		boolean approved = Boolean.parseBoolean(authorizationRequest.getApprovalParameters().get("user_oauth_approval"));
-
-		if (approved) {
+		if (Boolean.parseBoolean(authorizationRequest.getApprovalParameters().get("user_oauth_approval"))
+				&& authorizationRequest.getExtensions().get("csrf") != null
+				&& authorizationRequest.getExtensions().get("csrf").equals(authorizationRequest.getApprovalParameters().get("csrf"))) {
 
 			authorizationRequest.setApproved(true);