Avoid system(3) in ikectl

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Avoid system(3) in ikectl

Matthew Martin
I had sent a similar patch a while back. There seemed to me some
interest, but it was never comitted. Updated to apply to -current.

Apologies for the attachment; gmail still isn't sending emails sent
via mutt, but I suspect the patch in the body will be mangled.

- Matthew Martin"

diff --git ikeca.c ikeca.c
index bac76ab9c2f..01e600abb2c 100644
--- ikeca.c
+++ ikeca.c
@@ -18,6 +18,7 @@

 #include <sys/types.h>
 #include <sys/stat.h>
+#include <sys/wait.h>
 #include <stdio.h>
 #include <unistd.h>
 #include <err.h>
@@ -63,12 +64,12 @@

 struct ca {
  char sslpath[PATH_MAX];
- char passfile[PATH_MAX];
+ char passfile[PATH_MAX + 5];
  char index[PATH_MAX];
  char serial[PATH_MAX];
  char sslcnf[PATH_MAX];
  char extcnf[PATH_MAX];
- char batch[PATH_MAX];
+ char *batch;
  char *caname;
 };

@@ -116,6 +117,7 @@ void ca_setenv(const char *, const char *);
 void ca_clrenv(void);
 void ca_setcnf(struct ca *, const char *);
 void ca_create_index(struct ca *);
+int static run(char *const []);

 /* util.c */
 int expand_string(char *, size_t, const char *, const char *);
@@ -130,7 +132,6 @@ int
 ca_key_create(struct ca *ca, char *keyname)
 {
  struct stat st;
- char cmd[PATH_MAX * 2];
  char path[PATH_MAX];

  snprintf(path, sizeof(path), "%s/private/%s.key", ca->sslpath, keyname);
@@ -140,10 +141,8 @@ ca_key_create(struct ca *ca, char *keyname)
  return (0);
  }

- snprintf(cmd, sizeof(cmd),
-     "%s genrsa -out %s 2048",
-     PATH_OPENSSL, path);
- system(cmd);
+ char *cmd[] = { PATH_OPENSSL, "genrsa", "-out", path, "2048", NULL };
+ run(cmd);
  chmod(path, 0600);

  return (0);
@@ -200,9 +199,9 @@ ca_delkey(struct ca *ca, char *keyname)
 int
 ca_request(struct ca *ca, char *keyname, int type)
 {
- char cmd[PATH_MAX * 2];
  char hostname[HOST_NAME_MAX+1];
  char name[128];
+ char key[PATH_MAX];
  char path[PATH_MAX];

  ca_setenv("$ENV::CERT_CN", keyname);
@@ -226,13 +225,12 @@ ca_request(struct ca *ca, char *keyname, int type)

  ca_setcnf(ca, keyname);

+ snprintf(key, sizeof(key), "%s/private/%s.key", ca->sslpath, keyname);
  snprintf(path, sizeof(path), "%s/private/%s.csr", ca->sslpath, keyname);
- snprintf(cmd, sizeof(cmd), "%s req %s-new"
-     " -key %s/private/%s.key -out %s -config %s",
-     PATH_OPENSSL, ca->batch, ca->sslpath, keyname,
-     path, ca->sslcnf);

- system(cmd);
+ char *cmd[] = { PATH_OPENSSL, "req", "-new", "-key", key, "-out", path,
+     "-config", ca->sslcnf, ca->batch, NULL };
+ run(cmd);
  chmod(path, 0600);

  return (0);
@@ -241,8 +239,11 @@ ca_request(struct ca *ca, char *keyname, int type)
 int
 ca_sign(struct ca *ca, char *keyname, int type)
 {
- char cmd[PATH_MAX * 2];
- const char *extensions = NULL;
+ char cakey[PATH_MAX];
+ char cacrt[PATH_MAX];
+ char out[PATH_MAX];
+ char in[PATH_MAX];
+ char *extensions = NULL;

  if (type == HOST_IPADDR) {
  extensions = "x509v3_IPAddr";
@@ -258,19 +259,16 @@ ca_sign(struct ca *ca, char *keyname, int type)
  ca_setenv("$ENV::CASERIAL", ca->serial);
  ca_setcnf(ca, keyname);

- snprintf(cmd, sizeof(cmd),
-     "%s ca -config %s -keyfile %s/private/ca.key"
-     " -cert %s/ca.crt"
-     " -extfile %s -extensions %s -out %s/%s.crt"
-     " -in %s/private/%s.csr"
-     " -passin file:%s -outdir %s -batch",
-     PATH_OPENSSL, ca->sslcnf, ca->sslpath,
-     ca->sslpath,
-     ca->extcnf, extensions, ca->sslpath, keyname,
-     ca->sslpath, keyname,
-     ca->passfile, ca->sslpath);
+ snprintf(cakey, sizeof(cakey), "%s/private/ca.key", ca->sslpath);
+ snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath);
+ snprintf(out, sizeof(out), "%s/%s.crt", ca->sslpath, keyname);
+ snprintf(in, sizeof(in), "%s/private/%s.csr", ca->sslpath, keyname);

- system(cmd);
+ char *cmd[] = { PATH_OPENSSL, "ca", "-config", ca->sslcnf,
+     "-keyfile", cakey, "-cert", cacrt, "-extfile", ca->extcnf,
+     "-extensions", extensions, "-out", out, "-in", in,
+     "-passin", ca->passfile, "-outdir", ca->sslpath, "-batch", NULL };
+ run(cmd);

  return (0);
 }
@@ -313,9 +311,9 @@ int
 ca_key_install(struct ca *ca, char *keyname, char *dir)
 {
  struct stat st;
- char cmd[PATH_MAX * 2];
  char src[PATH_MAX];
  char dst[PATH_MAX];
+ char out[PATH_MAX];
  char *p = NULL;

  snprintf(src, sizeof(src), "%s/private/%s.key", ca->sslpath, keyname);
@@ -335,9 +333,11 @@ ca_key_install(struct ca *ca, char *keyname, char *dir)
  snprintf(dst, sizeof(dst), "%s/private/local.key", dir);
  fcopy(src, dst, 0600);

- snprintf(cmd, sizeof(cmd), "%s rsa -out %s/local.pub"
-     " -in %s/private/local.key -pubout", PATH_OPENSSL, dir, dir);
- system(cmd);
+ snprintf(out, sizeof(out), "%s/local.pub", dir);
+
+ char *cmd[] = { PATH_OPENSSL, "rsa", "-out", out, "-in", dst,
+     "-pubout", NULL };
+ run(cmd);

  free(p);

@@ -405,37 +405,41 @@ ca_newpass(char *passfile, char *password)
 int
 ca_create(struct ca *ca)
 {
- char cmd[PATH_MAX * 2];
- char path[PATH_MAX];
+ char key[PATH_MAX];
+ char csr[PATH_MAX];
+ char crt[PATH_MAX];

  ca_clrenv();

- snprintf(path, sizeof(path), "%s/private/ca.key", ca->sslpath);
- snprintf(cmd, sizeof(cmd), "%s genrsa -aes256 -out"
-     " %s -passout file:%s 2048", PATH_OPENSSL,
-     path, ca->passfile);
- system(cmd);
- chmod(path, 0600);
+ snprintf(key, sizeof(key), "%s/private/ca.key", ca->sslpath);
+ {
+ char *cmd[] = { PATH_OPENSSL, "genrsa", "-aes256", "-out", key,
+     "-passout", ca->passfile, "2048", NULL };
+ run(cmd);
+ }
+ chmod(key, 0600);

  ca_setenv("$ENV::CERT_CN", "VPN CA");
  ca_setenv("$ENV::REQ_EXT", "x509v3_CA");
  ca_setcnf(ca, "ca");

- snprintf(path, sizeof(path), "%s/private/ca.csr", ca->sslpath);
- snprintf(cmd, sizeof(cmd), "%s req %s-new"
-     " -key %s/private/ca.key"
-     " -config %s -out %s -passin file:%s", PATH_OPENSSL,
-     ca->batch, ca->sslpath, ca->sslcnf, path, ca->passfile);
- system(cmd);
- chmod(path, 0600);
-
- snprintf(cmd, sizeof(cmd), "%s x509 -req -days 4500"
-     " -in %s/private/ca.csr -signkey %s/private/ca.key"
-     " -sha256"
-     " -extfile %s -extensions x509v3_CA -out %s/ca.crt -passin file:%s",
-     PATH_OPENSSL, ca->sslpath, ca->sslpath, ca->extcnf, ca->sslpath,
-     ca->passfile);
- system(cmd);
+ snprintf(csr, sizeof(csr), "%s/private/ca.csr", ca->sslpath);
+ {
+ char *cmd[] = { PATH_OPENSSL, "req", "-new", "-key", key,
+     "-config", ca->sslcnf, "-out", csr,
+     "-passin", ca->passfile, ca->batch, NULL };
+ run(cmd);
+ }
+ chmod(csr, 0600);
+
+ snprintf(crt, sizeof(crt), "%s/ca.crt", ca->sslpath);
+ {
+ char *cmd[] = { PATH_OPENSSL, "x509", "-req", "-days", "4500",
+     "-in", csr, "-signkey", key, "-sha256",
+     "-extfile", ca->extcnf, "-extensions", "x509v3_CA",
+     "-out", crt, "-passin", ca->passfile, NULL };
+ run(cmd);
+ }

  /* Create the CRL revocation list */
  ca_revoke(ca, NULL);
@@ -485,7 +489,6 @@ ca_show_certs(struct ca *ca, char *name)
 {
  DIR *dir;
  struct dirent *de;
- char cmd[PATH_MAX * 2];
  char path[PATH_MAX];
  char *p;
  struct stat st;
@@ -495,9 +498,11 @@ ca_show_certs(struct ca *ca, char *name)
      ca->sslpath, name);
  if (stat(path, &st) != 0)
  err(1, "could not open file %s.crt", name);
- snprintf(cmd, sizeof(cmd), "%s x509 -text"
-     " -in %s", PATH_OPENSSL, path);
- system(cmd);
+ {
+ char *cmd[] = { PATH_OPENSSL, "x509", "-text",
+     "-in", path, NULL };
+ run(cmd);
+ }
  printf("\n");
  return (0);
  }
@@ -512,10 +517,10 @@ ca_show_certs(struct ca *ca, char *name)
  continue;
  snprintf(path, sizeof(path), "%s/%s", ca->sslpath,
      de->d_name);
- snprintf(cmd, sizeof(cmd), "%s x509 -subject"
-     " -fingerprint -dates -noout -in %s",
-     PATH_OPENSSL, path);
- system(cmd);
+ char *cmd[] = { PATH_OPENSSL, "x509", "-subject",
+     "-fingerprint", "-dates", "-noout", "-in", path,
+     NULL };
+ run(cmd);
  printf("\n");
  }
  }
