makeself

makeself copied to clipboard

When passing ; to the inner script the rest of the arguments are interpreted as a command to be executed:

$ cat pkg/test.sh
echo "Arguments:"
for arg in "$@"; do
  echo "- $arg"
done
echo "Done!"
$ makeself pkg/ runner.sh "Args tester" ./test.sh >/dev/null 2>&1
$ ./runner.sh -- "one;two"
Verifying archive integrity... All good.
Uncompressing Args tester  100%
Arguments:
- one
Done!
./runner.sh: 1: eval: two: not found

This allows execution of commands:

$ ./runner.sh -- "dummy;wc -l /etc/passwd"
Verifying archive integrity... All good.
Uncompressing Args tester  100%
Arguments:
- one
Done!
49 /etc/passwd

Relates to #57.

Good catch - what do you propose we do? Ignore everything after ; ?

At the same time this doesn't exactly trigger privilege escalation but I can see how injecting commands could be a security risk in certain cases.

So from skimming the code, I think the issue is with how scriptargs is built. It's first constructed with SCRIPTARGS="$*" in makeself.sh and then scriptargs="$SCRIPTARGS" in makeself-header.sh. Then finally used:

eval "\"\$script\" \$scriptargs \"\\\$@\""; res=\$?

Which will be (partly) expended as:

eval "\"\$script\" one;two \"\\\$@\""; res=\$?

Ignoring everything after the ; isn't good option IMO, since it'll essentially drop what should've been valid input to the script by the user (since ./runner.sh -- "foo;ls" is different than ./runner.sh -- foo;ls).

In bash I know of easy (enough) ways to either escape the variable or build the array, but sadly I don't know how to do that in sh...

I also think it's more than just ;, there are other operators you could use to chain commands, such as && or even &

To support this, makeself would need to change it's invocation signature to:

Usage: ./makeself.sh [params] archive_dir file_name label startup_script_and_args

where startup_script_and_args would be a single string that then gets stored and later passed to sh -c. That breaks compatibility with older scripting that might invoke makeself.

There's an open issue #112 where someone already tried to use this invocation and it's not an uncommon pattern for passing a script argument. On the other hand, literally anything is supported within a shell script and you could trivially write up that compound command in a script and invoke that as the startup_script with no arguments.

I propose parsing up to and including the label and then saving the rest with another Rich Felker trick:

save () {
for i do printf %s\\n "$i" | sed "s/'/'\\\\''/g;1s/^/'/;\$s/\$/' \\\\/" ; done
echo " "
}

https://www.etalabs.net/sh_tricks.html

It's on my ever-growing todo list. 😁

To support this, makeself would need to change it's invocation signature to:

Usage: ./makeself.sh [params] archive_dir file_name label startup_script_and_args

where startup_script_and_args would be a single string that then gets stored and later passed to sh -c. That breaks compatibility with older scripting that might invoke makeself.

I feel this passes the responsibility of quoting arguments to the caller instead of the tool taking care of it in an opaque way, so it'll make makeself-built scripts harder to use correctly.

That's a neat little bag of tricks there, @realtime-neil.

@nivbend I can see that point of view. On the other hand, it's a fairly standard pattern; and if the command doesn't have any variables in it, it's easy to just single-quote the whole thing and be done with it.

It's the equivalent of an API change, so it's not a thing to change lightly.