From 87530e9ebc872761c06506f3cb6a4fa5c494a614 Mon Sep 17 00:00:00 2001

From: Stephen Gallagher <sgallagh@redhat.com>

Date: Tue, 7 Jan 2020 14:32:01 -0500

Subject: [PATCH 10/10] Better validation of command line arguments

Check that key passphrases are within 4-1023 characters

OpenSSL CLI tools cannot handle files with passphrases outside this

range.

Resolves: rhbz#1784441

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Output private keys with 2048 iteration count

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Rework passphrase handling

Handle passphrases as part of the sscg_stream for a file. This will

allow us to check for relevance as well as reducing code duplication.

Resolves: rhbz#1784443

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Fix wrong x509 version in CSR

This was, fortunately, not causing any problems because the signing

process resulted in the certificates being generated with the

correct version. It's best to be correct anyway.

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Fix memory leaks

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Fix alignment issue with popt

The boolean values need to be explicitly defined as int because

a bool may not be aligned properly. It was working prior to some

recent changes by lucky accident.

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Prevent uninitialized read error

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Add missing newline for error message

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Fix OpenSSL 1.0 support

The symbol UI_F_UI_SET_RESULT changed to UI_F_UI_SET_RESULT_EX in

OpenSSL 1.1, but no other semantics changed that we care about.

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Fix formatting

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Fix missing error check

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

Read long password files properly

Long passphrase files may require more than a single call to BIO_read()

to gather the whole string.

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

---

 include/io_utils.h |  37 ++++++-

 include/key.h      |   6 +-

 include/sscg.h     |  42 +++++---

 include/x509.h     |   6 +-

 meson.build        |   1 +

 src/io_utils.c     | 199 ++++++++++++++++++++++++++++++++++++-

 src/sscg.c         | 239 +++++++--------------------------------------

 src/x509.c         |   6 +-

 8 files changed, 310 insertions(+), 226 deletions(-)

diff --git a/include/io_utils.h b/include/io_utils.h

index 6a89a476b3d982447b6603153c6765835cd67464..907097c7ff1f7ae3c3adf35d0dfba0f5763dc8c0 100644

--- a/include/io_utils.h

+++ b/include/io_utils.h

@@ -24,6 +24,7 @@

 #include <stdbool.h>

 #include <talloc.h>

+#include "include/key.h"

 #include "include/sscg.h"

@@ -33,6 +34,9 @@ struct sscg_stream

   char *path;

   int mode;

   int filetypes;

+

+  bool pass_prompt;

+  char *passphrase;

 };

