Discussion:
OK, I just remembered what the real point of my recent "popen()" thread was supposed to be.
(too old to reply)
Kenny McCormack
2021-01-23 11:35:24 UTC
Permalink
The idea is that you have a string (say "str") that comes from outside (so
is not trusted) and you want to pass it as an argument to a program, via
popen(). So, you do something like:

sprintf(buffer,"program '%s'",str);
fp = popen(buffer, "r");

and this is peachy keen as long as str doesn't contain a single quote. The
problem is if str contains something like: foo';rm -rf $HOME;echo 'ha! ha!
so you end up with: program 'foo';rm -rf $HOME;echo 'ha! ha!'

So, is it enough to just purge any single quotes from str, before
constructing buffer? (Assuming, as is the case, that single quote should
not ever occur in valid data).
--
He continues to assert that 2 plus 2 equals 4, despite being repeatedly
told otherwise.
Tavis Ormandy
2021-01-23 15:23:48 UTC
Permalink
Post by Kenny McCormack
So, is it enough to just purge any single quotes from str, before
constructing buffer? (Assuming, as is the case, that single quote should
not ever occur in valid data).
No, it might also contain options, like tar's --to-command=foo. There's
also \ that can change the shell parsing. Just removing quotes can also
introduce other problems, and might make some other blacklisted sequence
exist. For example, if you were trying to prevent ".." from occuring,
just removing the quotes from ".'." would introduce it, "program
.'./.'./.'./etc/passwd".

It also depends on the context, if this is a setuid application, using
popen is much much more difficult, because you also have to worry about
the environment.

Tavis.
--
_o) $ lynx lock.cmpxchg8b.com
/\\ _o) _o) $ finger ***@sdf.org
_\_V _( ) _( ) @taviso
Kaz Kylheku
2021-01-23 16:15:52 UTC
Permalink
["Followup-To:" header set to comp.unix.programmer.]
Post by Kenny McCormack
The idea is that you have a string (say "str") that comes from outside (so
is not trusted) and you want to pass it as an argument to a program, via
sprintf(buffer,"program '%s'",str);
fp = popen(buffer, "r");
and this is peachy keen as long as str doesn't contain a single quote. The
problem is if str contains something like: foo';rm -rf $HOME;echo 'ha! ha!
so you end up with: program 'foo';rm -rf $HOME;echo 'ha! ha!'
...
Also, and this is related, is there a version of popen() (or some library
or something available) that is bidirectional - i.e., you can both write
and read from it - for example, you could run the Unix 'sort' utility this
way - send it some data, then read back the sorted result (*).
No. You have to "sandbox" the contents of "str" yourself before passing
it to popen.
Just for clarity, these topics are related, but not in the way you think.
I.e., I wasn't implying that a bidirectional popen() would somehow make it
possible to pass arbitrary strings to popen() and have it magically become
safe.
Rather, my (unstated) point was that if I had a bidirectional popen(),
then I could pass data into the sub-process via stdin, rather than on the
command line. This would, in the context of my actual use case (still as
of yet unstated in this thread), solve the real life use case problem.
If the two approaches are viable alternatives, it means that you in fact
do not have a requirement to allow an untrusted user to execute
arbitrary program syntax that they specify.
Allowing a "canned" (therefore safely chosen by you) command to receive
input solves the problem of otherwise having to pass the input via
parameters (where they are treated as shell syntax).
If that is the situation, it's not too difficult to escape some data so
that can be passed as arguments. Wrap it in single quotes, and replace
every embedded single quote with '\''.
So, is it enough to just purge any single quotes from str, before
constructing buffer? (Assuming, as is the case, that single quote should
not ever occur in valid data).
sprintf(buffer,"program '%s'",str);
Yes, provided you also take care of any possibility that the program
itself can be attacked by the content of the properly escaped input.

If program applies special processing to options, you may need

sprintf(buffer, "program -- %s", single_quote_encoded_str);

and other measures. If program is actually /bin/sh, like in this
strawman example:

sprintf(buffer, "/bin/sh -c %s", single_quote_encoded_str);

