POSIX incompatibility in acme-client

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

POSIX incompatibility in acme-client

Wolf
Hi,
me again with another thing about acme-client. This time I run into an
issue with how it determines certdir. It uses

        if ((certdir = dirname(domain->cert)) != NULL) {
                if ((certdir = strdup(certdir)) == NULL)

which is wrong according to POSIX spec. Citing from
http://pubs.opengroup.org/onlinepubs/9699919799/ :

        The dirname() function may modify the string pointed to by path, and may
        return a pointer to internal storage.

On linux (at least on Alpine with musl-c), it actually does modify the
argument. That means that it changes `domain->cert', which introduces
issues later on.

I assume this currently is not an issue on openbsd since it probably
takes the second route (`... a pointer to internal storage.').

This patch fixes the issue for me:

diff --git a/main.c b/main.c
index fb895f1..d682658 100644
--- a/main.c
+++ b/main.c
@@ -37,7 +37,7 @@ int
 main(int argc, char *argv[])
 {
        const char       **alts = NULL;
-       char             *certdir = NULL, *certfile = NULL;
+       char             *certdir = NULL, *certdir_src = NULL, *certfile = NULL;
        char             *chainfile = NULL, *fullchainfile = NULL;
        char             *acctkey = NULL;
        char             *chngdir = NULL, *auth = NULL;
@@ -104,20 +104,13 @@ main(int argc, char *argv[])
        argc--;
        argv++;

-       if (domain->cert != NULL) {
-               if ((certdir = dirname(domain->cert)) != NULL) {
-                       if ((certdir = strdup(certdir)) == NULL)
-                               err(EXIT_FAILURE, "strdup");
-               } else
-                       err(EXIT_FAILURE, "dirname");
-       } else {
-               /* the parser enforces that at least cert or fullchain is set */
-               if ((certdir = dirname(domain->fullchain)) != NULL) {
-                       if ((certdir = strdup(certdir)) == NULL)
-                               err(EXIT_FAILURE, "strdup");
-               } else
-                       err(EXIT_FAILURE, "dirname");
-
+       /* the parser enforces that at least cert or fullchain is set */
+       certdir_src = domain->cert ? domain->cert : domain->fullchain;
+       if ((certdir_src = strdup(certdir_src)) == NULL) {
+               err(EXIT_FAILURE, "strdup");
+       }
+       if ((certdir = dirname(certdir_src)) == NULL) {
+               err(EXIT_FAILURE, "dirname");
        }

        if (domain->cert != NULL) {
@@ -415,6 +408,7 @@ main(int argc, char *argv[])
            checkexit(pids[COMP_DNS], COMP_DNS) +
            checkexit(pids[COMP_REVOKE], COMP_REVOKE);

+       free(certdir_src);
        free(alts);
        return rc != COMP__MAX ? EXIT_FAILURE : (c == 2 ? EXIT_SUCCESS : 2);
 usage:



Output of `acme-client -ADFv db.wolfsden.cz' before

        ...
        acme-client: /etc/acme/acme: created
        acme-client: /etc/acme/db.wolfsden.cz.pem: created

and after:

        ...
        acme-client: /etc/acme/db.wolfsden.cz.crt: created
        acme-client: /etc/acme/db.wolfsden.cz.pem: created

I'm not sure if you consider this a problem, but thought I would let you
know.

Have a nice day,
Gray Wolf

--
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: POSIX incompatibility in acme-client

Sebastian Benoit-3
For the mailing list archive: fixed in commits
usr.sbin/acme-client/main.c rev 1.42 and 1.43.

Thanks fo reporting this bug.

/Benno

Wolf([hidden email]) on 2019.03.08 11:44:58 +0100:

> Hi,
> me again with another thing about acme-client. This time I run into an
> issue with how it determines certdir. It uses
>
> if ((certdir = dirname(domain->cert)) != NULL) {
> if ((certdir = strdup(certdir)) == NULL)
>
> which is wrong according to POSIX spec. Citing from
> http://pubs.opengroup.org/onlinepubs/9699919799/ :
>
> The dirname() function may modify the string pointed to by path, and may
> return a pointer to internal storage.
>
> On linux (at least on Alpine with musl-c), it actually does modify the
> argument. That means that it changes `domain->cert', which introduces
> issues later on.
>
> I assume this currently is not an issue on openbsd since it probably
> takes the second route (`... a pointer to internal storage.').
>
> This patch fixes the issue for me:
>
> diff --git a/main.c b/main.c
> index fb895f1..d682658 100644
> --- a/main.c
> +++ b/main.c
> @@ -37,7 +37,7 @@ int
>  main(int argc, char *argv[])
>  {
>         const char       **alts = NULL;
> -       char             *certdir = NULL, *certfile = NULL;
> +       char             *certdir = NULL, *certdir_src = NULL, *certfile = NULL;
>         char             *chainfile = NULL, *fullchainfile = NULL;
>         char             *acctkey = NULL;
>         char             *chngdir = NULL, *auth = NULL;
> @@ -104,20 +104,13 @@ main(int argc, char *argv[])
>         argc--;
>         argv++;
>
> -       if (domain->cert != NULL) {
> -               if ((certdir = dirname(domain->cert)) != NULL) {
> -                       if ((certdir = strdup(certdir)) == NULL)
> -                               err(EXIT_FAILURE, "strdup");
> -               } else
> -                       err(EXIT_FAILURE, "dirname");
> -       } else {
> -               /* the parser enforces that at least cert or fullchain is set */
> -               if ((certdir = dirname(domain->fullchain)) != NULL) {
> -                       if ((certdir = strdup(certdir)) == NULL)
> -                               err(EXIT_FAILURE, "strdup");
> -               } else
> -                       err(EXIT_FAILURE, "dirname");
> -
> +       /* the parser enforces that at least cert or fullchain is set */
> +       certdir_src = domain->cert ? domain->cert : domain->fullchain;
> +       if ((certdir_src = strdup(certdir_src)) == NULL) {
> +               err(EXIT_FAILURE, "strdup");
> +       }
> +       if ((certdir = dirname(certdir_src)) == NULL) {
> +               err(EXIT_FAILURE, "dirname");
>         }
>
>         if (domain->cert != NULL) {
> @@ -415,6 +408,7 @@ main(int argc, char *argv[])
>             checkexit(pids[COMP_DNS], COMP_DNS) +
>             checkexit(pids[COMP_REVOKE], COMP_REVOKE);
>
> +       free(certdir_src);
>         free(alts);
>         return rc != COMP__MAX ? EXIT_FAILURE : (c == 2 ? EXIT_SUCCESS : 2);
>  usage:
>
>
>
> Output of `acme-client -ADFv db.wolfsden.cz' before
>
> ...
> acme-client: /etc/acme/acme: created
> acme-client: /etc/acme/db.wolfsden.cz.pem: created
>
> and after:
>
> ...
> acme-client: /etc/acme/db.wolfsden.cz.crt: created
> acme-client: /etc/acme/db.wolfsden.cz.pem: created
>
> I'm not sure if you consider this a problem, but thought I would let you
> know.
>
> Have a nice day,
> Gray Wolf
>
> --
> There are only two hard things in Computer Science:
> cache invalidation, naming things and off-by-one errors.