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

Avoid having to shell-escape arguments in ynh_handle_getopts_args #685

Merged
merged 2 commits into from Apr 1, 2019

Conversation

Projects
None yet
5 participants
@randomstuff
Copy link
Contributor

commented Mar 16, 2019

The problem

Shell characters are not all escaped in eval call.

Solution

Escape shell metacharacters with a dedicated function.

Alternative to #683.

PR Status

...

How to test

#!/bin/sh

. ./getopts

function my_helper()
{
    declare -Ar args_array=( [a]=arg1= [b]=arg2= [c]=arg3 )
    local arg1
    local arg2
    local arg3
    ynh_handle_getopts_args "$@"

    echo "Inner"
    echo arg1=$arg1
    echo arg2=$arg2
    echo arg2=$arg3
    echo
}

my_helper --arg1 '$toto`ls`' -b "'" -c

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :

@randomstuff randomstuff referenced this pull request Mar 16, 2019

Merged

Escape $ in getopts #683

0 of 4 tasks complete

@randomstuff randomstuff force-pushed the randomstuff:shell_escape branch 3 times, most recently from 909c135 to bee6b4b Mar 16, 2019

@randomstuff randomstuff referenced this pull request Mar 16, 2019

Merged

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

0 of 4 tasks complete
@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

I'm not in favor of this PR, the issue is fixed by #683. We do not need to complicate this helper if not absolutely needed.

@randomstuff

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

I actually find it simpler than the alternative:

# Escape double quote to prevent any interpretation during the eval
all_args[$i]="${all_args[$i]//\"/\\\"}"
# Escape $ as well to prevent the string following it to be seen as a variable.
all_args[$i]="${all_args[$i]//$/\\\$}"

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

And it is more correct: the implementation in the snippet does not properly handles backticks and (AFAIU) backslashes which could have usability and security implications.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Calling another function means passing parameters again, with risk of interpretation, using simple quote could have consequences.
It works. I'm don't want to change again because maybe, in a very specific edge case not seen yet, it could be better.
This helper isn't intended to be used by admin, only by helpers used inside apps scripts.

@randomstuff randomstuff force-pushed the randomstuff:shell_escape branch 2 times, most recently from 56a67a2 to b85dd8f Mar 24, 2019

@randomstuff randomstuff force-pushed the randomstuff:shell_escape branch from b85dd8f to 84c6699 Mar 25, 2019

@randomstuff randomstuff changed the title Escape all shell metacharacters in eval Avoid having to shell-escape arguments in ynh_handle_getopts_args Mar 25, 2019

@randomstuff

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Summary of the previous episodes

The current version of ynh_handle_getopts_args does not handles backslashes and backticks correctly.

For reference, the variables are currently assigned values with:

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

It's clear from the code that backticks and backslashes are not escaped properly, as examplified with:

ynh_debug -m '`ls`'
ynh_debug -m '\$(ls)'
ynh_debug -m '\";ls;\"'

As a consequence, backslashes and backticks cannot be used reliably as argument for any function using ynh_handle_getopts_args: it might be a problem for passwords for example.

Moreover, this could turn into a vulnerability issue: if any application calls any of these functions with untrusted input, it could be exploited to inject shell commands.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

Eventually, a real issue...

Real examples would have been better though...

echo '`ls`' >&2
ynh_debug -m '`ls`'

echo '\$(ls)' >&2
ynh_debug -m '\$(ls)'

echo '\";ls;\"' >&2
ynh_debug -m '\";ls;\"'

With the current unstable version of getopts:

Warning: `ls`
Warning: [DEBUG] _common.sh
Warning: backup
Warning: change_url
Warning: install
Warning: remove
Warning: restore
Warning: upgrade
Warning: \$(ls)
Warning: [DEBUG] \_common.sh
Warning: backup
Warning: change_url
Warning: install
Warning: remove
Warning: restore
Warning: upgrade
Warning: \";ls;\"
Warning: /usr/share/yunohost/helpers.d/getopts: line 171: \: command not found

With ` and \ escaped as the other characters.

Warning: `ls`
Warning: [DEBUG] `ls`
Warning: \$(ls)
Warning: [DEBUG] \$(ls)
Warning: \";ls;\"
Warning: [DEBUG] \";ls;\"

With eval ${option_var}+='"${all_args[$i]}"' without escaping characters

Warning: `ls`
Warning: [DEBUG] `ls`
Warning: \$(ls)
Warning: [DEBUG] \$(ls)
Warning: \";ls;\"
Warning: [DEBUG] \";ls;\"
@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

phpmyadmin is currently failing on unstable and testing because of this line where *.* are interpreted as a regex.
Using the current fix correct that issue for phpmyadmin.

Can't see the status of the unstable CI for the moment...
This fix should be tested on more apps.
So, we could merge in unstable to try it.

Keep useful comments...
We still use eval, comment about its usage is still relevant...
@Josue-T
Copy link
Contributor

left a comment

Haven't tested, but look like good.

# But... ${!option_var} can't be used as left part of an assignation.
# declare -g ${option_var} will create a local variable (despite -g !) and will not be available for the helper itself.
# So... Stop fucking arguing each time that eval is evil... Go find an other working solution if you can find one!

This comment has been minimized.

Copy link
@randomstuff

randomstuff Mar 29, 2019

Author Contributor

Still I find eval is evil 😁

@alexAubin

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Soooo, go for merge then ?

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

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

Rather wait for the CI to be available https://ci-apps-unstable.yunohost.org/ci/

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

Actually... It's a false start from the CI on testing.
An upgrade of dovecot has triggered the CI...

The CI will be available in a little while

@alexAubin

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Following discussion in the chat : mergin' !

@alexAubin alexAubin merged commit 8571ad8 into YunoHost:stretch-unstable Apr 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.