diff mbox

Never try to execute the file in ldd

Message ID mvma9cfobqi.fsf@hawking.suse.de
State New
Headers show

Commit Message

Andreas Schwab March 24, 2014, 3:30 p.m. UTC
Executing a random file is never a good idea.  Treat all arguments as if
they are invoked with __libc_enable_secure, and run them through the known
good dynamic linker.

	* elf/ldd.bash.in: Always run through the dynamic linker, even if
	the file has its own interpreter.  Remove unneeded executable
	check.
---
 elf/ldd.bash.in | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

Comments

Dmitry V. Levin March 24, 2014, 7:45 p.m. UTC | #1
On Mon, Mar 24, 2014 at 04:30:29PM +0100, Andreas Schwab wrote:
> Executing a random file is never a good idea.  Treat all arguments as if
> they are invoked with __libc_enable_secure, and run them through the known
> good dynamic linker.
> 
> 	* elf/ldd.bash.in: Always run through the dynamic linker, even if
> 	the file has its own interpreter.  Remove unneeded executable
> 	check.

I've been rebasing this fix since 2002.  Please commit.

> ---
>  elf/ldd.bash.in | 16 +---------------
>  1 file changed, 1 insertion(+), 15 deletions(-)
> 
> diff --git a/elf/ldd.bash.in b/elf/ldd.bash.in
> index 4ff140d..3986bcf 100644
> --- a/elf/ldd.bash.in
> +++ b/elf/ldd.bash.in
> @@ -150,8 +150,6 @@ for file do
>      echo "ldd: ${file}:" $"not regular file" >&2
>      result=1
>    elif test -r "$file"; then
> -    test -x "$file" || echo 'ldd:' $"\
> -warning: you do not have execution permission for" "\`$file'" >&2
>      RTLD=
>      ret=1
>      for rtld in ${RTLDLIST}; do
> @@ -164,18 +162,6 @@ warning: you do not have execution permission for" "\`$file'" >&2
>        fi
>      done
>      case $ret in
> -    0)
> -      # If the program exits with exit code 5, it means the process has been
> -      # invoked with __libc_enable_secure.  Fall back to running it through
> -      # the dynamic linker.
> -      try_trace "$file"
> -      rc=$?
> -      if [ $rc = 5 ]; then
> -	try_trace "$RTLD" "$file"
> -	rc=$?
> -      fi
> -      [ $rc = 0 ] || result=1
> -      ;;
>      1)
>        # This can be a non-ELF binary or no binary at all.
>        nonelf "$file" || {
> @@ -183,7 +169,7 @@ warning: you do not have execution permission for" "\`$file'" >&2
>  	result=1
>        }
>        ;;
> -    2)
> +    [02])
>        try_trace "$RTLD" "$file" || result=1
>        ;;
>      *)
Roland McGrath March 24, 2014, 10:10 p.m. UTC | #2
I always thought it wrong that it did that too.  But I vaguely recall being
told there was some reason for it.  (Maybe even I thought myself there was
an adequate reason.  I can't recall any details now.)  So we should
understand what the past reasoning was and be sure it no longer applies
today before we make such a change.

The only thing that comes to mind is cases where PT_INTERP points to a
different dynamic linker, such as a from build with a special --prefix=
setup or something stranger.  In those cases, what the vanilla rtld will
think about search paths and so forth won't match what the actual PT_INTERP
dynamic linker will do.

But I'm not at all sure that was the case (or was the only case) that
motivated the current behavior.
Rich Felker March 25, 2014, 3:11 a.m. UTC | #3
On Mon, Mar 24, 2014 at 03:10:23PM -0700, Roland McGrath wrote:
> I always thought it wrong that it did that too.  But I vaguely recall being
> told there was some reason for it.  (Maybe even I thought myself there was
> an adequate reason.  I can't recall any details now.)  So we should
> understand what the past reasoning was and be sure it no longer applies
> today before we make such a change.
> 
> The only thing that comes to mind is cases where PT_INTERP points to a
> different dynamic linker, such as a from build with a special --prefix=
> setup or something stranger.  In those cases, what the vanilla rtld will
> think about search paths and so forth won't match what the actual PT_INTERP
> dynamic linker will do.
> 
> But I'm not at all sure that was the case (or was the only case) that
> motivated the current behavior.

If there's really a need to support this kind of usage, I think by
default ldd should refuse to run when PT_INTERP doesn't match its own
idea of the dynamic linker, and should require a --force-run option or
something. In the default setup, it's completely non-obvious to most
admins that ldd _runs_ the program, and the "hey, root! this program
is spewing missing symbol errors!" social-engineering exploit is a
real risk.

Rich
diff mbox

Patch

diff --git a/elf/ldd.bash.in b/elf/ldd.bash.in
index 4ff140d..3986bcf 100644
--- a/elf/ldd.bash.in
+++ b/elf/ldd.bash.in
@@ -150,8 +150,6 @@  for file do
     echo "ldd: ${file}:" $"not regular file" >&2
     result=1
   elif test -r "$file"; then
-    test -x "$file" || echo 'ldd:' $"\
-warning: you do not have execution permission for" "\`$file'" >&2
     RTLD=
     ret=1
     for rtld in ${RTLDLIST}; do
@@ -164,18 +162,6 @@  warning: you do not have execution permission for" "\`$file'" >&2
       fi
     done
     case $ret in
-    0)
-      # If the program exits with exit code 5, it means the process has been
-      # invoked with __libc_enable_secure.  Fall back to running it through
-      # the dynamic linker.
-      try_trace "$file"
-      rc=$?
-      if [ $rc = 5 ]; then
-	try_trace "$RTLD" "$file"
-	rc=$?
-      fi
-      [ $rc = 0 ] || result=1
-      ;;
     1)
       # This can be a non-ELF binary or no binary at all.
       nonelf "$file" || {
@@ -183,7 +169,7 @@  warning: you do not have execution permission for" "\`$file'" >&2
 	result=1
       }
       ;;
-    2)
+    [02])
       try_trace "$RTLD" "$file" || result=1
       ;;
     *)