bug-bash
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Incorrect / Inconsistent behavior with nameref assignments in functi


From: Binarus
Subject: Re: Incorrect / Inconsistent behavior with nameref assignments in functions
Date: Mon, 31 Aug 2020 09:57:13 +0200
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0

On 30.08.2020 20:01, Koichi Murase wrote:
> 2020-08-30 19:24 Binarus <lists@binarus.de>:

> Yes, I recognize the problem when the function isn't properly used.
> But, the use of eval itself is not fatal.  When another user can call
> the function as in your example,
> 
>   Dummy 'myArray1[@]}"); echo Gotcha!; #'
> 
> that means that the user can already run arbitrary commands.  The user
> could have directly written
> 
>   echo 'Gotcha!'
> 
> The real problems occur when the user write like
> 
>   Dummy "$input_to_program"
> 
> with `input_to_program' provided by the third user who should not be
> able to run arbitrary commands, and the input is not checked nor
> sanitized.  In this case, the problem should be evaded by checking or
> sanitizing the input.  This check can be made inside the function
> Dummy, but it is also possible to make it at the time when the shell
> receives the input.

Thank you very much for the clarification. Of course, you are right. If
I would provide the functions in a file which can be sourced by users,
security isn't a problem at all (except in cases where users
accidentally make mistakes which lead to the HDD being purged ...). But
if I wrap them in a script and provide the interface via command line
parameters, and that script runs SUID, this imposes a new class of problems.

>> declare -a -i myArray1=('1' '2' '3')
>> Dummy 'myArray1[@]}"); echo Gotcha!; #'
>>
>> Output:
>>
>> root@cerberus:~/scripts# ./test6
>> Gotcha!
>> declare -a myArray=([0]="1" [1]="2" [2]="3")
> 
> Unfortunately, your original function `Dummy' also has the same
> vulnerability.  As Greg has written, there are many other places that
> cause the command execution in the shell because that is the purpose
> of the shell.  With your original method,
> 
>   $ cat testR2d.sh
>   function Dummy {
>     local -n namerefArray="$1"
>     local -a -i myArray=("${namerefArray[@]}")
>   }
>   myArray1=("$@")
>   Dummy 'myArray1'
>   $ bash testR2d.sh 'a[$(echo Gotcha1 >/dev/tty)]'
>   Gotcha1
> 
> This is caused by the arithmetic evaluation caused by `-i' flag.  In
> arithmetic evaluations, the array subscripts are subject to the extra
> expansions so that the string $(echo Gotcha1 >/dev/tty) is expanded as
> a command substitution.
> 
> Actually, the nameref also has the same behavior, so the use of
> `nameref' is not much safer than the use of `eval'.
> 
>   $ cat testR2e.sh
>   function Dummy2 {
>     local -n namerefScalar=$1
>     local var=$namerefScalar
>   }
>   Dummy2 "$1"
>   $ bash testR2e.sh 'a[$(echo Gotcha2 >/dev/tty)]'
>   Gotcha2
> 
> Yes, I guess it would be a valid strategy to disallow any use of
> `eval' because humans will make mistakes no matter how careful we are.
> But, there are still different traps, so anyway we need to carefully
> check or sanitize inputs even when we don't use `eval'.

Thank you very much for these insights! Actually, I didn't think
systematically about command line interfaces and safety yet, because
these scripts won't run SUID and don't provide command line interfaces.
However, the eval code injection stared me in the face, so I thought I'd
comment about it. Probably I shouldn't have done so, because I actually
at least don't have that problem at the moment, and because I don't
intend to to harden theses scripts anyway; as it currently stands, only
root will be able to execute them.