then what we are doing is implementing a 100% reliable, properly quoted way for
the user to run an arbitrary command. The above is then equivalent to just:

sprintf(buffer, "%s", unquoted_raw_input);

By the way, if we use sprintf, we have to be sure the buffer is
big enough to hold the worst case input situation:
the longest possible input from the user, consisting of nothing
but apostrophe characters (each of which blows up to four
characers under the '\'' encoding).

Better to use snprintf and bail if truncation occurs.
--
TXR Programming Language: http://nongnu.org/txr
Scott Lurndal
2021-01-23 21:39:44 UTC
Permalink
Post by Kenny McCormack
The idea is that you have a string (say "str") that comes from outside (so
is not trusted) and you want to pass it as an argument to a program, via
sprintf(buffer,"program '%s'",str);
fp = popen(buffer, "r");
and this is peachy keen as long as str doesn't contain a single quote. The
problem is if str contains something like: foo';rm -rf $HOME;echo 'ha! ha!
so you end up with: program 'foo';rm -rf $HOME;echo 'ha! ha!'
So, is it enough to just purge any single quotes from str, before
constructing buffer? (Assuming, as is the case, that single quote should
not ever occur in valid data).
No. It's not enough. You also need to remove grave accents, all
command substitution $(command) and variable substitution ${VARIABLE}
and all file redirection, amongst other things.
Spiros Bousbouras
2021-01-23 21:55:15 UTC
Permalink
On Sat, 23 Jan 2021 21:39:44 GMT
Post by Scott Lurndal
Post by Kenny McCormack
The idea is that you have a string (say "str") that comes from outside (so
is not trusted) and you want to pass it as an argument to a program, via
sprintf(buffer,"program '%s'",str);
fp = popen(buffer, "r");
and this is peachy keen as long as str doesn't contain a single quote. The
problem is if str contains something like: foo';rm -rf $HOME;echo 'ha! ha!
so you end up with: program 'foo';rm -rf $HOME;echo 'ha! ha!'
So, is it enough to just purge any single quotes from str, before
constructing buffer? (Assuming, as is the case, that single quote should
not ever occur in valid data).
No. It's not enough. You also need to remove grave accents, all
command substitution $(command) and variable substitution ${VARIABLE}
and all file redirection, amongst other things.
Why ? popen() will call /bin/sh -c '%s' where %s is what was passed from
the untrusted source. Within single quotes , command substitutions , variable
substitutions and file redirections do not happen. But %s may end in a
backslash so this needs to be taken into account. Overall , it seems easier
to me to call one of the exec* functions directly and arrange the appropriate
redirections instead of doing it through popen() and worry about sanitising
the input.
--
vlaho.ninja/prog
Kenny McCormack
2021-01-24 00:18:51 UTC
Permalink
In article <asamiya-***@bongo-ra.co>,
Spiros Bousbouras <***@gmail.com> wrote:
...
Post by Spiros Bousbouras
Why ? popen() will call /bin/sh -c '%s' where %s is what was passed from
the untrusted source. Within single quotes , command substitutions , variable
substitutions and file redirections do not happen. But %s may end in a
backslash so this needs to be taken into account.
You are of course correct that inside single quotes, none of those other
characters matter. The only thing that's magic is the single quote.

Can you give an example of what bad could happen if there is a trailing
backslash?

Remember: I don't care if bad input generates a garbage result. GIGO as
they say. I just don't want anything evil to happen.
--
People who want to share their religious views with you
almost never want you to share yours with them. -- Dave Barry
David W. Hodgins
2021-01-24 00:46:36 UTC
Permalink
Post by Kenny McCormack
Can you give an example of what bad could happen if there is a trailing
backslash?
The untrusted source could append a command string such as "; rm -rf ~/" (without
the double quotes).
--
Change ***@nomail.afraid.org to ***@teksavvy.com for
email replies.
Ben Bacarisse
2021-01-24 02:52:26 UTC
Permalink
Post by David W. Hodgins
Post by Kenny McCormack
Can you give an example of what bad could happen if there is a
trailing backslash?
The untrusted source could append a command string such as "; rm -rf
~/" (without the double quotes).
The question was about a string from an untrusted source that has a
trailing backslash.
--
Ben.
Kaz Kylheku
2021-01-24 03:26:05 UTC
Permalink
["Followup-To:" header set to comp.unix.programmer.]
Post by Kenny McCormack
...
Post by Spiros Bousbouras
Why ? popen() will call /bin/sh -c '%s' where %s is what was passed from
the untrusted source. Within single quotes , command substitutions , variable
substitutions and file redirections do not happen. But %s may end in a
backslash so this needs to be taken into account.
You are of course correct that inside single quotes, none of those other
characters matter. The only thing that's magic is the single quote.
Can you give an example of what bad could happen if there is a trailing
backslash?
NOthing. If we single-quote-and-escape the user's input, and then
interpolate it in place of %s in the command

