Hi,
I started a review of ikeca.c about the use of system(3). But as there
is a lot of others place of improvement in the code, I prefer to go
step by step, and proposing patches only for one function at time.
The following patch is a review of ca_export() function.
It adds:
- xsnprintf() function: same as snprintf() but check the return, and
call errx() on error.
It corrects:
- an invalid use of err() instead of errx()
- an invalid mode in fcopy (644 instead of 0644)
- check return for strlcpy (or add (void) casting on safe use),
chmod(2), realpath(3), replace snprintf(3) by xsnprintf()...
- convert system(3) to vfork(2)/execv(2)
The reason of the use of vfork(2) instead of fork(2) is that I follow
how system(3) is implemented (with vfork the parent is suspended until
the child makes a call to execve(2) or an exit).
Comments ? OK ?
--
Sebastien Marie
Index: ikeca.c
===================================================================
RCS file: /cvs/src/usr.sbin/ikectl/ikeca.c,v
retrieving revision 1.30
diff -u -p -r1.30 ikeca.c
--- ikeca.c 16 Jan 2015 06:40:17 -0000 1.30
+++ ikeca.c 30 Jul 2015 09:39:44 -0000
@@ -18,6 +18,8 @@
#include <sys/types.h>
#include <sys/stat.h>
+#include <sys/wait.h>
+
#include <stdio.h>
#include <unistd.h>
#include <err.h>
@@ -90,6 +92,20 @@ int fcopy(char *, char *, mode_t);
int rm_dir(char *);
int ca_hier(char *);
+static void
+xsnprintf(char *str, size_t size, const char *format, ...)
+{
+ va_list ap;
+ int ret;
+
+ va_start(ap, format);
+ ret = vsnprintf(str, size, format, ap);
+ va_end(ap);
+
+ if ((ret == -1) || ((size_t)ret >= size))
+ errx(1, "vsnprintf: output truncated");
+}
+
int
ca_delete(struct ca *ca)
{
@@ -550,20 +566,23 @@ ca_export(struct ca *ca, char *keyname,
struct stat st;
char *pass;
char prev[_PASSWORD_LEN + 1];
- char cmd[PATH_MAX * 2];
char oname[PATH_MAX];
char src[PATH_MAX];
char dst[PATH_MAX];
char *p;
char tpl[] = "/tmp/ikectl.XXXXXXXXXX";
- u_int i;
int fd;
+ pid_t pid;
+ int status;
+ char *argv[18];
+ int argc;
+ char *envp[2];
if (keyname != NULL) {
if (strlcpy(oname, keyname, sizeof(oname)) >= sizeof(oname))
- err(1, "name too long");
+ errx(1, "name too long");
} else {
- strlcpy(oname, "ca", sizeof(oname));
+ (void)strlcpy(oname, "ca", sizeof(oname));
}
/* colons are not valid characters in windows filenames... */
@@ -577,99 +596,227 @@ ca_export(struct ca *ca, char *keyname,
if (pass == NULL || *pass == '\0')
err(1, "password not set");
- strlcpy(prev, pass, sizeof(prev));
+ (void)strlcpy(prev, pass, sizeof(prev));
pass = getpass("Retype export passphrase:");
if (pass == NULL || strcmp(prev, pass) != 0)
errx(1, "passphrase does not match!");
}
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);
+ switch (pid = vfork()) {
+ case -1: /* error */
+ err(1, "vfork");
+
+ case 0: /* child */
+ /* XXX pass in env is unsafe */
+ if (asprintf(&envp[0], "EXPASS=%s", pass) <= 7)
+ exit(1);
+ envp[1] = NULL;
+
+ argc = 0;
+ argv[argc++] = PATH_OPENSSL;
+ argv[argc++] = "pkcs12";
+ argv[argc++] = "-export";
+ argv[argc++] = "-name";
+ argv[argc++] = keyname;
+ argv[argc++] = "-CAfile";
+ if (asprintf(&argv[argc++], "%s/ca.crt", ca->sslpath)
+ <= 8)
+ exit(1);
+ argv[argc++] = "-inkey";
+ if (asprintf(&argv[argc++], "%s/private/%s.key",
+ ca->sslpath, keyname) <= 15)
+ exit(1);
+ argv[argc++] = "-in";
+ if (asprintf(&argv[argc++], "%s/%s.crt", ca->sslpath,
+ keyname) <= 7)
+ exit(1);
+ argv[argc++] = "-out";
+ if (asprintf(&argv[argc++], "%s/private/%s.pfx",
+ ca->sslpath, oname) <= 15)
+ exit(1);
+ argv[argc++] = "-passout";
+ argv[argc++] = "env:EXPASS";
+ argv[argc++] = "-passin";
+ if (asprintf(&argv[argc++], "file:%s", ca->passfile)
+ <= 6)
+ exit(1);
+ argv[argc++] = NULL;
+
+ execve(PATH_OPENSSL, argv, envp);
+ exit(1);
+ }
+ if (waitpid(pid, &status, 0) < 0)
+ err(1, "waitpid");
+
+ if (status != 0)
+ errx(1, "export command failed: %s/private/%s.pfx",
+ ca->sslpath, oname);
}
- 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);
+
+ switch (pid = vfork()) {
+ case -1: /* error */
+ err(1, "vfork");
+
+ case 0: /* child */
+ /* XXX pass in env is unsafe */
+ if (asprintf(&envp[0], "EXPASS=%s", pass) <= 7)
+ exit(1);
+ envp[1] = NULL;
+
+ argc = 0;
+ argv[argc++] = PATH_OPENSSL;
+ argv[argc++] = "pkcs12";
+ argv[argc++] = "-export";
+ argv[argc++] = "-caname";
+ argv[argc++] = ca->caname;
+ argv[argc++] = "-name";
+ argv[argc++] = ca->caname;
+ argv[argc++] = "-cacerts";
+ argv[argc++] = "-nokeys";
+ argv[argc++] = "-in";
+ if (asprintf(&argv[argc++], "%s/ca.crt", ca->sslpath) <= 8)
+ exit(1);
+ argv[argc++] = "-out";
+ if (asprintf(&argv[argc++], "%s/ca.pfx", ca->sslpath) <= 8)
+ exit(1);
+ argv[argc++] = "-passout";
+ argv[argc++] = "env:EXPASS";
+ argv[argc++] = "-passin";
+ if (asprintf(&argv[argc++], "file:%s", ca->passfile) <= 6)
+ exit(1);
+ argv[argc++] = NULL;
+
+ execve(PATH_OPENSSL, argv, envp);
+ exit(1);
+ }
+ if (waitpid(pid, &status, 0) < 0)
+ err(1, "waitpid");
+
+ if (status != 0)
+ errx(1, "export command failed: %s/ca.pfx", ca->sslpath);
+
if ((p = mkdtemp(tpl)) == NULL)
err(1, "could not create temp dir");
- chmod(p, 0755);
+ if (chmod(p, 0755) != 0)
+ err(1, "chmod");
- for (i = 0; i < nitems(hier); i++) {
- strlcpy(dst, p, sizeof(dst));
- strlcat(dst, hier[i].dir, sizeof(dst));
- if (stat(dst, &st) != 0 && errno == ENOENT &&
- mkdir(dst, hier[i].mode) != 0)
- err(1, "failed to create dir %s", dst);
- }
+ ca_hier(p);
/* create a file with the address of the peer to connect to */
if (myname != NULL) {
- snprintf(dst, sizeof(dst), "%s/export/peer.txt", p);
+ xsnprintf(dst, sizeof(dst), "%s/export/peer.txt", p);
+
if ((fd = open(dst, O_WRONLY|O_CREAT, 0644)) == -1)
err(1, "open %s", dst);
write(fd, myname, strlen(myname));
close(fd);
}
- snprintf(src, sizeof(src), "%s/ca.pfx", ca->sslpath);
- snprintf(dst, sizeof(dst), "%s/export/ca.pfx", p);
- fcopy(src, dst, 0644);
-
- snprintf(src, sizeof(src), "%s/ca.crt", ca->sslpath);
- snprintf(dst, sizeof(dst), "%s/ca/ca.crt", p);
- fcopy(src, dst, 0644);
+ xsnprintf(src, sizeof(src), "%s/ca.pfx", ca->sslpath);
+ xsnprintf(dst, sizeof(dst), "%s/export/ca.pfx", p);
+ if (fcopy(src, dst, 0644) != 0)
+ errx(1, "fcopy: %s -> %s", src, dst);
+
+ xsnprintf(src, sizeof(src), "%s/ca.crt", ca->sslpath);
+ xsnprintf(dst, sizeof(dst), "%s/ca/ca.crt", p);
+ if (fcopy(src, dst, 0644) != 0)
+ errx(1, "fcopy: %s -> %s", src, dst);
- snprintf(src, sizeof(src), "%s/ca.crl", ca->sslpath);
+ xsnprintf(src, sizeof(src), "%s/ca.crl", ca->sslpath);
if (stat(src, &st) == 0) {
- snprintf(dst, sizeof(dst), "%s/crls/ca.crl", p);
- fcopy(src, dst, 0644);
+ xsnprintf(dst, sizeof(dst), "%s/crls/ca.crl", p);
+ if (fcopy(src, dst, 0644) != 0)
+ errx(1, "fcopy: %s -> %s", src, dst);
}
if (keyname != NULL) {
- snprintf(src, sizeof(src), "%s/private/%s.pfx", ca->sslpath,
+ xsnprintf(src, sizeof(src), "%s/private/%s.pfx", ca->sslpath,
oname);
- snprintf(dst, sizeof(dst), "%s/export/%s.pfx", p, oname);
- fcopy(src, dst, 0644);
+ xsnprintf(dst, sizeof(dst), "%s/export/%s.pfx", p, oname);
+ if (fcopy(src, dst, 0644) != 0)
+ errx(1, "fcopy: %s -> %s", src, dst);
- snprintf(src, sizeof(src), "%s/private/%s.key", ca->sslpath,
+ xsnprintf(src, sizeof(src), "%s/private/%s.key", ca->sslpath,
keyname);
- snprintf(dst, sizeof(dst), "%s/private/%s.key", p, keyname);
- fcopy(src, dst, 0600);
- snprintf(dst, sizeof(dst), "%s/private/local.key", p);
- fcopy(src, dst, 0600);
-
- snprintf(src, sizeof(src), "%s/%s.crt", ca->sslpath, keyname);
- 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);
+ xsnprintf(dst, sizeof(dst), "%s/private/%s.key", p, keyname);
+ if (fcopy(src, dst, 0600) != 0)
+ errx(1, "fcopy: %s -> %s", src, dst);
+ xsnprintf(dst, sizeof(dst), "%s/private/local.key", p);
+ if (fcopy(src, dst, 0600) != 0)
+ errx(1, "fcopy: %s -> %s", src, dst);
+
+ xsnprintf(src, sizeof(src), "%s/%s.crt", ca->sslpath, keyname);
+ xsnprintf(dst, sizeof(dst), "%s/certs/%s.crt", p, keyname);
+ if (fcopy(src, dst, 0644) != 0)
+ errx(1, "fcopy: %s -> %s", src, dst);
+
+ switch (pid = vfork()) {
+ case -1: /* error */
+ err(1, "vfork");
+
+ case 0: /* child */
+ argc = 0;
+ argv[argc++] = PATH_OPENSSL;
+ argv[argc++] = "rsa";
+ argv[argc++] = "-out";
+ if (asprintf(&argv[argc++], "%s/local.pub", p) <= 11)
+ exit(1);
+ argv[argc++] = "-in";
+ if (asprintf(&argv[argc++], "%s/private/%s.key",
+ ca->sslpath, keyname) <= 15)
+ exit(1);
+ argv[argc++] = "-pubout";
+ argv[argc++] = NULL;
+
+ execv(PATH_OPENSSL, argv);
+ exit(1);
+
+ }
+ if (waitpid(pid, &status, 0) < 0)
+ err(1, "waitpid");
+
+ if (status != 0)
+ errx(1, "export command failed: %s/local.pub",
+ ca->sslpath);
}
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);
+ switch (pid= vfork()) {
+ case -1: /* error */
+ err(1, "vfork");
+
+ case 0: /* child */
+ argc = 0;
+ argv[argc++] = PATH_TAR;
+ argv[argc++] = "-zcf";
+ if (asprintf(&argv[argc++], "%s.tgz", oname) <= 5)
+ exit(1);
+ argv[argc++] = "-C";
+ if (keyname == NULL)
+ argv[argc++] = ca->sslpath;
+ else
+ argv[argc++] = p;
+ argv[argc++] = ".";
+ argv[argc++] = NULL;
+
+ execv(PATH_TAR, argv);
+ exit(1);
+ }
+ if (waitpid(pid, &status, 0) < 0)
+ err(1, "waitpid");
+
+ if (status != 0)
+ errx(1, "export command failed: %s.tgz", oname);
+
+ xsnprintf(src, sizeof(src), "%s.tgz", oname);
if (realpath(src, dst) != NULL)
printf("exported files in %s\n", dst);
+ else
+ err(1, "realpath: %s", src);
}
if (stat(PATH_ZIP, &st) == 0) {
@@ -679,25 +826,47 @@ ca_export(struct ca *ca, char *keyname,
if (!strcmp(de->d_name, ".") ||
!strcmp(de->d_name, ".."))
continue;
- snprintf(src, sizeof(src), "%s/%s", EXPDIR,
+ xsnprintf(src, sizeof(src), "%s/%s", EXPDIR,
de->d_name);
- snprintf(dst, sizeof(dst), "%s/export/%s", p,
+ xsnprintf(dst, sizeof(dst), "%s/export/%s", p,
de->d_name);
- fcopy(src, dst, 644);
+ if (fcopy(src, dst, 0644) != 0)
+ errx(1, "fcopy: %s -> %s", src, dst);
}
closedir(dexp);
}
- snprintf(dst, sizeof(dst), "%s/export", p);
+ xsnprintf(dst, sizeof(dst), "%s/export", p);
if (getcwd(src, sizeof(src)) == NULL)
err(1, "could not get cwd");
if (chdir(dst) == -1)
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);
+ switch (pid = vfork()) {
+ case -1: /* error */
+ err(1, "vfork");
+
+ case 0: /* child */
+ argc = 0;
+ argv[argc++] = PATH_ZIP;
+ argv[argc++] = "-qr";
+ if (asprintf(&argv[argc++], "%s/%s.zip", src, oname)
+ <= 7)
+ exit(1);
+ argv[argc++] = ".";
+ argv[argc++] = NULL;
+
+ execv(PATH_ZIP, argv);
+ exit(1);
+ }
+ if (waitpid(pid, &status, 0) < 0)
+ err(1, "waitpid");
+
+ if (status != 0)
+ errx(1, "export command failed: %s/%s.zip", src, oname);
+
+ xsnprintf(dst, sizeof(dst), "%s/%s.zip", src, oname);
printf("exported files in %s\n", dst);
if (chdir(src) == -1)