>> I guess it would be very complicated, if possible at all, to protect
>> the code inside eval against every sort of such attacks.
> 
> I think the standard way is to check the input before passing it to
> `eval' and is not complicated.  You can just check if the array name
> has an expected form:
> 
>   function is-valid-array-name {
>     local reg='^[_[:alpha:]][_[:alnum:]]*$'
>     [[ $1 =~ $reg ]]
>   }
> 
>   # Check it inside Dummy
>   function Dummy {
>     is-valid-array-name "$1" || return 1
>     [[ $1 == myArray ]] || eval "local -a myArray=(\"\${$1[@]}\")"
>     declare -p myArray
>   }
> 
>   # Or check it when it receives the array name (I prefer this)
>   is-valid-array-name "$1" || exit 1
>   input_data=$1
>   Dummy "$input_data"

Thank you very much. These snippets are nice and clean. I'll file all
your messages and code patterns and use them as reference if I ever
write a bash script again.

>>> * If you want to use namerefs to eliminate the use of `eval', maybe
>>>   you could do like the following [...]
>>
>> However (hoping that I don't get flamed for a dumb question), I
>> don't understand why we need inputArray at all in that code.
> 
> Sorry, I should have explained it in detail.  The step of `inputArray'
> is only needed when you want to modify `myArray' locally in the
> function `Dummy' keeping the original array unmodified.  Without
> `inputArray',
> 
>   $ cat testR2f.sh
>   function Dummy {
>     [[ $1 == refArray ]] || local -n refArray=$1
>     [[ $1 == myArray ]] || local -ia myArray=("${refArray[@]}")
>     myArray[0]=${myArray[0]%/}
>   }
>   myArray=(my/dir/)
>   declare -p Dummy
>   Dummy myArray
>   declare -p Dummy
>   $ bash testR2f.sh
>   declare -a myArray=([0]="my/dir/")
>   declare -a myArray=([0]="my/dir")
> 
> This is because, when the outer array has the same name `myArray', the
> function Dummy sees the outer array directly instead of the local
> copy.

I wrongly assumed that this code would be for bash 5.1 where we didn't
need to work around my original problem, but only around the circular
name reference problem (in bash 5.1, if I got it right, we could declare
myArray without any precondition, so it would be local in any case). I
apologize for this misunderstanding.

>> Wouldn't the following function be sufficient?
>>
>> function Dummy {
>>   [[ $1 == refArray ]] || local -n refArray=$1
>>   local -ia myArray=("${refArray[@]}")
>>   declare -p myArray
>> }
> 
> No, that function solves the problem of the collision with `refArray'
> (the circular reference) but not the problem of the collision with
> `myArray' (the problem in your original post).

See previous comment - I assumed that we were talking about bash 5.1.
Sorry for not having clarified this ...

>> Unfortunately, these solutions (while solving the circular reference
>> problem) don't solve my original problem.
> 
> Have you tried?  In the examples in my previous reply, I intended to
> solve both problems with older versions of Bash.

You are absolutely right. I have been too fast, and the day was too long:

I immediately concentrated on your third example, because I generally
try to avoid eval (although it is not per se evil, as I have learned
now). Then I looked into bash 5.1 and came to the conclusion that my
Dummy from above (three lines in the body without inputArray) is
equivalent to your third example in bash 5.1.

Some hours later, I saw that my Dummy did not solve my original problem
in previous bash versions, and had forgotten that your example had one
line more :-) Silly me!

In summary, thanks to your help, we now have a clean and understandable
solution for my original problem as well as for the circular reference
problem.

>> I think I'll stick with my current (extremely ugly, but reliable)
>> solution
> 
> Yes, I think that is the simplest solution in your case.

This is the only thing where I dare to not agree :-) Your solution is so
far superior to this ugly hack that I will begin to change my scripts
immediately after having finished this message. Decorating variables
with numbers is so silly and hard to maintain that it can only be a last
resort; furthermore, it breaks my existing naming conventions.

Thank you very much for sharing your invaluable knowledge and for
putting so much time and effort into helping us!

Best regards,

Binarus



reply via email to

[Prev in Thread] Current Thread [Next in Thread]