program %s

then the shell will pass an argv[1] to program that is byte-for-byte
identical to the original user input (as long as that input does
not contain null bytes).

single-quote-and-escape means that it's wrapped with '...'
and every ' is replaced with '\''

So that means that this passage mechanism is a high fidelity way
of getting the datum into the program.

(The doesn't mean that things are secure; that depends on how that
program interprets that argv[1]. But that is out of scope for
the question, I think. If you had a more ideal popen function that
takes argv[] instead of a shell command, you'd still have *that* issue.)
--
TXR Programming Language: http://nongnu.org/txr
Spiros Bousbouras
2021-01-24 05:17:40 UTC
Permalink
On Sun, 24 Jan 2021 00:18:51 -0000 (UTC)
Post by Kenny McCormack
...
Post by Spiros Bousbouras
Why ? popen() will call /bin/sh -c '%s' where %s is what was passed from
the untrusted source. Within single quotes , command substitutions , variable
substitutions and file redirections do not happen. But %s may end in a
backslash so this needs to be taken into account.
You are of course correct that inside single quotes, none of those other
characters matter. The only thing that's magic is the single quote.
Can you give an example of what bad could happen if there is a trailing
backslash?
On further thought , nothing. I had forgotten that backslash has no special
meaning inside single quotes.
Post by Kenny McCormack
Remember: I don't care if bad input generates a garbage result. GIGO as
they say. I just don't want anything evil to happen.
But I realised that the addition of single quotes can cause problems. If for
example the user enters
echo Hello world

, with your addition of single quotes the shell will see 'echo Hello world'
i.e. it would be as if typing on the command line
'echo Hello world'

and the shell will generate an error message that there is no such command
whereas the user (of your programme) entered a valid command.
--
Correct grammar is classist.
Spiros Bousbouras
2021-01-24 05:47:38 UTC
Permalink
On Sun, 24 Jan 2021 05:17:40 GMT
Post by Spiros Bousbouras
On Sun, 24 Jan 2021 00:18:51 -0000 (UTC)
Post by Kenny McCormack
...
Post by Spiros Bousbouras
Why ? popen() will call /bin/sh -c '%s' where %s is what was passed from
the untrusted source. Within single quotes , command substitutions , variable
substitutions and file redirections do not happen. But %s may end in a
backslash so this needs to be taken into account.
You are of course correct that inside single quotes, none of those other
characters matter. The only thing that's magic is the single quote.
Can you give an example of what bad could happen if there is a trailing
backslash?
On further thought , nothing. I had forgotten that backslash has no special
meaning inside single quotes.
Post by Kenny McCormack
Remember: I don't care if bad input generates a garbage result. GIGO as
they say. I just don't want anything evil to happen.
But I realised that the addition of single quotes can cause problems. If for
example the user enters
echo Hello world
, with your addition of single quotes the shell will see 'echo Hello world'
i.e. it would be as if typing on the command line
'echo Hello world'
and the shell will generate an error message that there is no such command
whereas the user (of your programme) entered a valid command.
Ahh , never mind , I should have had another look at your code before posting
my reply. In the example above the shell will see
program 'echo Hello world'

so that's ok.
Kenny McCormack
2021-01-24 07:59:05 UTC
Permalink
In article <sukeban-***@bongo-ra.co>,
Spiros Bousbouras <***@gmail.com> wrote:
...
Post by Spiros Bousbouras
But I realised that the addition of single quotes can cause problems. If for
example the user enters
echo Hello world
, with your addition of single quotes the shell will see 'echo Hello world'
i.e. it would be as if typing on the command line
'echo Hello world'
and the shell will generate an error message that there is no such command
whereas the user (of your programme) entered a valid command.
But the point is that they are not supposed to be entering commands at all.
They are supposed to be supplying data strings (e.g., "The quick brown fox")
that are interpreted by "program" (see OP for what "program" refers to).

Anyway, sounds like this is now wrapped. Thanks to all.
Post by Spiros Bousbouras
Correct grammar is classist.
Very true.
--
"Everything Roy (aka, AU8YOG) touches turns to crap."
--citizens of alt.obituaries--
Jorgen Grahn
2021-01-24 18:57:39 UTC
Permalink
Post by Spiros Bousbouras
On Sat, 23 Jan 2021 21:39:44 GMT
Post by Scott Lurndal
Post by Kenny McCormack
The idea is that you have a string (say "str") that comes from outside (so
is not trusted) and you want to pass it as an argument to a program, via
sprintf(buffer,"program '%s'",str);
fp = popen(buffer, "r");
and this is peachy keen as long as str doesn't contain a single quote. The
problem is if str contains something like: foo';rm -rf $HOME;echo 'ha! ha!
so you end up with: program 'foo';rm -rf $HOME;echo 'ha! ha!'
So, is it enough to just purge any single quotes from str, before
constructing buffer? (Assuming, as is the case, that single quote should
not ever occur in valid data).
No. It's not enough. You also need to remove grave accents, all
command substitution $(command) and variable substitution ${VARIABLE}
and all file redirection, amongst other things.
Why ? popen() will call /bin/sh -c '%s' where %s is what was passed from
the untrusted source. Within single quotes , command substitutions , variable
substitutions and file redirections do not happen. But %s may end in a
backslash so this needs to be taken into account.
Overall, it seems easier to me to call one of the exec* functions
directly and arrange the appropriate redirections instead of doing
it through popen() and worry about sanitising the input.
That is indeed what I do. Why roundtrip through the shell, if all
features it adds are features that I don't want, and that I have to
defend my data against, using ill-defined strategies?

The only reason is that's the widely available interface.

And that's, as I understand it, the story behind SQL injection
attacks, too.

/Jorgen
--
// Jorgen Grahn <grahn@ Oo o. . .
\X/ snipabacken.se> O o .
Nicolas George
2021-01-24 19:03:13 UTC
Permalink
Jorgen Grahn , dans le message
Post by Jorgen Grahn
That is indeed what I do. Why roundtrip through the shell, if all
features it adds are features that I don't want, and that I have to
defend my data against, using ill-defined strategies?
The only reason is that's the widely available interface.
Using the shell can also be a good idea to allow user-configurable commands.

But in that case, using %s to inject the variable parts of the command is
not a good idea, even with careful escaping. Using $1, $2, etc. or environment
variables is much more robust.
Jorgen Grahn
2021-01-24 21:59:39 UTC
Permalink
Post by Nicolas George
Jorgen Grahn , dans le message
Post by Jorgen Grahn
That is indeed what I do. Why roundtrip through the shell, if all
features it adds are features that I don't want, and that I have to
defend my data against, using ill-defined strategies?
The only reason is that's the widely available interface.
Using the shell can also be a good idea to allow user-configurable commands.
I meant: in the OP's context, where that's something you cannot allow.
I don't want to get rid of popen().

/Jorgen
--
// Jorgen Grahn <grahn@ Oo o. . .
\X/ snipabacken.se> O o .
James K. Lowden
2021-01-25 14:34:41 UTC
Permalink
On 24 Jan 2021 18:57:39 GMT
Post by Jorgen Grahn
The only reason is that's the widely available interface.
And that's, as I understand it, the story behind SQL injection
attacks, too.
SQL injection is universally the product of poor design choices.

Most SQL DBMSs support parameterized queries and stored procedures. If
access permissions to the database are granted only through stored
procedures, and stored procedures are executed only with parameterized
queries, SQL injection is, by definition, impossible. Because the
parameters are passed as data, 'Bobby Tables' become a value in a row.
Even supposing the attacker somehow gained access to the SQL
interface, the webserver's access to the DBMS, being restricted to
stored-procedure calls, is severely restricted in what information he
can access or harm cause.

The shell/exec dichotomy is analogous insofar as exec parameters are
likewise not interpolated into the command.

--jkl
Ben Bacarisse
2021-01-26 12:25:30 UTC
Permalink
Post by James K. Lowden
On 24 Jan 2021 18:57:39 GMT
Post by Jorgen Grahn
The only reason is that's the widely available interface.
And that's, as I understand it, the story behind SQL injection
attacks, too.
SQL injection is universally the product of poor design choices.
https://xkcd.com/327/

... just in case someone has not yet seen it.
--
Ben.
Rainer Weikusat
2021-01-26 23:12:53 UTC
Permalink
Post by Spiros Bousbouras
On Sat, 23 Jan 2021 21:39:44 GMT
Post by Scott Lurndal
Post by Kenny McCormack
The idea is that you have a string (say "str") that comes from outside (so
is not trusted) and you want to pass it as an argument to a program, via
sprintf(buffer,"program '%s'",str);
fp = popen(buffer, "r");
and this is peachy keen as long as str doesn't contain a single quote.
[...]
Post by Spiros Bousbouras
Post by Scott Lurndal
No. It's not enough. You also need to remove grave accents, all
command substitution $(command) and variable substitution ${VARIABLE}
and all file redirection, amongst other things.
Why ? popen() will call /bin/sh -c '%s' where %s is what was passed from
the untrusted source. Within single quotes , command substitutions , variable
substitutions and file redirections do not happen. But %s may end in a
backslash so this needs to be taken into account. Overall , it seems easier
to me to call one of the exec* functions directly and arrange the appropriate
redirections instead of doing it through popen() and worry about sanitising
the input.
Never suggest a simple, correct solution if there's a complicated,
incorrect one involving useless intermediate execution of
/bin/sh. Programmers just love to do that!

:-))
Kenny McCormack
2021-01-27 00:47:07 UTC
Permalink
In article <***@doppelsaurus.mobileactivedefense.com>,
Rainer Weikusat <***@talktalk.net> wrote:
...
Post by Rainer Weikusat
Never suggest a simple, correct solution if there's a complicated,
incorrect one involving useless intermediate execution of
/bin/sh. Programmers just love to do that!
Not to disagree with your basic thrust, but in this case, I think we've
shown pretty conclusively that as long as you handle (one way or the other)
the single quote, you're OK.

