Deal with dl_open_by_name() toctou (CIS #1400053) (#4799)

* Deal with dl_open_by_name() toctou (CIS #1400053)

dl_open_by_name() gives you the option of looking though a list of
directories for a library, using the dlopen() function. If a dlopen()
succeeds, dl_open_by_name() succeeds and all is well. If it fails,
though, the question is why? If it doesn't exist in that directory,
you want to try the next on the list. If it does, permissions are
likely to be the problem, so report the issue and return failure.

The previous code uses access() on the path if the dlopen() fails,
hence toctou.

Unfortunately, there's not a good way to deal with it.

There is no dlopenat(), so one can't avoid toctou with dlopenat()
followed by accessat(). The only indication of why dlopen() failed
is in the string returned by dlerror()... which only guarantees
that the string is human-readable, NUL-terminated, and doesn't end
with a newline.

Looking at the result of dlerror() when the dlopen() for the
directory list fails suggests that it has the following format:

path COLON SPACE <message> COLON SPACE strerror_output

In the particular cases in the tests, that last part was always
"No such file or directory", what strerror() returns for the
ENOENT dl_open_by_name() is checking for.

As the least bad alternative, the code looks in the dlerror()
output for fr_syserror(ENOENT) to decide.

* Add note

Co-authored-by: Arran Cudbard-Bell <a.cudbardb@freeradius.org>

diff --git a/src/lib/util/dl.c b/src/lib/util/dl.c
index 6527d9b01cfb6363658c4cd3d1cdb4c1c44c8e6e..de82285d56068a331e426216e5d28ea3df37e75c 100644 (file)
--- a/src/lib/util/dl.c
+++ b/src/lib/util/dl.c
@@ -528,7 +528,7 @@ dl_t *dl_by_name(dl_loader_t *dl_loader, char const *name, void *uctx, bool uctx
 
                ctx = paths = talloc_typed_strdup(NULL, search_path);
                while ((path = strsep(&paths, ":")) != NULL) {
-                       int access_mode = R_OK | X_OK;
+                       char *dlerror_txt;
 
                        /*
                         *      Trim the trailing slash
@@ -538,40 +538,52 @@ dl_t *dl_by_name(dl_loader_t *dl_loader, char const *name, void *uctx, bool uctx
 
                        path = talloc_typed_asprintf(ctx, "%s/%s%s", path, name, DL_EXTENSION);
                        handle = dlopen(path, flags);
-                       if (handle) {
-                               talloc_free(path);
-                               break;
-                       }
+                       talloc_free(path);
+                       if (handle) break;
 
-#ifdef AT_ACCESS
-                       access_mode |= AT_ACCESS;
-#endif
+                       /*
+                        *      There's no dlopenat(), so one can't use it and later
+                        *      check acessatat() to avoid toctou.
+                        *
+                        *      The only indication of why dlopen() failed is thus the
+                        *      contents of the string dlerror() returns, but all the
+                        *      man page says about that is that it's "human readable",
+                        *      NUL-terminated, and doesn't end with a newline.
+                        *
+                        *      The following attempts to use the dlerror() output to
+                        *      determine whether dlopen() failed because path doesn't
+                        *      exist. This goes beyond what the API specifies (Hyrum's
+                        *      Law strikes again!), but the alternatives are
+                        *
+                        *      1. looking into the data structure dlerror() uses
+                        *      2. let the toctou remain
+                        *
+                        *      both of which seem worse.
+                        */
 
                        /*
-                        *      Check if the dlopen() failed
-                        *      because of access permissions.
+                        *      If the file doesn't exist, continue with the next element
+                        *      of  "path". Otherwise, stop looking for more libraries
+                        *      and instead complain about access permissions.
                         */
-                       /* coverity[fs_check_call] */
-                       if (access(path, access_mode) < 0) {
-                               /*
-                                *      It doesn't exist,
-                                *      continue with the next
-                                *      element of "path".
-                                */
-                               if (errno == ENOENT) continue;
-
-                               /*
-                                *      Stop looking for more
-                                *      libraries, and instead
-                                *      complain about access
-                                *      permissions.
-                                */
-                               fr_strerror_printf("Access check failed for %s - %s", path, fr_syserror(errno));
-                               talloc_free(path);
+                       dlerror_txt = dlerror();
+                       
+                       /*
+                        *      Yes, this really is the only way of getting the errno
+                        *      from the dlopen API.
+                        *
+                       if (!strstr(dlerror_txt, fr_syserror(ENOENT))) {
+#ifndef __linux__
+                               int access_mode = R_OK | X_OK;
+
+#ifdef AT_ACCESS
+                               access_mode |= AT_ACCESS;
+#endif
+                               if (access(path, access_mode) < 0 && errno == ENOENT) continue;
+#endif
+                               fr_strerror_printf("Access check failed: %s", dlerror_txt);
                                break;
                        }
-
-                       talloc_free(path);
                }
 
                /*