@@ -649,10 +654,15 @@ ca_export(struct ca *ca, char *keyname, char
*myname, char *password)
  struct stat st;
  char *pass;
  char prev[_PASSWORD_LEN + 1];
- char cmd[PATH_MAX * 2];
+ char passenv[_PASSWORD_LEN + 8];
  char oname[PATH_MAX];
  char src[PATH_MAX];
  char dst[PATH_MAX];
+ char cacrt[PATH_MAX];
+ char capfx[PATH_MAX];
+ char key[PATH_MAX];
+ char crt[PATH_MAX];
+ char pfx[PATH_MAX];
  char *p;
  char tpl[] = "/tmp/ikectl.XXXXXXXXXX";
  unsigned int i;
@@ -682,22 +692,33 @@ ca_export(struct ca *ca, char *keyname, char
*myname, char *password)
  errx(1, "passphrase does not match!");
  }

+ snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath);
+ snprintf(capfx, sizeof(capfx), "%s/ca.pfx", ca->sslpath);
+ snprintf(key, sizeof(key), "%s/private/%s.key", ca->sslpath, keyname);
+ snprintf(crt, sizeof(crt), "%s/%s.crt", ca->sslpath, keyname);
+ snprintf(pfx, sizeof(pfx), "%s/private/%s.pfx", ca->sslpath, oname);
+
+ snprintf(passenv, sizeof(passenv), "EXPASS=%s", pass);
+ putenv(passenv);
+
  if (keyname != NULL) {
- snprintf(cmd, sizeof(cmd), "env EXPASS=%s %s pkcs12 -export"
-     " -name %s -CAfile %s/ca.crt -inkey %s/private/%s.key"
-     " -in %s/%s.crt -out %s/private/%s.pfx -passout env:EXPASS"
-     " -passin file:%s", pass, PATH_OPENSSL, keyname,
-     ca->sslpath, ca->sslpath, keyname, ca->sslpath, keyname,
-     ca->sslpath, oname, ca->passfile);
- system(cmd);
- }
-
- snprintf(cmd, sizeof(cmd), "env EXPASS=%s %s pkcs12 -export"
-     " -caname '%s' -name '%s' -cacerts -nokeys"
-     " -in %s/ca.crt -out %s/ca.pfx -passout env:EXPASS -passin file:%s",
-     pass, PATH_OPENSSL, ca->caname, ca->caname, ca->sslpath,
-     ca->sslpath, ca->passfile);
- system(cmd);
+ char *cmd[] = { PATH_OPENSSL, "pkcs12", "-export",
+     "-name", keyname, "-CAfile", cacrt, "-inkey", key,
+     "-in", crt, "-out", pfx, "-passout", "env:EXPASS",
+     "-passin", ca->passfile, NULL };
+ run(cmd);
+ }
+
+ {
+ char *cmd[] = { PATH_OPENSSL, "pkcs12", "-export",
+     "-caname", ca->caname, "-name", ca->caname, "-cacerts",
+     "-nokeys", "-in", cacrt, "-out", capfx,
+     "-passout", "env:EXPASS", "-passin", ca->passfile, NULL };
+ run(cmd);
+ }
+
+ unsetenv("EXPASS");
+ explicit_bzero(passenv, sizeof(passenv));

  if ((p = mkdtemp(tpl)) == NULL)
  err(1, "could not create temp dir");
@@ -752,21 +773,23 @@ ca_export(struct ca *ca, char *keyname, char
*myname, char *password)
  snprintf(dst, sizeof(dst), "%s/certs/%s.crt", p, keyname);
  fcopy(src, dst, 0644);

- snprintf(cmd, sizeof(cmd), "%s rsa -out %s/local.pub"
-     " -in %s/private/%s.key -pubout", PATH_OPENSSL, p,
-     ca->sslpath, keyname);
- system(cmd);
+ snprintf(dst, sizeof(dst), "%s/local.pub", p);
+ char *cmd[] = { PATH_OPENSSL, "rsa", "-out", dst, "-in", key,
+     "-pubout", NULL };
+ run(cmd);
  }

  if (stat(PATH_TAR, &st) == 0) {
- if (keyname == NULL)
- snprintf(cmd, sizeof(cmd), "%s -zcf %s.tgz -C %s .",
-     PATH_TAR, oname, ca->sslpath);
- else
- snprintf(cmd, sizeof(cmd), "%s -zcf %s.tgz -C %s .",
-     PATH_TAR, oname, p);
- system(cmd);
  snprintf(src, sizeof(src), "%s.tgz", oname);
+ if (keyname == NULL) {
+ char *cmd[] = { PATH_TAR, "-zcf", src,
+     "-C", ca->sslpath, ".", NULL };
+ run(cmd);
+ } else {
+ char *cmd[] = { PATH_TAR, "-zcf", src, "-C", p, ".",
+     NULL };
+ run(cmd);
+ }
  if (realpath(src, dst) != NULL)
  printf("exported files in %s\n", dst);
  }
@@ -795,8 +818,8 @@ ca_export(struct ca *ca, char *keyname, char
*myname, char *password)
  err(1, "could not change %s", dst);

  snprintf(dst, sizeof(dst), "%s/%s.zip", src, oname);
- snprintf(cmd, sizeof(cmd), "%s -qr %s .", PATH_ZIP, dst);
- system(cmd);
+ char *cmd[] = { PATH_ZIP, "-qr", dst, ".", NULL };
+ run(cmd);
  printf("exported files in %s\n", dst);

  if (chdir(src) == -1)
@@ -849,8 +872,9 @@ int
 ca_revoke(struct ca *ca, char *keyname)
 {
  struct stat st;
- char cmd[PATH_MAX * 2];
  char path[PATH_MAX];
+ char cakey[PATH_MAX];
+ char cacrt[PATH_MAX];

  if (keyname) {
  snprintf(path, sizeof(path), "%s/%s.crt",
@@ -870,27 +894,21 @@ ca_revoke(struct ca *ca, char *keyname)

  ca_setcnf(ca, "ca-revoke");

+ snprintf(cakey, sizeof(cakey), "%s/private/ca.key", ca->sslpath);
+ snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath);
+
  if (keyname) {
- snprintf(cmd, sizeof(cmd),
-     "%s ca %s-config %s -keyfile %s/private/ca.key"
-     " -passin file:%s"
-     " -cert %s/ca.crt"
-     " -revoke %s/%s.crt",
-     PATH_OPENSSL, ca->batch, ca->sslcnf,
-     ca->sslpath, ca->passfile, ca->sslpath, ca->sslpath, keyname);
- system(cmd);
- }
-
- snprintf(cmd, sizeof(cmd),
-     "%s ca %s-config %s -keyfile %s/private/ca.key"
-     " -passin file:%s"
-     " -gencrl"
-     " -cert %s/ca.crt"
-     " -crldays 365"
-     " -out %s/ca.crl",
-     PATH_OPENSSL, ca->batch, ca->sslcnf, ca->sslpath,
-     ca->passfile, ca->sslpath, ca->sslpath);
- system(cmd);
+ char *cmd[] = { PATH_OPENSSL, "ca", "-config", ca->sslcnf,
+     "-keyfile", cakey, "-passin", ca->passfile, "-cert", cacrt,
+     "-revoke", path, ca->batch, NULL };
+ run(cmd);
+ }
+
+ snprintf(path, sizeof(path), "%s/ca.crl", ca->sslpath);
+ char *cmd[] = { PATH_OPENSSL, "ca", "-config", ca->sslcnf,
+     "-keyfile", cakey, "-passin", ca->passfile, "-gencrl",
+     "-cert", cacrt, "-crldays", "365", "-out", path, ca->batch, NULL };
+ run(cmd);

  return (0);
 }