And, as I mentioned, in my use case, all I have to do is look for a single
quote and if seen, reject the input.
--
The randomly chosen signature file that would have appeared here is more than 4
lines long. As such, it violates one or more Usenet RFCs. In order to remain
in compliance with said RFCs, the actual sig can be found at the following URL:
http://user.xmission.com/~gazelle/Sigs/ItsTough
Rainer Weikusat
2021-01-27 15:55:46 UTC
Permalink
Post by Kenny McCormack
...
Post by Rainer Weikusat
Never suggest a simple, correct solution if there's a complicated,
incorrect one involving useless intermediate execution of
/bin/sh. Programmers just love to do that!
Not to disagree with your basic thrust, but in this case, I think we've
shown pretty conclusively that as long as you handle (one way or the other)
the single quote, you're OK.
Syntactically, that's correct (AFAICT). OTOH, the basic task is still
runtime code generation for a turing-complete interpreter using data
from untrusted sources and this is going to end badly sooner or later,
cf the somewhat recent "remote root" feature in the OpenBSD mailer: That
started out (AFAIK) with someone writing a general function to run a
shell command because this was needed for a specific use case (shell
commands in aliases/ .forwards files). This function ended up being used
for executing all kinds of mailers regardless of them being simple
program and not something which has to be interpreted by a shell. Hence,
everybody invoking such a mailer had to quote/ validate arguments the
ensure passing them to a shell would be safe. One of the mailers (for
mbox delivery) had to run with elevated privileges. And then, somebody
"improved" the code of the argument quoting function and accidentally
broke it.

