diff mbox series

patch resend updated: PR28240, debuginfod cache for root

Message ID 20210930183656.GA21634@redhat.com
State Under Review
Headers show
Series patch resend updated: PR28240, debuginfod cache for root | expand

Commit Message

Frank Ch. Eigler Sept. 30, 2021, 6:36 p.m. UTC
commit 56f40abf2cf18775deb25797e1b89e7788b80630 (HEAD -> master)
Author: Frank Ch. Eigler <fche@redhat.com>
Date:   Wed Aug 18 18:29:34 2021 -0400

    PR28240: debuginfod client root-safe negative caching
    
    Negative cache (000-permission) files were incorrectly treated as
    valid cached files for the root user, because root can open even
    000-perm files without -EACCES.  Corrected this checking sequence.
    
    Fixed the debuginfod testsuite to run to completion as root or
    as an ordinary user, correcting corresponding permission checks:
        stat -c %A $FILE
    is right and
        [ -w $FILE]  [ -r $FILE ]
    were wrong.
    
    Signed-off-by: Frank Ch. Eigler <fche@redhat.com>

Comments

Mark Wielaard Oct. 5, 2021, 5:18 p.m. UTC | #1
Hi Frank,

On Thu, 2021-09-30 at 14:36 -0400, Frank Ch. Eigler via Elfutils-devel
wrote:
> @@ -766,24 +766,14 @@ debuginfod_query_server (debuginfod_client *c,
>    if (rc != 0)
>      goto out;
>  
> -  /* If the target is already in the cache then we are done.  */
> -  int fd = open (target_cache_path, O_RDONLY);
> -  if (fd >= 0)
> -    {
> -      /* Success!!!! */
> -      if (path != NULL)
> -        *path = strdup(target_cache_path);
> -      rc = fd;
> -      goto out;
> -    }
> -
>    struct stat st;
> -  time_t cache_miss;
> -  /* Check if the file exists and it's of permission 000*/
> -  if (errno == EACCES
> -      && stat(target_cache_path, &st) == 0
> +  /* Check if the file exists and it's of permission 000; must check
> +     explicitly rather than trying to open it first (PR28240). */
> +  if (stat(target_cache_path, &st) == 0
>        && (st.st_mode & 0777) == 0)
>      {
> +      time_t cache_miss;
> +
>        rc = debuginfod_config_cache(cache_miss_path,
> cache_miss_default_s, &st);
>        if (rc < 0)
>          goto out;
> @@ -797,6 +787,18 @@ debuginfod_query_server (debuginfod_client *c,
>        else
>          unlink(target_cache_path);
>      }
> +  
> +  /* If the target is already in the cache, and not a 000 file
> (PR28240), 
> +     then we are done. */
> +  int fd = open (target_cache_path, O_RDONLY);
> +  if (fd >= 0)
> +    {
> +      /* Success!!!! */
> +      if (path != NULL)
> +        *path = strdup(target_cache_path);
> +      rc = fd;
> +      goto out;
> +    }

Isn't this a time-of-check to time-of-use (TOCTOU) race now?
At least for the root user?
 
>    long timeout = default_timeout;
>    const char* timeout_envvar = getenv(DEBUGINFOD_TIMEOUT_ENV_VAR);
> diff --git a/tests/ChangeLog b/tests/ChangeLog
> index 578e8cb15502..e5aca513f1e8 100644
> --- a/tests/ChangeLog
> +++ b/tests/ChangeLog
> @@ -1,3 +1,9 @@
> +2021-09-30  Frank Ch. Eigler  <fche@redhat.com>
> +
> +	PR28240
> +	* run-debuginfod-000-permission.sh, -writable.sh:
> +	Correct negative-cache file permission checking.
> +
>  2021-09-30  Frank Ch. Eigler  <fche@redhat.com>
>  
>  	* run-debuginfod-*.sh: Use ! CMD to assert expected failure.
> diff --git a/tests/run-debuginfod-000-permission.sh b/tests/run-
> debuginfod-000-permission.sh
> index 8480ad486886..301ce7e7d9d3 100755
> --- a/tests/run-debuginfod-000-permission.sh
> +++ b/tests/run-debuginfod-000-permission.sh
> @@ -56,7 +56,7 @@ if [ ! -f $DEBUGINFOD_CACHE_PATH/01234567/debuginfo
> ]; then
>    err
>  fi
>  
> -if [ -r $DEBUGINFOD_CACHE_PATH/01234567/debuginfo ]; then
> +if [ `stat -c "%A" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` != "
> ----------" ]; then
>    echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debuginfo is
> readable"
>    err
>  fi
> diff --git a/tests/run-debuginfod-artifact-running.sh b/tests/run-
> debuginfod-artifact-running.sh
> index 51fa9c0a40bd..b94444267ade 100755
> --- a/tests/run-debuginfod-artifact-running.sh
> +++ b/tests/run-debuginfod-artifact-running.sh
> @@ -90,10 +90,6 @@ wait_ready $PORT1 'thread_busy{role="scan"}' 0
>  rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
>  filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find
> debuginfo $BUILDID`
>  cmp $filename F/prog.debug
> -if [ -w $filename ]; then
> -    echo "cache file writable, boo"
> -    err
> -fi

I assume you are just removing the test here, instead of replacing,
because it is already tested in the next one?

>  filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find
> executable F/prog`
>  cmp $filename F/prog
> diff --git a/tests/run-debuginfod-writable.sh b/tests/run-debuginfod-
> writable.sh
> index 69ececb39454..9cc4ea1db92c 100755
> --- a/tests/run-debuginfod-writable.sh
> +++ b/tests/run-debuginfod-writable.sh
> @@ -79,7 +79,7 @@ wait_ready $PORT1 'thread_busy{role="scan"}' 0
>  rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
>  filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find
> debuginfo $BUILDID`
>  cmp $filename F/p+r%o\$g.debug
> -if [ -w $filename ]; then
> +if [  `stat -c "%A" $filename` != "-r--------" ]; then
>      echo "cache file writable, boo"
>      err
>  fi

Cheers,

Mark
diff mbox series

Patch

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index c2bfce98fbd1..b37d6482dba3 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,9 @@ 
+2021-09-30  Frank Ch. Eigler  <fche@redhat.com>
+
+	PR28240
+	* debuginfod-client.c (debuginfod_query_server): Correct
+	negative-hit cache check sequence for root user.
+
 2021-09-17  Noah Sanci  <nsanci@redhat.com>
 
 	* debuginfod-client.c (debuginfod_query_server): curl_multi_perform
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 88e45567d44a..43ed4b3b9191 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1,5 +1,5 @@ 
 /* Retrieve ELF / DWARF / source files from the debuginfod.
-   Copyright (C) 2019-2020 Red Hat, Inc.
+   Copyright (C) 2019-2021 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -766,24 +766,14 @@  debuginfod_query_server (debuginfod_client *c,
   if (rc != 0)
     goto out;
 
-  /* If the target is already in the cache then we are done.  */
-  int fd = open (target_cache_path, O_RDONLY);
-  if (fd >= 0)
-    {
-      /* Success!!!! */
-      if (path != NULL)
-        *path = strdup(target_cache_path);
-      rc = fd;
-      goto out;
-    }
-
   struct stat st;
-  time_t cache_miss;
-  /* Check if the file exists and it's of permission 000*/
-  if (errno == EACCES
-      && stat(target_cache_path, &st) == 0
+  /* Check if the file exists and it's of permission 000; must check
+     explicitly rather than trying to open it first (PR28240). */
+  if (stat(target_cache_path, &st) == 0
       && (st.st_mode & 0777) == 0)
     {
+      time_t cache_miss;
+
       rc = debuginfod_config_cache(cache_miss_path, cache_miss_default_s, &st);
       if (rc < 0)
         goto out;
@@ -797,6 +787,18 @@  debuginfod_query_server (debuginfod_client *c,
       else
         unlink(target_cache_path);
     }
+  
+  /* If the target is already in the cache, and not a 000 file (PR28240), 
+     then we are done. */
+  int fd = open (target_cache_path, O_RDONLY);
+  if (fd >= 0)
+    {
+      /* Success!!!! */
+      if (path != NULL)
+        *path = strdup(target_cache_path);
+      rc = fd;
+      goto out;
+    }
 
   long timeout = default_timeout;
   const char* timeout_envvar = getenv(DEBUGINFOD_TIMEOUT_ENV_VAR);
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 578e8cb15502..e5aca513f1e8 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,9 @@ 
+2021-09-30  Frank Ch. Eigler  <fche@redhat.com>
+
+	PR28240
+	* run-debuginfod-000-permission.sh, -writable.sh:
+	Correct negative-cache file permission checking.
+
 2021-09-30  Frank Ch. Eigler  <fche@redhat.com>
 
 	* run-debuginfod-*.sh: Use ! CMD to assert expected failure.
diff --git a/tests/run-debuginfod-000-permission.sh b/tests/run-debuginfod-000-permission.sh
index 8480ad486886..301ce7e7d9d3 100755
--- a/tests/run-debuginfod-000-permission.sh
+++ b/tests/run-debuginfod-000-permission.sh
@@ -56,7 +56,7 @@  if [ ! -f $DEBUGINFOD_CACHE_PATH/01234567/debuginfo ]; then
   err
 fi
 
-if [ -r $DEBUGINFOD_CACHE_PATH/01234567/debuginfo ]; then
+if [ `stat -c "%A" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` != "----------" ]; then
   echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debuginfo is readable"
   err
 fi
diff --git a/tests/run-debuginfod-artifact-running.sh b/tests/run-debuginfod-artifact-running.sh
index 51fa9c0a40bd..b94444267ade 100755
--- a/tests/run-debuginfod-artifact-running.sh
+++ b/tests/run-debuginfod-artifact-running.sh
@@ -90,10 +90,6 @@  wait_ready $PORT1 'thread_busy{role="scan"}' 0
 rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
 filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID`
 cmp $filename F/prog.debug
-if [ -w $filename ]; then
-    echo "cache file writable, boo"
-    err
-fi
 
 filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find executable F/prog`
 cmp $filename F/prog
diff --git a/tests/run-debuginfod-writable.sh b/tests/run-debuginfod-writable.sh
index 69ececb39454..9cc4ea1db92c 100755
--- a/tests/run-debuginfod-writable.sh
+++ b/tests/run-debuginfod-writable.sh
@@ -79,7 +79,7 @@  wait_ready $PORT1 'thread_busy{role="scan"}' 0
 rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
 filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID`
 cmp $filename F/p+r%o\$g.debug
-if [ -w $filename ]; then
+if [  `stat -c "%A" $filename` != "-r--------" ]; then
     echo "cache file writable, boo"
     err
 fi