@@ -963,11 +981,9 @@ ca_setup(char *caname, int create, int quiet, char *pass)

  ca->caname = strdup(caname);
  snprintf(ca->sslpath, sizeof(ca->sslpath), SSLDIR "/%s", caname);
- strlcpy(ca->passfile, ca->sslpath, sizeof(ca->passfile));
- strlcat(ca->passfile, "/ikeca.passwd", sizeof(ca->passfile));

  if (quiet)
- strlcpy(ca->batch, "-batch ", sizeof(ca->batch));
+ ca->batch = "-batch";

  if (create == 0 && stat(ca->sslpath, &st) == -1) {
  free(ca->caname);
@@ -982,8 +998,31 @@ ca_setup(char *caname, int create, int quiet, char *pass)
  if (mkdir(path, 0700) == -1 && errno != EEXIST)
  err(1, "failed to create dir %s", path);

- if (create && stat(ca->passfile, &st) == -1 && errno == ENOENT)
- ca_newpass(ca->passfile, pass);
+ snprintf(path, sizeof(path), "%s/ikeca.passwd", ca->sslpath);
+ if (create && stat(path, &st) == -1 && errno == ENOENT)
+ ca_newpass(path, pass);
+ snprintf(ca->passfile, sizeof(ca->passfile), "file:%s", path);

  return (ca);
 }
+
+int static
+run(char *const argv[])
+{
+ pid_t pid, cpid;
+ int status;
+
+ switch (cpid = fork()) {
+ case -1:
+ return -1;
+ case 0:
+ execv(argv[0], argv);
+ _exit(127);
+ }
+
+ do {
+ pid = waitpid(cpid, &status, 0);
+ } while (pid == -1 && errno == EINTR);
+
+ return (pid == -1 ? -1 : WEXITSTATUS(status));
+}

ikectl-system.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Avoid system(3) in ikectl

Theo de Raadt-2
I'm not sure why this matters.

Fundamentally system is fork+exec via a shell.  So you write it as
minimal fork+exec.

What is the particular benefit you see here, is it security -- and if
so, what is the security benefit?  Have you identified a quoting problem?
Can you pinpoint the issue and explain it please?