This led to all kinds of wild fingerpointing at mbox delivery, the
setuid feature, the C language and the big bad universe in general. But
the original mistake was still "run a shell with untrusted input for no
particular reason" (save forcing everyone to "quote arguments").
Janis Papanagnou
2021-01-27 06:28:16 UTC
Permalink
Post by Rainer Weikusat
Post by Spiros Bousbouras
On Sat, 23 Jan 2021 21:39:44 GMT
Post by Scott Lurndal
Post by Kenny McCormack
The idea is that you have a string (say "str") that comes from outside (so
is not trusted) and you want to pass it as an argument to a program, via
sprintf(buffer,"program '%s'",str);
fp = popen(buffer, "r");
and this is peachy keen as long as str doesn't contain a single quote.
[...]
Post by Spiros Bousbouras
Post by Scott Lurndal
No. It's not enough. You also need to remove grave accents, all
command substitution $(command) and variable substitution ${VARIABLE}
and all file redirection, amongst other things.
Why ? popen() will call /bin/sh -c '%s' where %s is what was passed from
the untrusted source. Within single quotes , command substitutions , variable
substitutions and file redirections do not happen. But %s may end in a
backslash so this needs to be taken into account. Overall , it seems easier
to me to call one of the exec* functions directly and arrange the appropriate
redirections instead of doing it through popen() and worry about sanitising
the input.
Never suggest a simple, correct solution if there's a complicated,
incorrect one involving useless intermediate execution of
/bin/sh. Programmers just love to do that!
:-))
Well, to be fair, the popen() interface is simple, and that is what
makes it appealing to use it (instead of a in comparison complex
exec-based approach with all that you need there). Popen() only gets
complicated if you try to fix the inherent security issues. The OPs
case, though is (WRT security considerations) a simpler special case.