@@ -69,8 +73,6 @@ sscg_io_utils_get_path_by_type (struct sscg_stream **streams,

  * @path: The path to the file on disk.

  * @mode: The filesystem mode this file should have when written to disk.

  * See chmod(1) for the possible values.

- * @overwrite: If true, replace any existing file at @normalized_path. If

- * false, opening will fail if it already exists and return an error.

  *

  * Prepares all output filenames to be opened. Files are not created until

  * sscg_io_utils_open_output_files() is called.

@@ -82,9 +84,40 @@ sscg_io_utils_add_output_file (struct sscg_stream **streams,

                                int mode);

+/**

+ * sscg_io_utils_add_output_key:

+ * @streams: The array of streams from the sscg_options

+ * @filetype:

+ * @path: The path to the file on disk.

+ * @mode: The filesystem mode this file should have when written to disk.

+ * See chmod(1) for the possible values.

+ * @pass_prompt: Whether the user should be prompted to enter a passphrase

+ * interactively.

+ * @passphrase: The passphrase supplied at the command line.

+ * @passfile: The path to a file containing the passphrase.

+ *

+ * Prepares all output filenames to be opened. Files are not created until

+ * sscg_io_utils_open_output_files() is called.

+ */

+int

+sscg_io_utils_add_output_key (struct sscg_stream **streams,

+                              enum sscg_file_type filetype,

+                              const char *path,

+                              int mode,

+                              bool pass_prompt,

+                              char *passphrase,

+                              char *passfile);

+

+

 int

 sscg_io_utils_open_output_files (struct sscg_stream **streams, bool overwrite);

+int

+sscg_io_utils_write_privatekey (struct sscg_stream **streams,

+                                enum sscg_file_type filetype,

+                                struct sscg_evp_pkey *key,

+                                struct sscg_options *options);

+

 /* If this function fails, some of the output files may be left as 0400 */

 int

 sscg_io_utils_finalize_output_files (struct sscg_stream **streams);

diff --git a/include/key.h b/include/key.h

index ef871d6937e2fc805a445d6686263b023a38eaaa..4c32cad04950ee7fd75ec4144147eb919280c00a 100644

--- a/include/key.h

+++ b/include/key.h

@@ -17,15 +17,15 @@

     Copyright 2017 by Stephen Gallagher <sgallagh@redhat.com>

 */

+#ifndef _SSCG_KEY_H

+#define _SSCG_KEY_H

+

 #include <openssl/rsa.h>

 #include <openssl/evp.h>

 #include "include/sscg.h"

 #include "include/bignum.h"

-#ifndef _SSCG_KEY_H

-#define _SSCG_KEY_H

-

 struct sscg_evp_pkey

 {

   EVP_PKEY *evp_pkey;

diff --git a/include/sscg.h b/include/sscg.h

index 2744404c25c68ed905ca621bb955e0c04b33ca81..96b78152ccc492deafbbc61eb98702562a8fe5e6 100644

--- a/include/sscg.h

+++ b/include/sscg.h

@@ -20,17 +20,18 @@

 /* This is a master header file that should be included by all

    sscg source files. */

+

+#ifndef _SSCG_H

+#define _SSCG_H

+

 #include <errno.h>

 #include <openssl/ssl.h>

 #include <openssl/err.h>

+#include <openssl/ui.h>

 #include <stdbool.h>

 #include <talloc.h>

 #include <stdint.h>

-#include "include/io_utils.h"

-

-#ifndef _SSCG_H

-#define _SSCG_H

 /* TODO: implement internationalization */

@@ -81,15 +82,34 @@

     }                                                                         \

   while (0)

+/* The function changed in 1.1, but the library and reason names did not */

+#ifndef UI_F_UI_SET_RESULT_EX

+#define UI_F_UI_SET_RESULT_EX UI_F_UI_SET_RESULT

+#endif

+

 #define CHECK_SSL(_sslret, _fn)                                               \

   do                                                                          \

     {                                                                         \

       if (_sslret != 1)                                                       \

         {                                                                     \

           /* Get information about error from OpenSSL */                      \

+          unsigned long _ssl_error = ERR_get_error ();                        \

+          if ((ERR_GET_LIB (_ssl_error) == ERR_LIB_UI) &&                     \

+              (ERR_GET_FUNC (_ssl_error) == UI_F_UI_SET_RESULT_EX) &&         \

+              ((ERR_GET_REASON (_ssl_error) == UI_R_RESULT_TOO_LARGE) ||      \

+               (ERR_GET_REASON (_ssl_error) == UI_R_RESULT_TOO_SMALL)))       \

+            {                                                                 \

+              fprintf (                                                       \

+                stderr,                                                       \

+                "Passphrases must be between %d and %d characters. \n",       \

+                SSCG_MIN_KEY_PASS_LEN,                                        \

+                SSCG_MAX_KEY_PASS_LEN);                                       \

+              ret = EINVAL;                                                   \

+              goto done;                                                      \

+            }                                                                 \

           fprintf (stderr,                                                    \

                    "Error occurred in " #_fn ": [%s].\n",                     \

-                   ERR_error_string (ERR_get_error (), NULL));                \

+                   ERR_error_string (_ssl_error, NULL));                      \

           ret = EIO;                                                          \

           goto done;                                                          \

         }                                                                     \

@@ -223,12 +243,9 @@ struct sscg_options

   const EVP_CIPHER *cipher;

   const EVP_MD *hash_fn;

-  bool ca_key_pass_prompt;

-  char *ca_key_pass;

-  bool cert_key_pass_prompt;

-  char *cert_key_pass;

-  bool client_key_pass_prompt;

-  char *client_key_pass;

+  int ca_key_pass_prompt;

+  int cert_key_pass_prompt;

+  int client_key_pass_prompt;

   /* Output Files */

   struct sscg_stream **streams;

@@ -251,4 +268,7 @@ enum sscg_cert_type

   SSCG_NUM_CERT_TYPES

 };

+#define SSCG_MIN_KEY_PASS_LEN 4

+#define SSCG_MAX_KEY_PASS_LEN 1023

+

 #endif /* _SSCG_H */

diff --git a/include/x509.h b/include/x509.h

index 865cd0018d3ea77915cd86349e333ae6f4de2af0..cc7e498d06c4d2e503d7d8748dfd5386f9ad0794 100644

--- a/include/x509.h

+++ b/include/x509.h

@@ -17,6 +17,9 @@

     Copyright 2017 by Stephen Gallagher <sgallagh@redhat.com>

 */

