diff --git a/src/main/java/hudson/plugins/git/GitStatus.java b/src/main/java/hudson/plugins/git/GitStatus.java index 66f33a9015..3615c0c5cd 100644 --- a/src/main/java/hudson/plugins/git/GitStatus.java +++ b/src/main/java/hudson/plugins/git/GitStatus.java @@ -18,6 +18,7 @@ import java.util.*; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.regex.Pattern; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; @@ -28,6 +29,7 @@ import jenkins.triggers.SCMTriggerItem; import org.apache.commons.lang.StringUtils; import static org.apache.commons.lang.StringUtils.isNotEmpty; + import org.eclipse.jgit.transport.RemoteConfig; import org.eclipse.jgit.transport.URIish; import org.kohsuke.stapler.*; @@ -115,7 +117,10 @@ public HttpResponse doNotifyCommit(HttpServletRequest request, @QueryParameter(r @QueryParameter(required=false) String sha1) throws ServletException, IOException { lastURL = url; lastBranches = branches; - lastSHA1 = sha1; + if(StringUtils.isNotBlank(sha1)&&!SHA1_PATTERN.matcher(sha1.trim()).matches()){ + return HttpResponses.error(SC_BAD_REQUEST, new IllegalArgumentException("Illegal SHA1")); + } + lastSHA1 = cleanupSha1(sha1); lastBuildParameters = null; GitStatus.clearLastStaticBuildParameters(); URIish uri; @@ -316,6 +321,7 @@ public static class JenkinsAbstractProjectListener extends Listener { */ @Override public List onNotifyCommit(String origin, URIish uri, String sha1, List buildParameters, String... branches) { + sha1 = cleanupSha1(sha1); if (LOGGER.isLoggable(Level.FINE)) { LOGGER.log(Level.FINE, "Received notification from {0} for uri = {1} ; sha1 = {2} ; branches = {3}", new Object[]{StringUtils.defaultIfBlank(origin, "?"), uri, sha1, Arrays.toString(branches)}); @@ -594,15 +600,27 @@ public static class CommitHookCause extends Cause { public final String sha1; public CommitHookCause(String sha1) { - this.sha1 = sha1; + this.sha1 = cleanupSha1(sha1); } @Override public String getShortDescription() { - return "commit notification " + sha1; + return "commit notification " + cleanupSha1(sha1); } } + public static final Pattern SHA1_PATTERN = Pattern.compile("[a-fA-F0-9]++"); // we should have {40} but some compact sha1 + + public static final Pattern CLEANER_SHA1_PATTERN = Pattern.compile("[^a-fA-F0-9]"); + + /** + * @param sha1 the String to cleanup + * @return the String with all non hexa characters removed + */ + private static String cleanupSha1(String sha1){ + return sha1 == null?null:CLEANER_SHA1_PATTERN.matcher(sha1.trim()).replaceAll(""); + } + private static final Logger LOGGER = Logger.getLogger(GitStatus.class.getName()); private static final int MAX_REPORTED_CONTRIBUTORS = 10; diff --git a/src/test/java/hudson/plugins/git/GitStatusTest.java b/src/test/java/hudson/plugins/git/GitStatusTest.java index 407b305f3a..153046c4b2 100644 --- a/src/test/java/hudson/plugins/git/GitStatusTest.java +++ b/src/test/java/hudson/plugins/git/GitStatusTest.java @@ -23,6 +23,7 @@ import java.util.logging.Level; import java.util.logging.Logger; import org.eclipse.jgit.transport.URIish; +import org.kohsuke.stapler.HttpResponses; import org.mockito.Mockito; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -665,4 +666,19 @@ public void testDoNotifyCommitTriggeredHeadersLimited() throws Exception { assertEquals("URL: a Branches: master", this.gitStatus.toString()); } + + @Test + @Issue("SECURITY-2499") + public void testDoNotifyCommitWithWrongSha1Content() throws Exception { + setupProjectWithTrigger("a", "master", false); + + String content = ""; + + HttpResponse rsp = this.gitStatus.doNotifyCommit(requestWithNoParameter, "a", "master", content); + + HttpResponses.HttpResponseException responseException = ((HttpResponses.HttpResponseException) rsp); + assertEquals(IllegalArgumentException.class, responseException.getCause().getClass()); + assertEquals("Illegal SHA1", responseException.getCause().getMessage()); + + } }