Janis
Rainer Weikusat
2021-01-28 15:11:39 UTC
Permalink
[popen with untrusted arguments]
Post by Janis Papanagnou
Post by Rainer Weikusat
Never suggest a simple, correct solution if there's a complicated,
incorrect one involving useless intermediate execution of
/bin/sh. Programmers just love to do that!
:-))
Well, to be fair, the popen() interface is simple, and that is what
makes it appealing to use it (instead of a in comparison complex
exec-based approach with all that you need there).
I don't think there's anything particularly complex in the code below.

------

#include <stdio.h>
#include <unistd.h>

static FILE *open_ls(char **argv)
{
int fds[2];

pipe(fds);
if (fork() == 0) {
close(*fds);
dup2(fds[1], 1);

*argv = "ls";
execvp("ls", argv);
}

close(fds[1]);
return fdopen(*fds, "r");
}

int main(int argc, char **argv)
{
char buf[4096];
FILE *fp;

fp = open_ls(argv);
while (fgets(buf, sizeof(buf), fp)) fputs(buf, stdout);
return 0;
}
Janis Papanagnou
2021-01-28 15:27:31 UTC
Permalink
Post by Rainer Weikusat
[popen with untrusted arguments]
Post by Janis Papanagnou
Post by Rainer Weikusat
Never suggest a simple, correct solution if there's a complicated,
incorrect one involving useless intermediate execution of
/bin/sh. Programmers just love to do that!
:-))
Well, to be fair, the popen() interface is simple, and that is what
makes it appealing to use it (instead of a in comparison complex
exec-based approach with all that you need there).
I don't think there's anything particularly complex in the code below.
I didn't say it's "particularly complex", I said that popen() is simple
(and implying its use is [much] simpler that a fork/exec/dup2 etc.) and
thus appealing. - Don't you agree?