> I had sent a similar patch a while back. There seemed to me some
> interest, but it was never comitted. Updated to apply to -current.
>
> Apologies for the attachment; gmail still isn't sending emails sent
> via mutt, but I suspect the patch in the body will be mangled.
>
> - Matthew Martin"
>
> diff --git ikeca.c ikeca.c
> index bac76ab9c2f..01e600abb2c 100644
> --- ikeca.c
> +++ ikeca.c
> @@ -18,6 +18,7 @@
>
>  #include <sys/types.h>
>  #include <sys/stat.h>
> +#include <sys/wait.h>
>  #include <stdio.h>
>  #include <unistd.h>
>  #include <err.h>
> @@ -63,12 +64,12 @@
>
>  struct ca {
>   char sslpath[PATH_MAX];
> - char passfile[PATH_MAX];
> + char passfile[PATH_MAX + 5];
>   char index[PATH_MAX];
>   char serial[PATH_MAX];
>   char sslcnf[PATH_MAX];
>   char extcnf[PATH_MAX];
> - char batch[PATH_MAX];
> + char *batch;
>   char *caname;
>  };
>
> @@ -116,6 +117,7 @@ void ca_setenv(const char *, const char *);
>  void ca_clrenv(void);
>  void ca_setcnf(struct ca *, const char *);
>  void ca_create_index(struct ca *);
> +int static run(char *const []);
>
>  /* util.c */
>  int expand_string(char *, size_t, const char *, const char *);
> @@ -130,7 +132,6 @@ int
>  ca_key_create(struct ca *ca, char *keyname)
>  {
>   struct stat st;
> - char cmd[PATH_MAX * 2];
>   char path[PATH_MAX];
>
>   snprintf(path, sizeof(path), "%s/private/%s.key", ca->sslpath, keyname);
> @@ -140,10 +141,8 @@ ca_key_create(struct ca *ca, char *keyname)
>   return (0);
>   }
>
> - snprintf(cmd, sizeof(cmd),
> -     "%s genrsa -out %s 2048",
> -     PATH_OPENSSL, path);
> - system(cmd);
> + char *cmd[] = { PATH_OPENSSL, "genrsa", "-out", path, "2048", NULL };
> + run(cmd);
>   chmod(path, 0600);
>
>   return (0);
> @@ -200,9 +199,9 @@ ca_delkey(struct ca *ca, char *keyname)
>  int
>  ca_request(struct ca *ca, char *keyname, int type)
>  {
> - char cmd[PATH_MAX * 2];
>   char hostname[HOST_NAME_MAX+1];
>   char name[128];
> + char key[PATH_MAX];
>   char path[PATH_MAX];
>
>   ca_setenv("$ENV::CERT_CN", keyname);
> @@ -226,13 +225,12 @@ ca_request(struct ca *ca, char *keyname, int type)
>
>   ca_setcnf(ca, keyname);
>
> + snprintf(key, sizeof(key), "%s/private/%s.key", ca->sslpath, keyname);
>   snprintf(path, sizeof(path), "%s/private/%s.csr", ca->sslpath, keyname);
> - snprintf(cmd, sizeof(cmd), "%s req %s-new"
> -     " -key %s/private/%s.key -out %s -config %s",
> -     PATH_OPENSSL, ca->batch, ca->sslpath, keyname,
> -     path, ca->sslcnf);
>
> - system(cmd);
> + char *cmd[] = { PATH_OPENSSL, "req", "-new", "-key", key, "-out", path,
> +     "-config", ca->sslcnf, ca->batch, NULL };
> + run(cmd);
>   chmod(path, 0600);
>
>   return (0);
> @@ -241,8 +239,11 @@ ca_request(struct ca *ca, char *keyname, int type)
>  int
>  ca_sign(struct ca *ca, char *keyname, int type)
>  {
> - char cmd[PATH_MAX * 2];
> - const char *extensions = NULL;
> + char cakey[PATH_MAX];
> + char cacrt[PATH_MAX];
> + char out[PATH_MAX];
> + char in[PATH_MAX];
> + char *extensions = NULL;
>
>   if (type == HOST_IPADDR) {
>   extensions = "x509v3_IPAddr";
> @@ -258,19 +259,16 @@ ca_sign(struct ca *ca, char *keyname, int type)
>   ca_setenv("$ENV::CASERIAL", ca->serial);
>   ca_setcnf(ca, keyname);
>
> - snprintf(cmd, sizeof(cmd),
> -     "%s ca -config %s -keyfile %s/private/ca.key"
> -     " -cert %s/ca.crt"
> -     " -extfile %s -extensions %s -out %s/%s.crt"
> -     " -in %s/private/%s.csr"
> -     " -passin file:%s -outdir %s -batch",
> -     PATH_OPENSSL, ca->sslcnf, ca->sslpath,
> -     ca->sslpath,
> -     ca->extcnf, extensions, ca->sslpath, keyname,
> -     ca->sslpath, keyname,
> -     ca->passfile, ca->sslpath);
> + snprintf(cakey, sizeof(cakey), "%s/private/ca.key", ca->sslpath);
> + snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath);
> + snprintf(out, sizeof(out), "%s/%s.crt", ca->sslpath, keyname);
> + snprintf(in, sizeof(in), "%s/private/%s.csr", ca->sslpath, keyname);
>
> - system(cmd);
> + char *cmd[] = { PATH_OPENSSL, "ca", "-config", ca->sslcnf,
> +     "-keyfile", cakey, "-cert", cacrt, "-extfile", ca->extcnf,
> +     "-extensions", extensions, "-out", out, "-in", in,
> +     "-passin", ca->passfile, "-outdir", ca->sslpath, "-batch", NULL };
> + run(cmd);
>
>   return (0);
>  }
> @@ -313,9 +311,9 @@ int
>  ca_key_install(struct ca *ca, char *keyname, char *dir)
>  {
>   struct stat st;
> - char cmd[PATH_MAX * 2];
>   char src[PATH_MAX];
>   char dst[PATH_MAX];
> + char out[PATH_MAX];
>   char *p = NULL;
>
>   snprintf(src, sizeof(src), "%s/private/%s.key", ca->sslpath, keyname);
> @@ -335,9 +333,11 @@ ca_key_install(struct ca *ca, char *keyname, char *dir)
>   snprintf(dst, sizeof(dst), "%s/private/local.key", dir);
>   fcopy(src, dst, 0600);
>
> - snprintf(cmd, sizeof(cmd), "%s rsa -out %s/local.pub"
> -     " -in %s/private/local.key -pubout", PATH_OPENSSL, dir, dir);
> - system(cmd);
> + snprintf(out, sizeof(out), "%s/local.pub", dir);
> +
> + char *cmd[] = { PATH_OPENSSL, "rsa", "-out", out, "-in", dst,
> +     "-pubout", NULL };
> + run(cmd);
>
>   free(p);
>
> @@ -405,37 +405,41 @@ ca_newpass(char *passfile, char *password)
>  int
>  ca_create(struct ca *ca)
>  {
> - char cmd[PATH_MAX * 2];
> - char path[PATH_MAX];
> + char key[PATH_MAX];
> + char csr[PATH_MAX];
> + char crt[PATH_MAX];
>
>   ca_clrenv();
>
> - snprintf(path, sizeof(path), "%s/private/ca.key", ca->sslpath);
> - snprintf(cmd, sizeof(cmd), "%s genrsa -aes256 -out"
> -     " %s -passout file:%s 2048", PATH_OPENSSL,
> -     path, ca->passfile);
> - system(cmd);
> - chmod(path, 0600);
> + snprintf(key, sizeof(key), "%s/private/ca.key", ca->sslpath);
> + {
> + char *cmd[] = { PATH_OPENSSL, "genrsa", "-aes256", "-out", key,
> +     "-passout", ca->passfile, "2048", NULL };
> + run(cmd);
> + }
> + chmod(key, 0600);
>
>   ca_setenv("$ENV::CERT_CN", "VPN CA");
>   ca_setenv("$ENV::REQ_EXT", "x509v3_CA");
>   ca_setcnf(ca, "ca");
>
> - snprintf(path, sizeof(path), "%s/private/ca.csr", ca->sslpath);
> - snprintf(cmd, sizeof(cmd), "%s req %s-new"
> -     " -key %s/private/ca.key"
> -     " -config %s -out %s -passin file:%s", PATH_OPENSSL,
> -     ca->batch, ca->sslpath, ca->sslcnf, path, ca->passfile);
> - system(cmd);
> - chmod(path, 0600);
> -
> - snprintf(cmd, sizeof(cmd), "%s x509 -req -days 4500"
> -     " -in %s/private/ca.csr -signkey %s/private/ca.key"
> -     " -sha256"
> -     " -extfile %s -extensions x509v3_CA -out %s/ca.crt -passin file:%s",
> -     PATH_OPENSSL, ca->sslpath, ca->sslpath, ca->extcnf, ca->sslpath,
> -     ca->passfile);
> - system(cmd);
> + snprintf(csr, sizeof(csr), "%s/private/ca.csr", ca->sslpath);
> + {
> + char *cmd[] = { PATH_OPENSSL, "req", "-new", "-key", key,
> +     "-config", ca->sslcnf, "-out", csr,
> +     "-passin", ca->passfile, ca->batch, NULL };
> + run(cmd);
> + }
> + chmod(csr, 0600);
> +
> + snprintf(crt, sizeof(crt), "%s/ca.crt", ca->sslpath);
> + {
> + char *cmd[] = { PATH_OPENSSL, "x509", "-req", "-days", "4500",
> +     "-in", csr, "-signkey", key, "-sha256",
> +     "-extfile", ca->extcnf, "-extensions", "x509v3_CA",
> +     "-out", crt, "-passin", ca->passfile, NULL };
> + run(cmd);
> + }
>
>   /* Create the CRL revocation list */
>   ca_revoke(ca, NULL);
> @@ -485,7 +489,6 @@ ca_show_certs(struct ca *ca, char *name)
>  {
>   DIR *dir;
>   struct dirent *de;
> - char cmd[PATH_MAX * 2];
>   char path[PATH_MAX];
>   char *p;
>   struct stat st;
> @@ -495,9 +498,11 @@ ca_show_certs(struct ca *ca, char *name)
>       ca->sslpath, name);
>   if (stat(path, &st) != 0)
>   err(1, "could not open file %s.crt", name);
> - snprintf(cmd, sizeof(cmd), "%s x509 -text"
> -     " -in %s", PATH_OPENSSL, path);
> - system(cmd);
> + {
> + char *cmd[] = { PATH_OPENSSL, "x509", "-text",
> +     "-in", path, NULL };
> + run(cmd);
> + }
>   printf("\n");
>   return (0);
>   }
> @@ -512,10 +517,10 @@ ca_show_certs(struct ca *ca, char *name)
>   continue;
>   snprintf(path, sizeof(path), "%s/%s", ca->sslpath,
>       de->d_name);
> - snprintf(cmd, sizeof(cmd), "%s x509 -subject"
> -     " -fingerprint -dates -noout -in %s",
> -     PATH_OPENSSL, path);
> - system(cmd);
> + char *cmd[] = { PATH_OPENSSL, "x509", "-subject",
> +     "-fingerprint", "-dates", "-noout", "-in", path,
> +     NULL };
> + run(cmd);
>   printf("\n");
>   }
>   }
> @@ -649,10 +654,15 @@ ca_export(struct ca *ca, char *keyname, char
> *myname, char *password)
>   struct stat st;
>   char *pass;
>   char prev[_PASSWORD_LEN + 1];
> - char cmd[PATH_MAX * 2];
> + char passenv[_PASSWORD_LEN + 8];
>   char oname[PATH_MAX];
>   char src[PATH_MAX];
>   char dst[PATH_MAX];
> + char cacrt[PATH_MAX];
> + char capfx[PATH_MAX];
> + char key[PATH_MAX];
> + char crt[PATH_MAX];
> + char pfx[PATH_MAX];
>   char *p;
>   char tpl[] = "/tmp/ikectl.XXXXXXXXXX";
>   unsigned int i;
> @@ -682,22 +692,33 @@ ca_export(struct ca *ca, char *keyname, char
> *myname, char *password)
>   errx(1, "passphrase does not match!");
>   }
>
> + snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath);
> + snprintf(capfx, sizeof(capfx), "%s/ca.pfx", ca->sslpath);
> + snprintf(key, sizeof(key), "%s/private/%s.key", ca->sslpath, keyname);
> + snprintf(crt, sizeof(crt), "%s/%s.crt", ca->sslpath, keyname);
> + snprintf(pfx, sizeof(pfx), "%s/private/%s.pfx", ca->sslpath, oname);
> +
> + snprintf(passenv, sizeof(passenv), "EXPASS=%s", pass);
> + putenv(passenv);
> +
>   if (keyname != NULL) {
> - snprintf(cmd, sizeof(cmd), "env EXPASS=%s %s pkcs12 -export"
> -     " -name %s -CAfile %s/ca.crt -inkey %s/private/%s.key"
> -     " -in %s/%s.crt -out %s/private/%s.pfx -passout env:EXPASS"
> -     " -passin file:%s", pass, PATH_OPENSSL, keyname,
> -     ca->sslpath, ca->sslpath, keyname, ca->sslpath, keyname,
> -     ca->sslpath, oname, ca->passfile);
> - system(cmd);
> - }
> -
> - snprintf(cmd, sizeof(cmd), "env EXPASS=%s %s pkcs12 -export"
> -     " -caname '%s' -name '%s' -cacerts -nokeys"
> -     " -in %s/ca.crt -out %s/ca.pfx -passout env:EXPASS -passin file:%s",
> -     pass, PATH_OPENSSL, ca->caname, ca->caname, ca->sslpath,
> -     ca->sslpath, ca->passfile);
> - system(cmd);
> + char *cmd[] = { PATH_OPENSSL, "pkcs12", "-export",
> +     "-name", keyname, "-CAfile", cacrt, "-inkey", key,
> +     "-in", crt, "-out", pfx, "-passout", "env:EXPASS",
> +     "-passin", ca->passfile, NULL };
> + run(cmd);
> + }
> +
> + {
> + char *cmd[] = { PATH_OPENSSL, "pkcs12", "-export",
> +     "-caname", ca->caname, "-name", ca->caname, "-cacerts",
> +     "-nokeys", "-in", cacrt, "-out", capfx,
> +     "-passout", "env:EXPASS", "-passin", ca->passfile, NULL };
> + run(cmd);
> + }
> +
> + unsetenv("EXPASS");
> + explicit_bzero(passenv, sizeof(passenv));
>
>   if ((p = mkdtemp(tpl)) == NULL)
>   err(1, "could not create temp dir");
> @@ -752,21 +773,23 @@ ca_export(struct ca *ca, char *keyname, char
> *myname, char *password)
>   snprintf(dst, sizeof(dst), "%s/certs/%s.crt", p, keyname);
>   fcopy(src, dst, 0644);
>
> - snprintf(cmd, sizeof(cmd), "%s rsa -out %s/local.pub"
> -     " -in %s/private/%s.key -pubout", PATH_OPENSSL, p,
> -     ca->sslpath, keyname);
> - system(cmd);
> + snprintf(dst, sizeof(dst), "%s/local.pub", p);
> + char *cmd[] = { PATH_OPENSSL, "rsa", "-out", dst, "-in", key,
> +     "-pubout", NULL };
> + run(cmd);
>   }
>
>   if (stat(PATH_TAR, &st) == 0) {
> - if (keyname == NULL)
> - snprintf(cmd, sizeof(cmd), "%s -zcf %s.tgz -C %s .",
> -     PATH_TAR, oname, ca->sslpath);
> - else
> - snprintf(cmd, sizeof(cmd), "%s -zcf %s.tgz -C %s .",
> -     PATH_TAR, oname, p);
> - system(cmd);
>   snprintf(src, sizeof(src), "%s.tgz", oname);
> + if (keyname == NULL) {
> + char *cmd[] = { PATH_TAR, "-zcf", src,
> +     "-C", ca->sslpath, ".", NULL };
> + run(cmd);
> + } else {
> + char *cmd[] = { PATH_TAR, "-zcf", src, "-C", p, ".",
> +     NULL };
> + run(cmd);
> + }
>   if (realpath(src, dst) != NULL)
>   printf("exported files in %s\n", dst);
>   }
> @@ -795,8 +818,8 @@ ca_export(struct ca *ca, char *keyname, char
> *myname, char *password)
>   err(1, "could not change %s", dst);
>
>   snprintf(dst, sizeof(dst), "%s/%s.zip", src, oname);
> - snprintf(cmd, sizeof(cmd), "%s -qr %s .", PATH_ZIP, dst);
> - system(cmd);
> + char *cmd[] = { PATH_ZIP, "-qr", dst, ".", NULL };
> + run(cmd);
>   printf("exported files in %s\n", dst);
>
>   if (chdir(src) == -1)
> @@ -849,8 +872,9 @@ int
>  ca_revoke(struct ca *ca, char *keyname)
>  {
>   struct stat st;
> - char cmd[PATH_MAX * 2];
>   char path[PATH_MAX];
> + char cakey[PATH_MAX];
> + char cacrt[PATH_MAX];
>
>   if (keyname) {
>   snprintf(path, sizeof(path), "%s/%s.crt",
> @@ -870,27 +894,21 @@ ca_revoke(struct ca *ca, char *keyname)
>
>   ca_setcnf(ca, "ca-revoke");
>
> + snprintf(cakey, sizeof(cakey), "%s/private/ca.key", ca->sslpath);
> + snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath);
> +
>   if (keyname) {
> - snprintf(cmd, sizeof(cmd),
> -     "%s ca %s-config %s -keyfile %s/private/ca.key"
> -     " -passin file:%s"
> -     " -cert %s/ca.crt"
> -     " -revoke %s/%s.crt",
> -     PATH_OPENSSL, ca->batch, ca->sslcnf,
> -     ca->sslpath, ca->passfile, ca->sslpath, ca->sslpath, keyname);
> - system(cmd);
> - }
> -
> - snprintf(cmd, sizeof(cmd),
> -     "%s ca %s-config %s -keyfile %s/private/ca.key"
> -     " -passin file:%s"
> -     " -gencrl"
> -     " -cert %s/ca.crt"
> -     " -crldays 365"
> -     " -out %s/ca.crl",
> -     PATH_OPENSSL, ca->batch, ca->sslcnf, ca->sslpath,
> -     ca->passfile, ca->sslpath, ca->sslpath);
> - system(cmd);
> + char *cmd[] = { PATH_OPENSSL, "ca", "-config", ca->sslcnf,
> +     "-keyfile", cakey, "-passin", ca->passfile, "-cert", cacrt,
> +     "-revoke", path, ca->batch, NULL };
> + run(cmd);
> + }
> +
> + snprintf(path, sizeof(path), "%s/ca.crl", ca->sslpath);
> + char *cmd[] = { PATH_OPENSSL, "ca", "-config", ca->sslcnf,
> +     "-keyfile", cakey, "-passin", ca->passfile, "-gencrl",
> +     "-cert", cacrt, "-crldays", "365", "-out", path, ca->batch, NULL };
> + run(cmd);
>
>   return (0);
>  }
> @@ -963,11 +981,9 @@ ca_setup(char *caname, int create, int quiet, char *pass)
>
>   ca->caname = strdup(caname);
>   snprintf(ca->sslpath, sizeof(ca->sslpath), SSLDIR "/%s", caname);
> - strlcpy(ca->passfile, ca->sslpath, sizeof(ca->passfile));
> - strlcat(ca->passfile, "/ikeca.passwd", sizeof(ca->passfile));
>
>   if (quiet)
> - strlcpy(ca->batch, "-batch ", sizeof(ca->batch));
> + ca->batch = "-batch";
>
>   if (create == 0 && stat(ca->sslpath, &st) == -1) {
>   free(ca->caname);
> @@ -982,8 +998,31 @@ ca_setup(char *caname, int create, int quiet, char *pass)
>   if (mkdir(path, 0700) == -1 && errno != EEXIST)
>   err(1, "failed to create dir %s", path);
>
> - if (create && stat(ca->passfile, &st) == -1 && errno == ENOENT)
> - ca_newpass(ca->passfile, pass);
> + snprintf(path, sizeof(path), "%s/ikeca.passwd", ca->sslpath);
> + if (create && stat(path, &st) == -1 && errno == ENOENT)
> + ca_newpass(path, pass);
> + snprintf(ca->passfile, sizeof(ca->passfile), "file:%s", path);
>
>   return (ca);
>  }
> +
> +int static
> +run(char *const argv[])
> +{
> + pid_t pid, cpid;
> + int status;
> +
> + switch (cpid = fork()) {
> + case -1:
> + return -1;
> + case 0:
> + execv(argv[0], argv);
> + _exit(127);
> + }
> +
> + do {
> + pid = waitpid(cpid, &status, 0);
> + } while (pid == -1 && errno == EINTR);
> +
> + return (pid == -1 ? -1 : WEXITSTATUS(status));
> +}

