Ted Unangst <[email protected]> wrote:
> Matthew Martin wrote:
> > ping
> >
> > On Thu, Apr 25, 2019 at 11:21:00PM -0500, Matthew Martin wrote:
> > > On Thu, Apr 25, 2019 at 08:59:56PM -0600, Theo de Raadt wrote:
> > > > > + argv = alloca((n + 1) * sizeof(*argv));
> > > >
> > > > Our source tree is exceedingly sparing in the use of alloca().
> > > > This will not do.
> > >
> > > Was staying as close as possible to exec.c, but avoiding alloca is
> > > preferable; replaced with reallocarray. err message is "reallocarray" to
> > > match style with the calloc call in the same file.
> > >
> > >
> > - 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);
> > + 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)
> > + ca_system(PATH_OPENSSL, "pkcs12", "-export", "-name", keyname,
> > + "-CAfile", cacrt, "-inkey", key, "-in", crt, "-out", pfx,
> > + "-passout", "env:EXPASS", "-passin", ca->passfile, NULL);
> > +
> > + ca_system(PATH_OPENSSL, "pkcs12", "-export", "-caname", ca->caname,
> > + "-name", ca->caname, "-cacerts", "-nokeys", "-in", cacrt,
> > + "-out", capfx, "-passout", "env:EXPASS", "-passin", ca->passfile,
> > + NULL);
> > +
> > + unsetenv("EXPASS");
> > + explicit_bzero(passenv, sizeof(passenv));
>
> This looks weird, but it's still probably an improvement over the original
> code, which is trying to avoid leaking password via argv but only mostly
> succeeds.
>
> There's probably a better way to do environment handling, but I think this
> diff is better than using system() now. ok with me.
Isn't something like better -- to avoid marshalling code to convert
arguments -> array?
char *pkcs_args[] =
PATH_OPENSSL,
"pkcs12",
"-export",
"-caname",
ca->caname,
"-name",
ca->caname,
"-cacerts",
"-nokeys",
"-in",
cacrt,
"-out",
capfx,
"-passout",
"env:EXPASS",
"-passin",
ca->passfile,
NULL
};
ca_system(pkcs_args);
