diff --git a/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java index cfef0f6d..5124323d 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/GithubSecurityRealm.java @@ -88,6 +88,7 @@ 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.HashSet; import java.util.Set; @@ -337,6 +338,9 @@ public String getOauthScopes() { public HttpResponse doCommenceLogin(StaplerRequest request, @QueryParameter String from, @Header("Referer") final String referer) throws IOException { + // 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; @@ -347,6 +351,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 +359,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 +377,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 +479,21 @@ private String getAccessToken(@Nonnull String code) throws IOException { return null; } + /** + * Generates a random URL Safe String of n characters + */ + 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. */ @@ -789,6 +823,9 @@ 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(); /** * Asks for the password. 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 2ef7d9a9..e565c1d9 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 {