Reply | Threaded
Open this post in threaded view
|

Re: Avoid system(3) in ikectl

Stuart Henderson
On 2019/03/06 22:20, Theo de Raadt wrote:
> I'm not sure why this matters.
>
> Fundamentally system is fork+exec via a shell.  So you write it as
> minimal fork+exec.
>
> What is the particular benefit you see here, is it security -- and if
> so, what is the security benefit?  Have you identified a quoting problem?
> Can you pinpoint the issue and explain it please?

> > I had sent a similar patch a while back. There seemed to me some
> > interest, but it was never comitted. Updated to apply to -current.

At the time of the first version of this diff there was a quoting
problem (and a "passwords showing in ps(1)" problem) but it was fixed
differently in ikeca.c:1.46

Reply | Threaded
Open this post in threaded view
|

Re: Avoid system(3) in ikectl

Matthew Martin
On Thu, Mar 7, 2019 at 4:53 AM Stuart Henderson <[hidden email]> wrote:

>
> On 2019/03/06 22:20, Theo de Raadt wrote:
> > I'm not sure why this matters.
> >
> > Fundamentally system is fork+exec via a shell.  So you write it as
> > minimal fork+exec.
> >
> > What is the particular benefit you see here, is it security -- and if
> > so, what is the security benefit?  Have you identified a quoting problem?
> > Can you pinpoint the issue and explain it please?

If an administrator is given doas access to ikectl, they can start
a root shell with doas ikectl ca '; sh; : ' create.

Additionally if a user wants to create a ca or certificate or key with
a special character, it must be double quoted to survive system(3). At
the very least this fact should be documented, but removing the use of
system(3) is the safer and less surprising way.

> > > I had sent a similar patch a while back. There seemed to me some
> > > interest, but it was never comitted. Updated to apply to -current.
>
> At the time of the first version of this diff there was a quoting
> problem (and a "passwords showing in ps(1)" problem) but it was fixed
> differently in ikeca.c:1.46

The export password is still shown in ps(1) since it's put into the
environment via env(1) (cf. ikeca.c 686).

Reply | Threaded
Open this post in threaded view
|

Re: Avoid system(3) in ikectl

