From 8d51832643e60c6b60b3280febcdb61c23278989 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Sun, 13 Jan 2019 19:00:38 +0200 Subject: [PATCH 1/3] [JENKINS-55557] Add support for state parameter which mitigates CSRF attacks when using the authorization code grant flow of oAuth2 --- .../plugins/GithubSecurityRealm.java | 37 ++++++++++++++++++- .../GithubAccessTokenPropertyTest.java | 3 +- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java index cfef0f6d..f4772f7d 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java @@ -88,7 +88,9 @@ of this software and associated documentation files (the "Software"), to deal import java.io.IOException; import java.net.InetSocketAddress; import java.net.Proxy; +import java.security.SecureRandom; import java.util.Arrays; +import java.util.Base64; import java.util.HashSet; import java.util.Set; import java.util.logging.Logger; @@ -337,6 +339,7 @@ public String getOauthScopes() { public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter String from, @Header("Referer") final String referer) throws IOException { + final String state = getSecureRandomString(); String redirectOnFinish; if (from != null && Util.isSafeToRedirectTo(from)) { redirectOnFinish = from; @@ -347,6 +350,7 @@ public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter Stri } request.getSession().setAttribute(REFERER_ATTRIBUTE, redirectOnFinish); + request.getSession().setAttribute(STATE_ATTRIBUTE, state); Set scopes = new HashSet<>(); for (GitHubOAuthScope s : getJenkins().getExtensionList(GitHubOAuthScope.class)) { @@ -354,11 +358,11 @@ public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter Stri } String suffix=""; if (!scopes.isEmpty()) { - suffix = "&scope="+Util.join(scopes,","); + suffix = "&scope="+Util.join(scopes,",")+"&state="+state; } else { // We need repo scope in order to access private repos // See https://developer.github.com/v3/oauth/#scopes - suffix = "&scope=" + oauthScopes; + suffix = "&scope=" + oauthScopes +"&state="+state; } return new HttpRedirect(githubWebUri + "/login/oauth/authorize?client_id=" @@ -372,13 +376,27 @@ public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter Stri public HttpResponse doFinishLogin(StaplerRequest request) throws IOException { String code = request.getParameter("code"); + String state = request.getParameter(STATE_ATTRIBUTE); String referer = (String)request.getSession().getAttribute(REFERER_ATTRIBUTE); + String expectedState = (String)request.getSession().getAttribute(STATE_ATTRIBUTE); if (code == null || code.trim().length() == 0) { Log.info("doFinishLogin: missing code."); return HttpResponses.redirectToContextRoot(); } + if (state == null){ + Log.info("doFinishLogin: missing state parameter from Github response."); + return HttpResponses.redirectToContextRoot(); + } else if (expectedState == null){ + Log.info("doFinishLogin: missing state parameter from user's session."); + return HttpResponses.redirectToContextRoot(); + } else if (!state.equals(expectedState)){ + Log.info("state parameter value ["+state+"] does not match the expected one ["+expectedState+"]"); + return HttpResponses.redirectToContextRoot(); + } + + String accessToken = getAccessToken(code); if (accessToken != null && accessToken.trim().length() > 0) { @@ -460,6 +478,16 @@ private String getAccessToken(@Nonnull String code) throws IOException { return null; } + /** + * Generates a random 20 byte String that conforms to the specification + * requirements + * @return a string that can be used as a state parameter + */ + private String getSecureRandomString() { + final byte[] bytes = new byte[20]; + SECURE_RANDOM.nextBytes(bytes); + return BASE64_ENCODER.encodeToString(bytes); + } /** * Returns the proxy to be used when connecting to the given URI. */ @@ -789,6 +817,11 @@ static Jenkins getJenkins() { private static final Logger LOGGER = Logger.getLogger(GithubSecurityRealm.class.getName()); private static final String REFERER_ATTRIBUTE = GithubSecurityRealm.class.getName()+".referer"; + private static final String STATE_ATTRIBUTE = "state"; + + private static final SecureRandom SECURE_RANDOM = new SecureRandom(); + + private static final Base64.Encoder BASE64_ENCODER = Base64.getUrlEncoder().withoutPadding(); /** * Asks for the password. diff --git a/src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertyTest.java b/src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertyTest.java index 2ef7d9a9..e407e4f9 100644 --- a/src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertyTest.java +++ b/src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertyTest.java @@ -245,7 +245,8 @@ private void onUserTeams(HttpServletRequest req, HttpServletResponse resp) throw private void onLoginOAuthAuthorize(HttpServletRequest req, HttpServletResponse resp) throws IOException { String code = "test"; - resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code); + String state = req.getParameter("state"); + resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code+"&state="+state); } private void onLoginOAuthAccessToken(HttpServletRequest req, HttpServletResponse resp) throws IOException { From df16ae3d33091470dc40921aa7655a00d85a3210 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 14 Jan 2019 08:05:39 +0200 Subject: [PATCH 2/3] Remove the dependency to java.util.Base64 Generate a random string satisfying the specification's requirements without using Base64 so that the plugin can be used with installations using Java 7 --- .../plugins/GithubSecurityRealm.java | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java index f4772f7d..5124323d 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java @@ -90,7 +90,6 @@ of this software and associated documentation files (the "Software"), to deal import java.net.Proxy; import java.security.SecureRandom; import java.util.Arrays; -import java.util.Base64; import java.util.HashSet; import java.util.Set; import java.util.logging.Logger; @@ -339,7 +338,9 @@ public String getOauthScopes() { public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter String from, @Header("Referer") final String referer) throws IOException { - final String state = getSecureRandomString(); + // https://tools.ietf.org/html/rfc6749#section-10.10 dictates that probability that an attacker guesses the string + // SHOULD be less than or equal to 2^(-160) and our Strings consist of 65 chars. (65^27 ~= 2^160) + final String state = getSecureRandomString(27); String redirectOnFinish; if (from != null && Util.isSafeToRedirectTo(from)) { redirectOnFinish = from; @@ -479,14 +480,19 @@ private String getAccessToken(@Nonnull String code) throws IOException { } /** - * Generates a random 20 byte String that conforms to the specification - * requirements - * @return a string that can be used as a state parameter + * Generates a random URL Safe String of n characters */ - private String getSecureRandomString() { - final byte[] bytes = new byte[20]; - SECURE_RANDOM.nextBytes(bytes); - return BASE64_ENCODER.encodeToString(bytes); + private String getSecureRandomString(int n) { + if (n < 0){ + throw new IllegalArgumentException("Length must be a positive integer"); + } + // See RFC3986 + final String urlSafeChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789.-_"; + final StringBuilder sb = new StringBuilder(); + for (int i = 0; i < n ; i++){ + sb.append(urlSafeChars.charAt(SECURE_RANDOM.nextInt(urlSafeChars.length()))); + } + return sb.toString(); } /** * Returns the proxy to be used when connecting to the given URI. @@ -821,8 +827,6 @@ static Jenkins getJenkins() { private static final SecureRandom SECURE_RANDOM = new SecureRandom(); - private static final Base64.Encoder BASE64_ENCODER = Base64.getUrlEncoder().withoutPadding(); - /** * Asks for the password. */ From 962a063c684b31bf9489233f2648b753f8ab0664 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 14 Jan 2019 08:16:08 +0200 Subject: [PATCH 3/3] Fix sessionFixation test --- .../jenkinsci/plugins/GithubAccessTokenPropertySEC797Test.java | 3 ++- .../org/jenkinsci/plugins/GithubAccessTokenPropertyTest.java | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertySEC797Test.java b/src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertySEC797Test.java index 369bc228..6743b81a 100644 --- a/src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertySEC797Test.java +++ b/src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertySEC797Test.java @@ -249,7 +249,8 @@ private void onUserTeams(HttpServletRequest req, HttpServletResponse resp) throw private void onLoginOAuthAuthorize(HttpServletRequest req, HttpServletResponse resp) throws IOException { String code = "test"; - resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code); + String state = req.getParameter("state"); + resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code + "&state=" + state); } private void onLoginOAuthAccessToken(HttpServletRequest req, HttpServletResponse resp) throws IOException { diff --git a/src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertyTest.java b/src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertyTest.java index e407e4f9..e565c1d9 100644 --- a/src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertyTest.java +++ b/src/test/java/org/jenkinsci/plugins/GithubAccessTokenPropertyTest.java @@ -246,7 +246,7 @@ private void onUserTeams(HttpServletRequest req, HttpServletResponse resp) throw private void onLoginOAuthAuthorize(HttpServletRequest req, HttpServletResponse resp) throws IOException { String code = "test"; String state = req.getParameter("state"); - resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code+"&state="+state); + resp.sendRedirect(jenkinsRule.getURL() + "securityRealm/finishLogin?code=" + code + "&state=" + state); } private void onLoginOAuthAccessToken(HttpServletRequest req, HttpServletResponse resp) throws IOException {