Skip to content
Permalink
Browse files

Avoid catastrophic regex backtracking (#3551)

While validating for leading and trailing whitespaces in `exec` command,
we were using a regex with nested construct that has `+` and `*`.
  • Loading branch information...
ketan committed May 26, 2017
1 parent 483fdb7 commit 26bfeab2c8c1821802e89a9baa0f584780f7c273
@@ -1,6 +1,6 @@
<?xml version="1.0" encoding="utf-8"?>
<!--
~ Copyright 2016 ThoughtWorks, Inc.
~ Copyright 2017 ThoughtWorks, Inc.
~
~ Licensed under the Apache License, Version 2.0 (the "License");
~ you may not use this file except in compliance with the License.
@@ -841,7 +841,7 @@
</xsd:attributeGroup>
<xsd:simpleType name="commandType">
<xsd:restriction base="xsd:string">
<xsd:pattern value="\S+(.*\S+)*"/>
<xsd:pattern value="\S(.*\S)?"/>
</xsd:restriction>
</xsd:simpleType>
<xsd:complexType name="dependsType">
@@ -21,7 +21,6 @@
import com.thoughtworks.go.config.elastic.ElasticProfile;
import com.thoughtworks.go.config.exceptions.GoConfigInvalidException;
import com.thoughtworks.go.config.materials.*;
import com.thoughtworks.go.config.materials.Filter;
import com.thoughtworks.go.config.materials.git.GitMaterialConfig;
import com.thoughtworks.go.config.materials.mercurial.HgMaterialConfig;
import com.thoughtworks.go.config.materials.perforce.P4MaterialConfig;
@@ -35,7 +34,6 @@
import com.thoughtworks.go.config.remote.ConfigRepoConfig;
import com.thoughtworks.go.config.remote.FileConfigOrigin;
import com.thoughtworks.go.config.remote.PartialConfig;
import com.thoughtworks.go.config.server.security.ldap.BaseConfig;
import com.thoughtworks.go.config.validation.*;
import com.thoughtworks.go.domain.*;
import com.thoughtworks.go.domain.config.Admin;
@@ -64,7 +62,10 @@
import com.thoughtworks.go.plugin.api.task.TaskExecutor;
import com.thoughtworks.go.plugin.api.task.TaskView;
import com.thoughtworks.go.security.GoCipher;
import com.thoughtworks.go.util.*;
import com.thoughtworks.go.util.ConfigElementImplementationRegistryMother;
import com.thoughtworks.go.util.FileUtil;
import com.thoughtworks.go.util.ReflectionUtil;
import com.thoughtworks.go.util.XsdValidationException;
import com.thoughtworks.go.util.command.HgUrlArgument;
import com.thoughtworks.go.util.command.UrlArgument;
import org.apache.commons.collections.CollectionUtils;
@@ -898,7 +899,7 @@ public void shouldThrowExceptionWhenCommandIsEmpty() throws Exception {

fail("Should not allow empty command");
} catch (Exception e) {
assertThat(e.getMessage(), containsString("Command is invalid. \"\" should conform to the pattern - \\S+(.*\\S+)*"));
assertThat(e.getMessage(), containsString("Command is invalid. \"\" should conform to the pattern - \\S(.*\\S)?"));
}
}

@@ -928,7 +929,7 @@ public void shouldThrowExceptionWhenCommandsContainTrailingSpaces() throws Excep

fail("Should not allow command with trailing spaces");
} catch (Exception e) {
assertThat(e.getMessage(), containsString("Command is invalid. \"bundle \" should conform to the pattern - \\S+(.*\\S+)*"));
assertThat(e.getMessage(), containsString("Command is invalid. \"bundle \" should conform to the pattern - \\S(.*\\S)?"));
}
}

@@ -958,7 +959,7 @@ public void shouldThrowExceptionWhenCommandsContainLeadingSpaces() throws Except

fail("Should not allow command with trailing spaces");
} catch (Exception e) {
assertThat(e.getMessage(), containsString("Command is invalid. \" bundle\" should conform to the pattern - \\S+(.*\\S+)*"));
assertThat(e.getMessage(), containsString("Command is invalid. \" bundle\" should conform to the pattern - \\S(.*\\S)?"));
}
}

@@ -1,5 +1,5 @@
/*
* Copyright 2016 ThoughtWorks, Inc.
* Copyright 2017 ThoughtWorks, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -45,6 +45,7 @@
import com.thoughtworks.go.util.command.UrlArgument;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.StringUtils;
import org.hamcrest.core.Is;
import org.jdom2.input.JDOMParseException;
import org.junit.After;
@@ -1141,7 +1142,22 @@ public void shouldNotWriteEmptyAuthorizationUnderEachTemplateTagOntoConfigFile()
assertThat(writtenConfigXml, not(containsString("<authorization>")));
}

@Test(timeout = 1000)
public void shouldValidateLeadingAndTrailingSpacesOnExecCommandInReasonableTime() throws Exception {
// See https://github.com/gocd/gocd/issues/3551
// This is only reproducible on longish strings, so don't try shortening the exec task length...
String longPath = StringUtils.repeat("f", 100);
CruiseConfig config = GoConfigMother.configWithPipelines("pipeline1");
config.findJob("pipeline1", "stage", "job").addTask(new ExecTask(longPath + " ", "arg1", (String) null));

output = new ByteArrayOutputStream();
try {
xmlWriter.write(config, output, false);
fail("expected to blow up");
}catch (XsdValidationException e){
assertThat(e.getMessage(), containsString("should conform to the pattern - \\S(.*\\S)?"));
}
}

@Test
public void shouldDisplayTheFlagInXmlIfTemplateAuthorizationDoesNotAllowGroupAdmins() throws Exception {

0 comments on commit 26bfeab

Please sign in to comment.
You can’t perform that action at this time.