Reyk Floeter-2
In reply to this post by Matthew Martin
On Wed, Mar 06, 2019 at 10:42:15PM -0600, Matthew Martin wrote:
> I had sent a similar patch a while back. There seemed to me some
> interest, but it was never comitted. Updated to apply to -current.
>

I vaguely remember that there was a diff that had issues that I didn't
like for different reasons.  The explanation in your other mail (about
using it with doas, escaping of special characters) make some sense,
so I wouldn't be opposed to the idea in general.

But the diff is not OK as it is; a few suggestions:

- I see that "PATH_MAX + 5" is for the "file:" prefix.  This lacks a
comment as it looks confusing at first sight.

- I don't like the way how you run your run() function: declaring a
*cmd[] variable just before calling the function, very often in a
nested {} block, doesn't align to the style C code is written in
OpenBSD.

I think I have also commented the last time that you should use an
execl-interface instead that uses varags terminated by a NULL instead.

int
ca_system(const char *arg, ...);

for example
        ca_system(PATH_OPENSSL, "x509", "-h", NULL);

You can re-construct the argv[] in the function internally, looping
over the va_list.

- And why does run() return int if you never check the return value?
The current system() calls are not checked either but that doesn't
mean that a local function would have to define an unused return
value.  Blindly err'ing out in the function wouldn't help either as I
guess that there are a few calls that might fail if something exists
but allow ikectl to proceed without problems.

Reyk