+#ifndef _SSCG_X509_H

+#define _SSCG_X509_H

+

 #include <openssl/x509.h>

 #include <openssl/x509v3.h>

@@ -24,9 +27,6 @@

 #include "include/bignum.h"

 #include "include/key.h"

-#ifndef _SSCG_X509_H

-#define _SSCG_X509_H

-

 struct sscg_cert_info

 {

   /* === Input Data === */

diff --git a/meson.build b/meson.build

index eb339ea8c768adab6d576736fbe476b83529e78d..3d8937ce73dc84f652f6fdad461a1468a532f0f2 100644

--- a/meson.build

+++ b/meson.build

@@ -76,6 +76,7 @@ sscg_lib_hdrs = [

     'include/dhparams.h',

     'include/io_utils.h',

     'include/key.h',

+    'include/sscg.h',

     'include/x509.h',

 ]

diff --git a/src/io_utils.c b/src/io_utils.c

index a2502afb20f4bcb536428f3528900c2bb06997f5..1b8bc41c3849acbe4657ae14dfe55e3010957129 100644

--- a/src/io_utils.c

+++ b/src/io_utils.c

@@ -24,8 +24,14 @@

 #include <talloc.h>

 #include "include/io_utils.h"

+#include "include/key.h"

 #include "include/sscg.h"

+

+/* Same as OpenSSL CLI */

+#define MAX_PW_LEN 1024

+

+

 int

 sscg_normalize_path (TALLOC_CTX *mem_ctx,

                      const char *path,

@@ -62,6 +68,12 @@ sscg_stream_destructor (TALLOC_CTX *ptr)

   BIO_free (stream->bio);

+  /* Zero out the memory before freeing it so we don't leak passwords */

+  if (stream->passphrase)

+    {

+      memset (stream->passphrase, 0, strnlen (stream->passphrase, MAX_PW_LEN));

+    }

+

   return 0;

 }

@@ -147,11 +159,101 @@ sscg_io_utils_get_path_by_type (struct sscg_stream **streams,

 }

+/* This function takes a copy of a string into a talloc hierarchy and memsets

+ * the original string to zeroes to avoid leaking it when that memory is freed.

+ */

+static char *

+sscg_secure_string_steal (TALLOC_CTX *mem_ctx, char *src)

+{

+  char *dest = talloc_strdup (mem_ctx, src);

+

+  memset ((void *)src, 0, strlen (src));

+

+  return dest;

+}

+

+

+static int

+validate_passphrase (struct sscg_stream *stream)

+{

+  /* Ignore non-key types */

+  if (!(stream->filetypes & SSCG_FILE_TYPE_KEYS))

+    return EOK;

+

+  /* Ignore unset passwords; these will be prompted for when writing out the

+   * key file

+   */

+  if (!stream->passphrase)

+    return EOK;

+

+  size_t pass_len = strnlen (stream->passphrase, SSCG_MAX_KEY_PASS_LEN + 1);

+

+  if ((pass_len < SSCG_MIN_KEY_PASS_LEN) || (pass_len > SSCG_MAX_KEY_PASS_LEN))

+    {

+      SSCG_ERROR ("Passphrases must be between %d and %d characters. \n",

+                  SSCG_MIN_KEY_PASS_LEN,

+                  SSCG_MAX_KEY_PASS_LEN);

+      return EINVAL;

+    }

+  return EOK;

+}

+

+

+static char *

+sscg_read_pw_file (TALLOC_CTX *mem_ctx, char *path)

+{

+  int i;

+  BIO *pwdbio = NULL;

+  char tpass[MAX_PW_LEN + 1];

+  int offset = 0;

+  char *tmp = NULL;

+  char *password = NULL;

+

+  pwdbio = BIO_new_file (path, "r");

+  if (pwdbio == NULL)

+    {

+      fprintf (stderr, "Can't open file %s\n", path);

+      return NULL;

+    }

+

+  /* Read up to one more character than the MAX_PW_LEN */

+  for (offset = 0;

+       (i = BIO_read (pwdbio, tpass + offset, MAX_PW_LEN + 1 - offset)) > 0 &&

+       offset < (MAX_PW_LEN + 1);

+       offset += i)

+    ;

+

+  tpass[MAX_PW_LEN] = '\0';

+

+  BIO_free_all (pwdbio);

+  pwdbio = NULL;

+

+  if (i < 0)

+    {

+      fprintf (stderr, "Error reading password from BIO\n");

+      return NULL;

+    }

+

+  tmp = strchr (tpass, '\n');

+  if (tmp != NULL)

+    *tmp = 0;

+

+  password = talloc_strdup (mem_ctx, tpass);

+

+  memset (tpass, 0, MAX_PW_LEN + 1);

+

+  return password;

+}

+

+

 int

-sscg_io_utils_add_output_file (struct sscg_stream **streams,

-                               enum sscg_file_type filetype,

-                               const char *path,

-                               int mode)

+sscg_io_utils_add_output_key (struct sscg_stream **streams,

+                              enum sscg_file_type filetype,

+                              const char *path,

+                              int mode,

+                              bool pass_prompt,

+                              char *passphrase,

+                              char *passfile)

 {

   int ret, i;

   TALLOC_CTX *tmp_ctx = NULL;

@@ -163,6 +265,22 @@ sscg_io_utils_add_output_file (struct sscg_stream **streams,

    */

   if (path == NULL)

     {

+      if (pass_prompt)

+        {

+          SSCG_ERROR (

+            "Passphrase prompt requested for %s, but no file path provided.\n",

+            sscg_get_file_type_name (filetype));

+          return EINVAL;

+        }

+

+      if (passphrase)

+        {

+          SSCG_ERROR (

+            "Passphrase provided for %s, but no file path provided.\n",

+            sscg_get_file_type_name (filetype));

+          return EINVAL;

+        }

+

       SSCG_LOG (SSCG_DEBUG,

                 "Got a NULL path with filetype: %s\n",

                 sscg_get_file_type_name (filetype));

@@ -220,6 +338,31 @@ sscg_io_utils_add_output_file (struct sscg_stream **streams,

   /* Add the file type */

   stream->filetypes |= (1 << filetype);

+

+  /* Set the password options */

+  stream->pass_prompt = pass_prompt;

+

+  if (passphrase)

+    {

+      stream->passphrase = sscg_secure_string_steal (stream, passphrase);

+      ret = validate_passphrase (stream);

+      if (ret != EOK)

+        goto done;

+    }

+  else if (passfile)

+    {

+      stream->passphrase = sscg_read_pw_file (stream, passfile);

+      if (!stream->passphrase)

+        {

+          fprintf (stderr, "Failed to read passphrase from %s", passfile);

+          ret = EIO;

+          goto done;

+        }

+    }

+  ret = validate_passphrase (stream);

+  if (ret != EOK)

+    goto done;

+

   ret = EOK;

 done:

@@ -228,6 +371,17 @@ done:

 }

+int

+sscg_io_utils_add_output_file (struct sscg_stream **streams,

+                               enum sscg_file_type filetype,

+                               const char *path,

+                               int mode)

+{

+  return sscg_io_utils_add_output_key (

+    streams, filetype, path, mode, false, NULL, NULL);

+}

+

+

 enum io_utils_errors

 {

   IO_UTILS_OK = 0,

@@ -400,6 +554,43 @@ done:

 }

+int

+sscg_io_utils_write_privatekey (struct sscg_stream **streams,

+                                enum sscg_file_type filetype,

+                                struct sscg_evp_pkey *key,

+                                struct sscg_options *options)

+{

+  int ret, sret;

+

+  struct sscg_stream *stream =

+    sscg_io_utils_get_stream_by_type (streams, filetype);

+  if (stream)

+    {

+      /* This function has a default mechanism for prompting for the

+       * password if it is passed a cipher and gets a NULL password.

+       *

+       * Only pass the cipher if we have a password or were instructed

+       * to prompt for one.

+       */

+      sret = PEM_write_bio_PKCS8PrivateKey (

+        stream->bio,

+        key->evp_pkey,

+        stream->pass_prompt || stream->passphrase ? options->cipher : NULL,

+        stream->passphrase,

+        stream->passphrase ? strlen (stream->passphrase) : 0,

+        NULL,

+        NULL);

+      CHECK_SSL (sret, PEM_write_bio_PKCS8PrivateKey);

+      ANNOUNCE_WRITE (filetype);

+    }

+

+  ret = EOK;

+

+done:

+  return ret;

+}

+

+

 int

 sscg_io_utils_finalize_output_files (struct sscg_stream **streams)

 {

diff --git a/src/sscg.c b/src/sscg.c

index 4d009a67488e83c4332f58ee52f7d6ea72a8ddbd..96a9be1232d890590e97c126f8f4a78d571d7247 100644

--- a/src/sscg.c

+++ b/src/sscg.c

@@ -18,6 +18,7 @@

 */

 #define _GNU_SOURCE

+#include <assert.h>

 #include <popt.h>

 #include <stdlib.h>

 #include <stdio.h>

@@ -40,9 +41,6 @@

 int verbosity;

-/* Same as OpenSSL CLI */

-#define MAX_PW_LEN 1024

-

 static int

 get_security_level (void)

 {

@@ -140,79 +138,6 @@ print_options (struct sscg_options *opts)

 }

-/* This function takes a copy of a string into a talloc hierarchy and memsets

- * the original string to zeroes to avoid leaking it when that memory is freed.

- */

-static char *

-sscg_secure_string_steal (TALLOC_CTX *mem_ctx, char *src)

-{

-  char *dest = talloc_strdup (mem_ctx, src);

-

-  memset (src, 0, strlen (src));

-

-  return dest;

-}

-

-

-static int

-sscg_options_destructor (TALLOC_CTX *opts)

-{

-  struct sscg_options *options =

-    talloc_get_type_abort (opts, struct sscg_options);

-

-  /* Zero out the memory before freeing it so we don't leak passwords */

-  if (options->ca_key_pass)

-    {

-      memset (options->ca_key_pass, 0, strlen (options->ca_key_pass));

-    }

-

-  if (options->cert_key_pass)

-    {

-      memset (options->cert_key_pass, 0, strlen (options->cert_key_pass));

-    }

-

-  return 0;

-}

-

-

-static char *

-sscg_read_pw_file (TALLOC_CTX *mem_ctx, char *path)

-{

-  int i;

-  BIO *pwdbio = NULL;

-  char tpass[MAX_PW_LEN];

-  char *tmp = NULL;

-  char *password = NULL;

-

-  pwdbio = BIO_new_file (path, "r");

-  if (pwdbio == NULL)

-    {

-      fprintf (stderr, "Can't open file %s\n", path);

-      return NULL;

-    }

-

-  i = BIO_gets (pwdbio, tpass, MAX_PW_LEN);

-  BIO_free_all (pwdbio);

-  pwdbio = NULL;

-

-  if (i <= 0)

-    {

-      fprintf (stderr, "Error reading password from BIO\n");

-      return NULL;

-    }

-

-  tmp = strchr (tpass, '\n');

-  if (tmp != NULL)

-    *tmp = 0;

-

-  password = talloc_strdup (mem_ctx, tpass);

-

-  memset (tpass, 0, MAX_PW_LEN);

-

-  return password;

-}

-

-

 const char *

 sscg_get_verbosity_name (enum sscg_verbosity type)

 {

@@ -310,12 +235,14 @@ main (int argc, const char **argv)

   struct sscg_evp_pkey *cakey;

   struct sscg_x509_cert *svc_cert;

   struct sscg_evp_pkey *svc_key;

-  struct sscg_x509_cert *client_cert;

-  struct sscg_evp_pkey *client_key;

+  struct sscg_x509_cert *client_cert = NULL;

+  struct sscg_evp_pkey *client_key = NULL;

   int dhparams_mode = SSCG_CERT_DEFAULT_MODE;

   struct sscg_dhparams *dhparams = NULL;

+  struct sscg_stream *stream = NULL;

+

   /* Always use umask 0577 for generating certificates and keys

        This means that it's opened as write-only by the effective

        user. */

@@ -335,7 +262,6 @@ main (int argc, const char **argv)

   options = talloc_zero (main_ctx, struct sscg_options);

   CHECK_MEM (options);

-  talloc_set_destructor ((TALLOC_CTX *)options, sscg_options_destructor);

   options->streams =

     talloc_zero_array (options, struct sscg_stream *, SSCG_NUM_FILE_TYPES);

@@ -965,56 +891,6 @@ main (int argc, const char **argv)

         }

     }

-  /* Password handling */

-  if (ca_key_password)

-    {

-      options->ca_key_pass =

-        sscg_secure_string_steal (options, ca_key_password);

-    }

-  else if (ca_key_passfile)

-    {

-      options->ca_key_pass = sscg_read_pw_file (options, ca_key_passfile);

-      if (!options->ca_key_pass)

-        {

-          fprintf (

-            stderr, "Failed to read passphrase from %s", ca_key_passfile);

-          goto done;

-        }

-    }

-

-  if (cert_key_password)

-    {

-      options->cert_key_pass =

-        sscg_secure_string_steal (options, cert_key_password);

-    }

-  else if (cert_key_passfile)

-    {

-      options->cert_key_pass = sscg_read_pw_file (options, cert_key_passfile);

-      if (!options->cert_key_pass)

-        {

-          fprintf (

-            stderr, "Failed to read passphrase from %s", cert_key_passfile);