Janis
Post by Rainer Weikusat
------
#include <stdio.h>
#include <unistd.h>
static FILE *open_ls(char **argv)
{
int fds[2];
pipe(fds);
if (fork() == 0) {
close(*fds);
dup2(fds[1], 1);
*argv = "ls";
execvp("ls", argv);
}
close(fds[1]);
return fdopen(*fds, "r");
}
int main(int argc, char **argv)
{
char buf[4096];
FILE *fp;
fp = open_ls(argv);
while (fgets(buf, sizeof(buf), fp)) fputs(buf, stdout);
return 0;
}
Rainer Weikusat
2021-01-28 16:36:39 UTC
Permalink
Post by Janis Papanagnou
Post by Rainer Weikusat
[popen with untrusted arguments]
Post by Janis Papanagnou
Post by Rainer Weikusat
Never suggest a simple, correct solution if there's a complicated,
incorrect one involving useless intermediate execution of
/bin/sh. Programmers just love to do that!
:-))
Well, to be fair, the popen() interface is simple, and that is what
makes it appealing to use it (instead of a in comparison complex
exec-based approach with all that you need there).
I don't think there's anything particularly complex in the code below.
I didn't say it's "particularly complex", I said that popen() is simple
(and implying its use is [much] simpler that a fork/exec/dup2 etc.) and
thus appealing. - Don't you agree?
No. Something which enables "saving" <20 lines of code is not worth any
effort of supporting it, especially not when it's murkily designed[*] and
has a history of causing problems.

[*] There's a so-called "least privilege principle" supposed to apply to
security/ safety critical code. Invoking the shell just to execute
another program is the exact opposite of that: Everything and two
kitchen sinks just to make sure it can do anything someone could
conceivably ever need.

[code example in original post]
Janis Papanagnou
2021-01-29 12:42:20 UTC
Permalink
Post by Rainer Weikusat
Post by Janis Papanagnou
Post by Rainer Weikusat
[popen with untrusted arguments]
Post by Janis Papanagnou
Post by Rainer Weikusat
Never suggest a simple, correct solution if there's a complicated,
incorrect one involving useless intermediate execution of
/bin/sh. Programmers just love to do that!
Well, to be fair, the popen() interface is simple, and that is what
makes it appealing to use it (instead of a in comparison complex
exec-based approach with all that you need there).
I don't think there's anything particularly complex in the code below.
I didn't say it's "particularly complex", I said that popen() is simple
(and implying its use is [much] simpler that a fork/exec/dup2 etc.) and
thus appealing. - Don't you agree?
No. Something which enables "saving" <20 lines of code is not worth any
effort of supporting it, especially not when it's murkily designed[*] and
has a history of causing problems.
It's not (not primary) a question of "saving lines"; it's also about
avoiding complexity. More code and higher complexity makes software
more error prone. Of course it is worth to support short solutions,
libraries and languages with a higher abstraction level (to produce
shorter code).

Support of simple to use and formulate solutions is a primary concern
of IT; that's why folks prefer, for example, an Awk one-liner instead
of a C/C++ 25-liner. That's what I expressed when I spoke of an
"appealing" use. It always depends on the actual requirements.

The "history of causing problems" is likely to be interpreted as the
already mentioned security issues, depending on how you use it. In my
post upthread I addressed this already, but you decided to not quote
Post by Rainer Weikusat
Post by Janis Papanagnou
Post by Rainer Weikusat
Post by Janis Papanagnou
Popen() only gets complicated if you try to fix the inherent
security issues.
For the context of this thread, the OP has a well defined special
case of an application and requirements, and I don't seem to recall
any post that proved his use of popen() to have security issues.

