From bc038b323d7c76b048e6f1e4bdbe99244154182f Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Wed, 11 Oct 2017 12:30:19 +1000 Subject: [PATCH] [Security] Apply validation when parsing certgen input (elastic/x-pack-elasticsearch#2711) When certgen configuration was read from an input file (`-in` option) validation errors were collected but never reported. Depending on the type of error this may have caused the tool to exit with an internal error (e.g. NPE). Validation is now applied after parsing the file and if errors are found the tool exits. Original commit: elastic/x-pack-elasticsearch@b2262ed1d7252d446d719c3eab33d8193954760e --- .../elasticsearch/xpack/ssl/CertUtils.java | 4 +++ .../xpack/ssl/CertificateTool.java | 25 ++++++++++++++++++- .../xpack/ssl/CertificateToolTests.java | 25 +++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ssl/CertUtils.java b/plugin/src/main/java/org/elasticsearch/xpack/ssl/CertUtils.java index 2e66cec0767..af6c9fa19ed 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ssl/CertUtils.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ssl/CertUtils.java @@ -38,6 +38,7 @@ import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Locale; +import java.util.Objects; import java.util.Set; import java.util.function.Supplier; @@ -360,6 +361,9 @@ public class CertUtils { */ static PKCS10CertificationRequest generateCSR(KeyPair keyPair, X500Principal principal, GeneralNames sanList) throws IOException, OperatorCreationException { + Objects.requireNonNull(keyPair, "Key-Pair must not be null"); + Objects.requireNonNull(keyPair.getPublic(), "Public-Key must not be null"); + Objects.requireNonNull(principal, "Principal must not be null"); JcaPKCS10CertificationRequestBuilder builder = new JcaPKCS10CertificationRequestBuilder(principal, keyPair.getPublic()); if (sanList != null) { ExtensionsGenerator extGen = new ExtensionsGenerator(); diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ssl/CertificateTool.java b/plugin/src/main/java/org/elasticsearch/xpack/ssl/CertificateTool.java index 4723840da59..9b6917098c1 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ssl/CertificateTool.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ssl/CertificateTool.java @@ -47,7 +47,10 @@ import org.bouncycastle.openssl.jcajce.JcePEMEncryptorBuilder; import org.bouncycastle.pkcs.PKCS10CertificationRequest; import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.cli.EnvironmentAwareCommand; +import org.elasticsearch.cli.ExitCodes; import org.elasticsearch.cli.Terminal; +import org.elasticsearch.cli.Terminal.Verbosity; +import org.elasticsearch.cli.UserException; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.Strings; import org.elasticsearch.common.SuppressForbidden; @@ -222,7 +225,7 @@ public class CertificateTool extends EnvironmentAwareCommand { static Collection getCertificateInformationList(Terminal terminal, String inputFile) throws Exception { if (inputFile != null) { - return parseFile(resolvePath(inputFile).toAbsolutePath()); + return parseAndValidateFile(terminal, resolvePath(inputFile).toAbsolutePath()); } Map map = new HashMap<>(); boolean done = false; @@ -267,6 +270,26 @@ public class CertificateTool extends EnvironmentAwareCommand { return map.values(); } + static Collection parseAndValidateFile(Terminal terminal, Path file) throws Exception { + final Collection config = parseFile(file); + boolean hasError = false; + for (CertificateInformation certInfo : config) { + final List errors = certInfo.validate(); + if (errors.size() > 0) { + hasError = true; + terminal.println(Verbosity.SILENT, "Configuration for instance " + certInfo.name.originalName + " has invalid details"); + for (String message : errors) { + terminal.println(Verbosity.SILENT, " * " + message); + } + terminal.println(""); + } + } + if (hasError) { + throw new UserException(ExitCodes.CONFIG, "File " + file + " contains invalid configuration details (see messages above)"); + } + return config; + } + /** * Parses the input file to retrieve the certificate information * @param file the file to parse diff --git a/plugin/src/test/java/org/elasticsearch/xpack/ssl/CertificateToolTests.java b/plugin/src/test/java/org/elasticsearch/xpack/ssl/CertificateToolTests.java index 97f5a4cb49c..93305e7b1e7 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/ssl/CertificateToolTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/ssl/CertificateToolTests.java @@ -56,6 +56,8 @@ import org.bouncycastle.openssl.PEMEncryptedKeyPair; import org.bouncycastle.openssl.PEMParser; import org.bouncycastle.pkcs.PKCS10CertificationRequest; import org.elasticsearch.cli.MockTerminal; +import org.elasticsearch.cli.Terminal; +import org.elasticsearch.cli.UserException; import org.elasticsearch.common.Strings; import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.common.io.PathUtils; @@ -211,6 +213,18 @@ public class CertificateToolTests extends ESTestCase { assertEquals("different file", certInfo.name.filename); } + public void testParsingFileWithInvalidDetails() throws Exception { + Path tempDir = initTempDir(); + Path instanceFile = writeInvalidInstanceInformation(tempDir.resolve("instances-invalid.yml")); + final MockTerminal terminal = new MockTerminal(); + final UserException exception = expectThrows(UserException.class, + () -> CertificateTool.parseAndValidateFile(terminal, instanceFile)); + assertThat(exception.getMessage(), containsString("invalid configuration")); + assertThat(exception.getMessage(), containsString(instanceFile.toString())); + assertThat(terminal.getOutput(), containsString("THIS=not a,valid DN")); + assertThat(terminal.getOutput(), containsString("could not be converted to a valid DN")); + } + public void testGeneratingCsr() throws Exception { Path tempDir = initTempDir(); Path outputFile = tempDir.resolve("out.zip"); @@ -528,6 +542,17 @@ public class CertificateToolTests extends ESTestCase { return Files.write(path, instances, StandardCharsets.UTF_8); } + /** + * Writes the description of instances to a given {@link Path} + */ + private Path writeInvalidInstanceInformation(Path path) throws IOException { + Iterable instances = Arrays.asList( + "instances:", + " - name: \"THIS=not a,valid DN\"", + " ip: \"127.0.0.1\""); + return Files.write(path, instances, StandardCharsets.UTF_8); + } + @SuppressForbidden(reason = "resolve paths against CWD for a CLI tool") private static Path resolvePath(String path) { return PathUtils.get(path).toAbsolutePath();