Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] Escape double quote before eval in getopts #646

Merged
merged 1 commit into from Feb 11, 2019

Conversation

Projects
None yet
4 participants
@maniackcrudelis
Copy link
Contributor

commented Feb 10, 2019

The problem

Transmission fails on this line while using getopts.

Solution

Escape double quotes before the eval to prevent an interpretation.

PR Status

Ready to be reviewed.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
[fix] Escape double quote before eval in getopts
Work in progress...
Need to be tested.
@alexAubin
Copy link
Member

left a comment

LGTM, you know what you're doing I guess :D

@maniackcrudelis maniackcrudelis referenced this pull request Feb 10, 2019

Merged

Add app debugger helper #647

0 of 4 tasks complete

@maniackcrudelis maniackcrudelis merged commit 2d3ff79 into stretch-unstable Feb 11, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@maniackcrudelis maniackcrudelis deleted the fix_getopts branch Feb 11, 2019

@alexAubin alexAubin added this to the 3.5.x milestone Mar 5, 2019

@JimboJoe

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

getops fails on this line in discourse app on YunoHost 3.5.0 (full log here). Is there a chance that it could be related to that PR...?

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Quoting double quotes is not enough when the argument is in double quotes (you can inject $(...), you can \", etc.). A more correct solution is to do something like shell_quote in python.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

Hum, interesting case...
Clearly what's happening here is that your argument has eventually been interpreted as a variable because of the $ at the beginning. Did you try with a simple escape ?
Quote are interpreted anyway when sent to a function.

If you're comfortable with getopts, try to debug by removing the set +x in getopts, you'll see then what's happening.

@JimboJoe

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

I tried with

 ynh_replace_string "\$proxy_add_x_forwarded_for" "\$http_your_original_ip_header" "/etc/nginx/conf.d/$domain.d/$app.conf"

with no more luck...
I'm not comfortable at all with getops 😅 and agree to @randomstuff general approach because there may be other corner cases ($, ${, etc.)

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Except that it's absolutely not related to how python is passing arguments or whatever to bash...
Here it's about how bash is passing arguments to each functions. And that's not that simple to not interpret special characters.

I think in your case JimboJoe, it's about the eval in getopts, I'll have a look to it.

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

Here it's about how bash is passing arguments to each functions.

Well in normal bash commands it's not super hard. The problem here is using eval which its potential to shell metacharacters interpretation (and shell command injection). eval is really quite evil and should be avoided if possible.

If you're restricting to bash, you should be able to do something like:

declare ${option_var}+="${arguments[$i]}"
@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

eval is evil because it can interpret every bash command and execute it, I know. But there's no reason here to have any injection. And as explained before, I can't see any reason for an admin with a root permission (needed to run those scripts) to try to inject anything since he can simply do whatever he wants.

We're using eval here to get the content of the variable stored as a string in arguments[$i], not to get the content of this cell of the array.

eval ${option_var}+=\"${arguments[$i]}\"

Gives the content of the variable in ${arguments[$i]}

declare ${option_var}+="${arguments[$i]}"

Would gives the content of ${arguments[$i]} as a string.

If ${arguments[$i]}contains val1, withevalwe'd have the content of$val1, without we'd have val1` as a simple string.

@JimboJoe I do not reproduce, whether with the stable version of getopts or with the unstable version.
In both case, I got

sudo sed --in-place 's@$proxy_add_x_forwarded_for@$http_your_original_ip_header@g' /etc/nginx/conf.d/crudelis-test6.fr.d/discourse.conf
@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

@maniackcrudelis, are you sure?

I'm trying this:

#!/bin/sh

val1=whatever
arguments=(a b c val1 e f g)
option_var=res
i=3

echo Test 0
echo ${arguments[$i]}
echo

echo Test 1
res=""
eval ${option_var}+=\"${arguments[$i]}\"
echo $res
echo

echo Test 2
res=""
declare ${option_var}+="${arguments[$i]}"
echo $res
echo

I get:

Test 0
val1

Test 1
val1

Test 2
val1

However with:

#!/bin/sh

val1=whatever
arguments=(a b c '${val1}' e f g)
option_var=res
i=3

echo Test 0
echo ${arguments[$i]}
echo

echo Test 1
res=""
eval ${option_var}+=\"${arguments[$i]}\"
echo $res
echo

echo Test 2
res=""
declare ${option_var}+="${arguments[$i]}"
echo $res
echo

I get:

Test 0
${val1}

Test 1
whatever

Test 2
${val1}

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

I reread the code, indeed @randomstuff, that's the opposite, that's ${option_var} which is interpreted to get the variable inside. (Just written above in the code actually)

Hum ok JimboJoe, my ynh_replace_string was still on stable. I now reproduce.
I see that it's happening indeed during the eval

getopts-L199: eval 'match_string+="$proxy_add_x_forwarded_for"'

I'll try other ways to get the variable contents, the thing is that I know that I put aside the 2 other ways I know. But what was the reason by this time...

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

So, from my tests (With the unstable/testing version of getopts)

  • It works by using '\$proxy_add_x_forwarded_for' instead of '$proxy_add_x_forwarded_for'
    So just by escaping the $.
  • With "\$proxy_add_x_forwarded_for", double quotes instead of simple, it doesn't, which is expected because double quotes are immediately interpreted by bash. So the backslash after.
  • ${!option_var} without eval doesn't work in the left part of a assignation.
  • declare ${option_var}+=\"${arguments[$i]}\" doesn't work either because declare create a local variable.
  • declare -g ${option_var}+=\"${arguments[$i]}\" doesn't work either.
    -g is supposed to fix the behavior of declare by forcing the variable to a global scope.

There was indeed a good reason to decide at this time to use eval instead of those 2 other ways to get the content of the variable...

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

As your syntax is correct @JimboJoe, escaping $ should be done on getopts part.
By the way, I tried many other characters (exactly those: \ ' & % + - ) ( } { ] [ = # @ ! / , ;) in order to be sure, either inside double or simple quotes for the entire string.
Tried with ynh_replace_special_string to not trig any sed command.

Only $ seems to be a problem, as eval tries to interpret the text following $ as a variable.

Thanks JimboJoe for this bug report, I was expecting some edge cases from this testing release.

@maniackcrudelis maniackcrudelis referenced this pull request Mar 16, 2019

Merged

Escape $ in getopts #683

0 of 4 tasks complete
@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

@maniackcrudelis, what about:

declare -g ${option_var}+="${arguments[$i]}"

Edit: does not work (see below).

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

You still have backtick and $(...) shell command subtitution working:

my_helper --arg1 'aa`ls`' -b "'" -c
my_helper --arg1 'aa$(ls)' -b "'" -c

Gives (both);

arg1=aabackend debug filesystem getopts ip mysql network nodejs package print psql setting string system testit.sh user utils
@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

-g is supposed to fix the behavior of declare by forcing the variable to a global scope.

For reference, the reason -g does not work is that it makes the variable appear in the global scope (top-level), not the caller local scope.

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

@maniackcrudelis, I think you did not understand what I intended to say (my formulation was somewhat ambiguous) in:

A more correct solution is to do something like shell_quote in python.

What I meant to say that a similar quoting strategy should be implemented in bash script than the one which is implemented in Python in shell_quote(). (Which is what #685 is attempting to do.)

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

I can't see any reason for an admin with a root permission (needed to run those scripts) to try to inject anything since he can simply do whatever he wants.

Of course the admin does not need to inject himself. However he may need to use shell metacharacters in arguments.

At some point, the admin may want to use these helpers in scripts (that's the reason CLI are nice after all). Suppose someone does something like that:

ynh_print_warn -m "Status of remote service is $(curl https://remoteserver/status)"

ynh_print_warn is currently relying on ynh_handle_getopts_args which does backtick shell command expansion under the hood so remote server gets the ability to injection shell commands.

Simplified version:

ynh_print_warn -m '`ls`'  # Compare to: echo '`ls`'
@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

By the way, I tried many other characters (exactly those: \ ' & % + - ) ( } { ] [ = # @ ! / , ;) in order to be sure, either inside double or simple quotes for the entire string.
Tried with ynh_replace_special_string to not trig any sed command.

Only $ seems to be a problem, as eval tries to interpret the text following $ as a variable.

It works perfectly as it is.
Using $() will start a sub shell, which could have side effects, adding simple quote could have also side effects, should be tested to be sure.
Anyway, getopts is here a helper used by other helpers, there's no meaning in testing solution into a simple bash script. Fixes have to be tested in real situation with an app script.

With the current fix it works. With all characters.

@randomstuff

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

With the current fix it works. With all characters.

Not with backticks.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

Backticks are deprecated in favor of $().
As getopts is a helper of helpers, finally meant to be used by apps. To not cover that edge case isn't a problem for me.
Again, it works as it is. We'll see if other cases emerge from the testing or usages.
But I'm interested in usage according to the purpose of this helper. Not in diverted usages for other purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.