> diff --git ikeca.c ikeca.c
> index bac76ab9c2f..01e600abb2c 100644
> --- ikeca.c
> +++ ikeca.c
> @@ -18,6 +18,7 @@
>  
>  #include <sys/types.h>
>  #include <sys/stat.h>
> +#include <sys/wait.h>
>  #include <stdio.h>
>  #include <unistd.h>
>  #include <err.h>
> @@ -63,12 +64,12 @@
>  
>  struct ca {
>   char sslpath[PATH_MAX];
> - char passfile[PATH_MAX];
> + char passfile[PATH_MAX + 5];
>   char index[PATH_MAX];
>   char serial[PATH_MAX];
>   char sslcnf[PATH_MAX];
>   char extcnf[PATH_MAX];
> - char batch[PATH_MAX];
> + char *batch;
>   char *caname;
>  };
>  
> @@ -116,6 +117,7 @@ void ca_setenv(const char *, const char *);
>  void ca_clrenv(void);
>  void ca_setcnf(struct ca *, const char *);
>  void ca_create_index(struct ca *);
> +int static run(char *const []);
>  
>  /* util.c */
>  int expand_string(char *, size_t, const char *, const char *);
> @@ -130,7 +132,6 @@ int
>  ca_key_create(struct ca *ca, char *keyname)
>  {
>   struct stat st;
> - char cmd[PATH_MAX * 2];
>   char path[PATH_MAX];
>  
>   snprintf(path, sizeof(path), "%s/private/%s.key", ca->sslpath, keyname);
> @@ -140,10 +141,8 @@ ca_key_create(struct ca *ca, char *keyname)
>   return (0);
>   }
>  
> - snprintf(cmd, sizeof(cmd),
> -    "%s genrsa -out %s 2048",
> -    PATH_OPENSSL, path);
> - system(cmd);
> + char *cmd[] = { PATH_OPENSSL, "genrsa", "-out", path, "2048", NULL };
> + run(cmd);
>   chmod(path, 0600);
>  
>   return (0);
> @@ -200,9 +199,9 @@ ca_delkey(struct ca *ca, char *keyname)
>  int
>  ca_request(struct ca *ca, char *keyname, int type)
>  {
> - char cmd[PATH_MAX * 2];
>   char hostname[HOST_NAME_MAX+1];
>   char name[128];
> + char key[PATH_MAX];
>   char path[PATH_MAX];
>  
>   ca_setenv("$ENV::CERT_CN", keyname);
> @@ -226,13 +225,12 @@ ca_request(struct ca *ca, char *keyname, int type)
>  
>   ca_setcnf(ca, keyname);
>  
> + snprintf(key, sizeof(key), "%s/private/%s.key", ca->sslpath, keyname);
>   snprintf(path, sizeof(path), "%s/private/%s.csr", ca->sslpath, keyname);
> - snprintf(cmd, sizeof(cmd), "%s req %s-new"
> -    " -key %s/private/%s.key -out %s -config %s",
> -    PATH_OPENSSL, ca->batch, ca->sslpath, keyname,
> -    path, ca->sslcnf);
>  
> - system(cmd);
> + char *cmd[] = { PATH_OPENSSL, "req", "-new", "-key", key, "-out", path,
> +    "-config", ca->sslcnf, ca->batch, NULL };
> + run(cmd);
>   chmod(path, 0600);
>  
>   return (0);
> @@ -241,8 +239,11 @@ ca_request(struct ca *ca, char *keyname, int type)
>  int
>  ca_sign(struct ca *ca, char *keyname, int type)
>  {
> - char cmd[PATH_MAX * 2];
> - const char *extensions = NULL;
> + char cakey[PATH_MAX];
> + char cacrt[PATH_MAX];
> + char out[PATH_MAX];
> + char in[PATH_MAX];
> + char *extensions = NULL;
>  
>   if (type == HOST_IPADDR) {
>   extensions = "x509v3_IPAddr";
> @@ -258,19 +259,16 @@ ca_sign(struct ca *ca, char *keyname, int type)
>   ca_setenv("$ENV::CASERIAL", ca->serial);
>   ca_setcnf(ca, keyname);
>  
> - snprintf(cmd, sizeof(cmd),
> -    "%s ca -config %s -keyfile %s/private/ca.key"
> -    " -cert %s/ca.crt"
> -    " -extfile %s -extensions %s -out %s/%s.crt"
> -    " -in %s/private/%s.csr"
> -    " -passin file:%s -outdir %s -batch",
> -    PATH_OPENSSL, ca->sslcnf, ca->sslpath,
> -    ca->sslpath,
> -    ca->extcnf, extensions, ca->sslpath, keyname,
> -    ca->sslpath, keyname,
> -    ca->passfile, ca->sslpath);
> + snprintf(cakey, sizeof(cakey), "%s/private/ca.key", ca->sslpath);
> + snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath);
> + snprintf(out, sizeof(out), "%s/%s.crt", ca->sslpath, keyname);
> + snprintf(in, sizeof(in), "%s/private/%s.csr", ca->sslpath, keyname);
>  
> - system(cmd);
> + char *cmd[] = { PATH_OPENSSL, "ca", "-config", ca->sslcnf,
> +    "-keyfile", cakey, "-cert", cacrt, "-extfile", ca->extcnf,
> +    "-extensions", extensions, "-out", out, "-in", in,
> +    "-passin", ca->passfile, "-outdir", ca->sslpath, "-batch", NULL };
> + run(cmd);
>  
>   return (0);
>  }
> @@ -313,9 +311,9 @@ int
>  ca_key_install(struct ca *ca, char *keyname, char *dir)
>  {
>   struct stat st;
> - char cmd[PATH_MAX * 2];
>   char src[PATH_MAX];
>   char dst[PATH_MAX];
> + char out[PATH_MAX];
>   char *p = NULL;
>  
>   snprintf(src, sizeof(src), "%s/private/%s.key", ca->sslpath, keyname);
> @@ -335,9 +333,11 @@ ca_key_install(struct ca *ca, char *keyname, char *dir)
>   snprintf(dst, sizeof(dst), "%s/private/local.key", dir);
>   fcopy(src, dst, 0600);
>  
> - snprintf(cmd, sizeof(cmd), "%s rsa -out %s/local.pub"
> -    " -in %s/private/local.key -pubout", PATH_OPENSSL, dir, dir);
> - system(cmd);
> + snprintf(out, sizeof(out), "%s/local.pub", dir);
> +
> + char *cmd[] = { PATH_OPENSSL, "rsa", "-out", out, "-in", dst,
> +    "-pubout", NULL };
> + run(cmd);
>  
>   free(p);
>  
> @@ -405,37 +405,41 @@ ca_newpass(char *passfile, char *password)
>  int
>  ca_create(struct ca *ca)
>  {
> - char cmd[PATH_MAX * 2];
> - char path[PATH_MAX];
> + char key[PATH_MAX];
> + char csr[PATH_MAX];
> + char crt[PATH_MAX];
>  
>   ca_clrenv();
>  
> - snprintf(path, sizeof(path), "%s/private/ca.key", ca->sslpath);
> - snprintf(cmd, sizeof(cmd), "%s genrsa -aes256 -out"
> -    " %s -passout file:%s 2048", PATH_OPENSSL,
> -    path, ca->passfile);
> - system(cmd);
> - chmod(path, 0600);
> + snprintf(key, sizeof(key), "%s/private/ca.key", ca->sslpath);
> + {
> + char *cmd[] = { PATH_OPENSSL, "genrsa", "-aes256", "-out", key,
> +    "-passout", ca->passfile, "2048", NULL };
> + run(cmd);
> + }
> + chmod(key, 0600);
>  
>   ca_setenv("$ENV::CERT_CN", "VPN CA");
>   ca_setenv("$ENV::REQ_EXT", "x509v3_CA");
>   ca_setcnf(ca, "ca");
>  
> - snprintf(path, sizeof(path), "%s/private/ca.csr", ca->sslpath);
> - snprintf(cmd, sizeof(cmd), "%s req %s-new"
> -    " -key %s/private/ca.key"
> -    " -config %s -out %s -passin file:%s", PATH_OPENSSL,
> -    ca->batch, ca->sslpath, ca->sslcnf, path, ca->passfile);
> - system(cmd);
> - chmod(path, 0600);
> -
> - snprintf(cmd, sizeof(cmd), "%s x509 -req -days 4500"
> -    " -in %s/private/ca.csr -signkey %s/private/ca.key"
> -    " -sha256"
> -    " -extfile %s -extensions x509v3_CA -out %s/ca.crt -passin file:%s",
> -    PATH_OPENSSL, ca->sslpath, ca->sslpath, ca->extcnf, ca->sslpath,
> -    ca->passfile);
> - system(cmd);
> + snprintf(csr, sizeof(csr), "%s/private/ca.csr", ca->sslpath);
> + {
> + char *cmd[] = { PATH_OPENSSL, "req", "-new", "-key", key,
> +    "-config", ca->sslcnf, "-out", csr,
> +    "-passin", ca->passfile, ca->batch, NULL };
> + run(cmd);
> + }
> + chmod(csr, 0600);
> +
> + snprintf(crt, sizeof(crt), "%s/ca.crt", ca->sslpath);
> + {
> + char *cmd[] = { PATH_OPENSSL, "x509", "-req", "-days", "4500",
> +    "-in", csr, "-signkey", key, "-sha256",
> +    "-extfile", ca->extcnf, "-extensions", "x509v3_CA",
> +    "-out", crt, "-passin", ca->passfile, NULL };
> + run(cmd);
> + }
>  
>   /* Create the CRL revocation list */
>   ca_revoke(ca, NULL);
> @@ -485,7 +489,6 @@ ca_show_certs(struct ca *ca, char *name)
>  {
>   DIR *dir;
>   struct dirent *de;
> - char cmd[PATH_MAX * 2];
>   char path[PATH_MAX];
>   char *p;
>   struct stat st;
> @@ -495,9 +498,11 @@ ca_show_certs(struct ca *ca, char *name)
>      ca->sslpath, name);
>   if (stat(path, &st) != 0)
>   err(1, "could not open file %s.crt", name);
> - snprintf(cmd, sizeof(cmd), "%s x509 -text"
> -    " -in %s", PATH_OPENSSL, path);
> - system(cmd);
> + {
> + char *cmd[] = { PATH_OPENSSL, "x509", "-text",
> +    "-in", path, NULL };
> + run(cmd);
> + }
>   printf("\n");
>   return (0);
>   }
> @@ -512,10 +517,10 @@ ca_show_certs(struct ca *ca, char *name)
>   continue;
>   snprintf(path, sizeof(path), "%s/%s", ca->sslpath,
>      de->d_name);
> - snprintf(cmd, sizeof(cmd), "%s x509 -subject"
> -    " -fingerprint -dates -noout -in %s",
> -    PATH_OPENSSL, path);
> - system(cmd);
> + char *cmd[] = { PATH_OPENSSL, "x509", "-subject",
> +    "-fingerprint", "-dates", "-noout", "-in", path,
> +    NULL };
> + run(cmd);
>   printf("\n");
>   }
>   }
> @@ -649,10 +654,15 @@ ca_export(struct ca *ca, char *keyname, char *myname, char *password)
>   struct stat st;
>   char *pass;
>   char prev[_PASSWORD_LEN + 1];
> - char cmd[PATH_MAX * 2];
> + char passenv[_PASSWORD_LEN + 8];
>   char oname[PATH_MAX];
>   char src[PATH_MAX];
>   char dst[PATH_MAX];
> + char cacrt[PATH_MAX];
> + char capfx[PATH_MAX];
> + char key[PATH_MAX];
> + char crt[PATH_MAX];
> + char pfx[PATH_MAX];
>   char *p;
>   char tpl[] = "/tmp/ikectl.XXXXXXXXXX";
>   unsigned int i;
> @@ -682,22 +692,33 @@ ca_export(struct ca *ca, char *keyname, char *myname, char *password)
>   errx(1, "passphrase does not match!");
>   }
>  
> + snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath);
> + snprintf(capfx, sizeof(capfx), "%s/ca.pfx", ca->sslpath);
> + snprintf(key, sizeof(key), "%s/private/%s.key", ca->sslpath, keyname);
> + snprintf(crt, sizeof(crt), "%s/%s.crt", ca->sslpath, keyname);
> + snprintf(pfx, sizeof(pfx), "%s/private/%s.pfx", ca->sslpath, oname);
> +
> + snprintf(passenv, sizeof(passenv), "EXPASS=%s", pass);
> + putenv(passenv);
> +
>   if (keyname != NULL) {
> - snprintf(cmd, sizeof(cmd), "env EXPASS=%s %s pkcs12 -export"
> -    " -name %s -CAfile %s/ca.crt -inkey %s/private/%s.key"
> -    " -in %s/%s.crt -out %s/private/%s.pfx -passout env:EXPASS"
> -    " -passin file:%s", pass, PATH_OPENSSL, keyname,
> -    ca->sslpath, ca->sslpath, keyname, ca->sslpath, keyname,
> -    ca->sslpath, oname, ca->passfile);
> - system(cmd);
> - }
> -
> - snprintf(cmd, sizeof(cmd), "env EXPASS=%s %s pkcs12 -export"
> -    " -caname '%s' -name '%s' -cacerts -nokeys"
> -    " -in %s/ca.crt -out %s/ca.pfx -passout env:EXPASS -passin file:%s",
> -    pass, PATH_OPENSSL, ca->caname, ca->caname, ca->sslpath,
> -    ca->sslpath, ca->passfile);
> - system(cmd);
> + char *cmd[] = { PATH_OPENSSL, "pkcs12", "-export",
> +    "-name", keyname, "-CAfile", cacrt, "-inkey", key,
> +    "-in", crt, "-out", pfx, "-passout", "env:EXPASS",
> +    "-passin", ca->passfile, NULL };
> + run(cmd);
> + }
> +
> + {
> + char *cmd[] = { PATH_OPENSSL, "pkcs12", "-export",
> +    "-caname", ca->caname, "-name", ca->caname, "-cacerts",
> +    "-nokeys", "-in", cacrt, "-out", capfx,
> +    "-passout", "env:EXPASS", "-passin", ca->passfile, NULL };
> + run(cmd);
> + }
> +
> + unsetenv("EXPASS");
> + explicit_bzero(passenv, sizeof(passenv));
>  
>   if ((p = mkdtemp(tpl)) == NULL)
>   err(1, "could not create temp dir");
> @@ -752,21 +773,23 @@ ca_export(struct ca *ca, char *keyname, char *myname, char *password)
>   snprintf(dst, sizeof(dst), "%s/certs/%s.crt", p, keyname);
>   fcopy(src, dst, 0644);
>  
> - snprintf(cmd, sizeof(cmd), "%s rsa -out %s/local.pub"
> -    " -in %s/private/%s.key -pubout", PATH_OPENSSL, p,
> -    ca->sslpath, keyname);
> - system(cmd);
> + snprintf(dst, sizeof(dst), "%s/local.pub", p);
> + char *cmd[] = { PATH_OPENSSL, "rsa", "-out", dst, "-in", key,
> +    "-pubout", NULL };
> + run(cmd);
>   }
>  
>   if (stat(PATH_TAR, &st) == 0) {
> - if (keyname == NULL)
> - snprintf(cmd, sizeof(cmd), "%s -zcf %s.tgz -C %s .",
> -    PATH_TAR, oname, ca->sslpath);
> - else
> - snprintf(cmd, sizeof(cmd), "%s -zcf %s.tgz -C %s .",
> -    PATH_TAR, oname, p);
> - system(cmd);
>   snprintf(src, sizeof(src), "%s.tgz", oname);
> + if (keyname == NULL) {
> + char *cmd[] = { PATH_TAR, "-zcf", src,
> +    "-C", ca->sslpath, ".", NULL };
> + run(cmd);
> + } else {
> + char *cmd[] = { PATH_TAR, "-zcf", src, "-C", p, ".",
> +    NULL };
> + run(cmd);
> + }
>   if (realpath(src, dst) != NULL)
>   printf("exported files in %s\n", dst);
>   }
> @@ -795,8 +818,8 @@ ca_export(struct ca *ca, char *keyname, char *myname, char *password)
>   err(1, "could not change %s", dst);
>  
>   snprintf(dst, sizeof(dst), "%s/%s.zip", src, oname);
> - snprintf(cmd, sizeof(cmd), "%s -qr %s .", PATH_ZIP, dst);
> - system(cmd);
> + char *cmd[] = { PATH_ZIP, "-qr", dst, ".", NULL };
> + run(cmd);
>   printf("exported files in %s\n", dst);
>  
>   if (chdir(src) == -1)
> @@ -849,8 +872,9 @@ int
>  ca_revoke(struct ca *ca, char *keyname)
>  {
>   struct stat st;
> - char cmd[PATH_MAX * 2];
>   char path[PATH_MAX];
> + char cakey[PATH_MAX];
> + char cacrt[PATH_MAX];
>  
>   if (keyname) {
>   snprintf(path, sizeof(path), "%s/%s.crt",
> @@ -870,27 +894,21 @@ ca_revoke(struct ca *ca, char *keyname)
>  
>   ca_setcnf(ca, "ca-revoke");
>  
> + snprintf(cakey, sizeof(cakey), "%s/private/ca.key", ca->sslpath);
> + snprintf(cacrt, sizeof(cacrt), "%s/ca.crt", ca->sslpath);
> +
>   if (keyname) {
> - snprintf(cmd, sizeof(cmd),
> -    "%s ca %s-config %s -keyfile %s/private/ca.key"
> -    " -passin file:%s"
> -    " -cert %s/ca.crt"
> -    " -revoke %s/%s.crt",
> -    PATH_OPENSSL, ca->batch, ca->sslcnf,
> -    ca->sslpath, ca->passfile, ca->sslpath, ca->sslpath, keyname);
> - system(cmd);
> - }
> -
> - snprintf(cmd, sizeof(cmd),
> -    "%s ca %s-config %s -keyfile %s/private/ca.key"
> -    " -passin file:%s"
> -    " -gencrl"
> -    " -cert %s/ca.crt"
> -    " -crldays 365"
> -    " -out %s/ca.crl",
> -    PATH_OPENSSL, ca->batch, ca->sslcnf, ca->sslpath,
> -    ca->passfile, ca->sslpath, ca->sslpath);
> - system(cmd);
> + char *cmd[] = { PATH_OPENSSL, "ca", "-config", ca->sslcnf,
> +    "-keyfile", cakey, "-passin", ca->passfile, "-cert", cacrt,
> +    "-revoke", path, ca->batch, NULL };
> + run(cmd);
> + }
> +
> + snprintf(path, sizeof(path), "%s/ca.crl", ca->sslpath);
> + char *cmd[] = { PATH_OPENSSL, "ca", "-config", ca->sslcnf,
> +    "-keyfile", cakey, "-passin", ca->passfile, "-gencrl",
> +    "-cert", cacrt, "-crldays", "365", "-out", path, ca->batch, NULL };
> + run(cmd);
>  
>   return (0);
>  }
> @@ -963,11 +981,9 @@ ca_setup(char *caname, int create, int quiet, char *pass)
>  
>   ca->caname = strdup(caname);
>   snprintf(ca->sslpath, sizeof(ca->sslpath), SSLDIR "/%s", caname);
> - strlcpy(ca->passfile, ca->sslpath, sizeof(ca->passfile));
> - strlcat(ca->passfile, "/ikeca.passwd", sizeof(ca->passfile));
>  
>   if (quiet)
> - strlcpy(ca->batch, "-batch ", sizeof(ca->batch));
> + ca->batch = "-batch";
>  
>   if (create == 0 && stat(ca->sslpath, &st) == -1) {
>   free(ca->caname);
> @@ -982,8 +998,31 @@ ca_setup(char *caname, int create, int quiet, char *pass)
>   if (mkdir(path, 0700) == -1 && errno != EEXIST)
>   err(1, "failed to create dir %s", path);
>  
> - if (create && stat(ca->passfile, &st) == -1 && errno == ENOENT)
> - ca_newpass(ca->passfile, pass);
> + snprintf(path, sizeof(path), "%s/ikeca.passwd", ca->sslpath);
> + if (create && stat(path, &st) == -1 && errno == ENOENT)
> + ca_newpass(path, pass);
> + snprintf(ca->passfile, sizeof(ca->passfile), "file:%s", path);
>  
>   return (ca);
>  }
> +
> +int static
> +run(char *const argv[])
> +{
> + pid_t pid, cpid;
> + int status;
> +
> + switch (cpid = fork()) {
> + case -1:
> + return -1;
> + case 0:
> + execv(argv[0], argv);
> + _exit(127);
> + }
> +
> + do {
> + pid = waitpid(cpid, &status, 0);
> + } while (pid == -1 && errno == EINTR);
> +
> + return (pid == -1 ? -1 : WEXITSTATUS(status));
> +}

Reply | Threaded
Open this post in threaded view
|

Re: Avoid system(3) in ikectl

Matthew Martin
On Fri, Mar 8, 2019 at 3:39 AM Reyk Floeter <[hidden email]> wrote:

>
> On Wed, Mar 06, 2019 at 10:42:15PM -0600, Matthew Martin wrote:
> > I had sent a similar patch a while back. There seemed to me some
> > interest, but it was never comitted. Updated to apply to -current.
> >
>
> I vaguely remember that there was a diff that had issues that I didn't
> like for different reasons.  The explanation in your other mail (about
> using it with doas, escaping of special characters) make some sense,
> so I wouldn't be opposed to the idea in general.
>
> But the diff is not OK as it is; a few suggestions:
>
> - I see that "PATH_MAX + 5" is for the "file:" prefix.  This lacks a
> comment as it looks confusing at first sight.
>
> - I don't like the way how you run your run() function: declaring a
> *cmd[] variable just before calling the function, very often in a
> nested {} block, doesn't align to the style C code is written in
> OpenBSD.
>
> I think I have also commented the last time that you should use an
> execl-interface instead that uses varags terminated by a NULL instead.
>
> int
> ca_system(const char *arg, ...);
>
> for example
>         ca_system(PATH_OPENSSL, "x509", "-h", NULL);
>
> You can re-construct the argv[] in the function internally, looping
> over the va_list.
>
> - And why does run() return int if you never check the return value?
> The current system() calls are not checked either but that doesn't
> mean that a local function would have to define an unused return
> value.  Blindly err'ing out in the function wouldn't help either as I
> guess that there are a few calls that might fail if something exists
> but allow ikectl to proceed without problems.
>
> Reyk
I believe I've addressed your comments. The code for turning the varargs
into argv is from execl in lib/libc/gen/exec.c. If the allocation for
argv fails, instead of returning -1 with ENOMEM, I thought it makes more
sense to err out.

ikectl-system.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Avoid system(3) in ikectl

Matthew Martin
ping

On Fri, Mar 8, 2019 at 8:52 PM Matthew Martin <[hidden email]> wrote:

>
> On Fri, Mar 8, 2019 at 3:39 AM Reyk Floeter <[hidden email]> wrote:
> >
> > On Wed, Mar 06, 2019 at 10:42:15PM -0600, Matthew Martin wrote:
> > > I had sent a similar patch a while back. There seemed to me some
> > > interest, but it was never comitted. Updated to apply to -current.
> > >
> >
> > I vaguely remember that there was a diff that had issues that I didn't
> > like for different reasons.  The explanation in your other mail (about
> > using it with doas, escaping of special characters) make some sense,
> > so I wouldn't be opposed to the idea in general.
> >
> > But the diff is not OK as it is; a few suggestions:
> >
> > - I see that "PATH_MAX + 5" is for the "file:" prefix.  This lacks a
> > comment as it looks confusing at first sight.
> >
> > - I don't like the way how you run your run() function: declaring a
> > *cmd[] variable just before calling the function, very often in a
> > nested {} block, doesn't align to the style C code is written in
> > OpenBSD.
> >
> > I think I have also commented the last time that you should use an
> > execl-interface instead that uses varags terminated by a NULL instead.
> >
> > int
> > ca_system(const char *arg, ...);
> >
> > for example
> >         ca_system(PATH_OPENSSL, "x509", "-h", NULL);
> >
> > You can re-construct the argv[] in the function internally, looping
> > over the va_list.
> >
> > - And why does run() return int if you never check the return value?
> > The current system() calls are not checked either but that doesn't
> > mean that a local function would have to define an unused return
> > value.  Blindly err'ing out in the function wouldn't help either as I
> > guess that there are a few calls that might fail if something exists
> > but allow ikectl to proceed without problems.
> >
> > Reyk
>
> I believe I've addressed your comments. The code for turning the varargs
> into argv is from execl in lib/libc/gen/exec.c. If the allocation for
> argv fails, instead of returning -1 with ENOMEM, I thought it makes more
> sense to err out.

ikectl-system.patch (14K) Download Attachment