Janis
Post by Rainer Weikusat
[*] There's a so-called "least privilege principle" supposed to apply to
security/ safety critical code. Invoking the shell just to execute
another program is the exact opposite of that: Everything and two
kitchen sinks just to make sure it can do anything someone could
conceivably ever need.
[code example in original post]
Rainer Weikusat
2021-01-29 15:26:16 UTC
Permalink
Post by Janis Papanagnou
Post by Rainer Weikusat
Post by Janis Papanagnou
Post by Rainer Weikusat
[popen with untrusted arguments]
Post by Janis Papanagnou
Post by Rainer Weikusat
Never suggest a simple, correct solution if there's a complicated,
incorrect one involving useless intermediate execution of
/bin/sh. Programmers just love to do that!
Well, to be fair, the popen() interface is simple, and that is what
makes it appealing to use it (instead of a in comparison complex
exec-based approach with all that you need there).
I don't think there's anything particularly complex in the code below.
I didn't say it's "particularly complex", I said that popen() is simple
(and implying its use is [much] simpler that a fork/exec/dup2 etc.) and
thus appealing. - Don't you agree?
No. Something which enables "saving" <20 lines of code is not worth any
effort of supporting it, especially not when it's murkily designed[*] and
has a history of causing problems.
It's not (not primary) a question of "saving lines"; it's also about
avoiding complexity. More code and higher complexity makes software
more error prone. Of course it is worth to support short solutions,
libraries and languages with a higher abstraction level (to produce
shorter code).
There is no "complexitiy" in 20 lines of code, most of which are doing
syscalls. OTOH, there's a hell lot of complexity in the shell. And this
is known to cause real problems, not just hypothetical ones ("prone to
errors").

Even the "higher abstraction" on its own already adds
complexity. Leaving issue with multiple threads alone, a general
solution would also need to ensure that SIGINT occuring while the other
programs runs doesn't affect the parent process but only the child,
taking care to avoid disrupting whatever signal handling the program
calling the library function might be using. Further, it would also
need to stop a SIGCHLD handler which might exist from eating up the exit
status of the invoked program, again without disrupting whatever signal
handling might be in place.

All these fairly difficult to address technical problems ("complexity")
don't magically vanish just because someone slaps a name like "popen" on
whatever the implementation happens to be. There's no special magic in
code written by someone else: It's just as likely to be buggy.

[...]
Post by Janis Papanagnou
The "history of causing problems" is likely to be interpreted as the
already mentioned security issues, depending on how you use it. In my
post upthread I addressed this already, but you decided to not quote
Post by Rainer Weikusat
Post by Janis Papanagnou
Post by Rainer Weikusat
Post by Janis Papanagnou
Popen() only gets complicated if you try to fix the inherent
security issues.
For the context of this thread, the OP has a well defined special
case of an application and requirements, and I don't seem to recall
any post that proved his use of popen() to have security issues.
popen is inherently much more complicated than anything involving pipe,
dup2, fork and exec addressing a specific problem will be. That it's
additionally very difficult to use the moment one starts to need any of
the "advance" features it's supposed to provide is not a point in favour
of it.

Helmut Waitzmann
2021-01-24 00:06:54 UTC
Permalink
Post by Kenny McCormack
The idea is that you have a string (say "str") that comes from
outside (so is not trusted) and you want to pass it as an argument
sprintf(buffer,"program '%s'",str);
fp = popen(buffer, "r");
and this is peachy keen as long as str doesn't contain a single
foo';rm -rf $HOME;echo 'ha! ha!
program 'foo';rm -rf $HOME;echo 'ha! ha!'
So, is it enough to just purge any single quotes from str, before
constructing buffer? (Assuming, as is the case, that single quote
should not ever occur in valid data).
It's not enough, it's the wrong direction.  Single quotes should be
added to str rather than purged from: 

First, after each single quote in str insert \'' (i. e. a backslash
and two single quotes).  Then put a single quote before and after
str (as you already showed above in the sprintf format string) when
inserting it into the command line. 

For example,


foo';rm -rf $HOME;echo 'ha! ha!

will end up in


program 'foo'\'';rm -rf $HOME;echo '\''ha! ha!'
Kenny McCormack
2021-01-24 10:16:43 UTC
Permalink
In article <***@helmutwaitzmann.news.arcor.de>,
Helmut Waitzmann <***@xoxy.net> wrote:
...
Post by Kenny McCormack
So, is it enough to just purge any single quotes from str, before
constructing buffer? (Assuming, as is the case, that single quote
should not ever occur in valid data).
It's not enough, it's the wrong direction. Single quotes should be
That may be correct advice in the abstract, but it doesn't apply in my use
case, since, as mentioned in the OP, single quote is never part of valid
input data.

In fact, now that I think about it, best might be to not even bother
purging them, but rather, just check to see if the string contains any
single quote(s), and immediately reject if so. I.e., not even run popen()
at all in that case.
--
If you think you have any objections to anything I've said above, please
navigate to this URL:
http://www.xmission.com/~gazelle/Truth
This should clear up any misconceptions you may have.
Loading...