strspn() use in SystemCommand()

Questions and postings pertaining to the development of ImageMagick, feature enhancements, and ImageMagick internals. ImageMagick source code and algorithms are discussed here. Usage questions which are too arcane for the normal user list should also be posted here.
Post Reply
agapon
Posts: 10
Joined: 2011-10-13T06:38:44-07:00
Authentication code: 8675308

strspn() use in SystemCommand()

Post by agapon »

In magick/utility.c, in the SystemCommand function I see the following line:

Code: Select all

if ((asynchronous != MagickFalse) || (strspn(shell_command,"&;<>|") == 0)) {
I wonder what was the intention of the strspn call?
Because it looks like it does something that doesn't make much sense - checks that the first symbol of shell_command is not one of &;<>|.
Unless I am confused about what strspn does.
User avatar
magick
Site Admin
Posts: 11064
Joined: 2003-05-31T11:32:55-07:00

Re: strspn() use in SystemCommand()

Post by magick »

Looks like a bug. We'll have a patch within a day or two. Thanks.
agapon
Posts: 10
Joined: 2011-10-13T06:38:44-07:00
Authentication code: 8675308

Re: strspn() use in SystemCommand()

Post by agapon »

Thank you.
BTW, is there a big benefit in trying to use fork+exec for the simpler case as opposed to always using system?
User avatar
anthony
Posts: 8883
Joined: 2004-05-31T19:27:03-07:00
Authentication code: 8675308
Location: Brisbane, Australia

Re: strspn() use in SystemCommand()

Post by anthony »

This is a bit of a side topic, but I'll answer it as it should be a major concern to any programmer (yet isn't)!

fork-exec, or specifically the faster vfork() but only if you are immediately doing an exec() afterwards thus replacing the current program invocation
is faster.

however exec() forms require the arguments to be pre-processed into a array of separate string arguments. You can not give it a single string.

The system() method is much slower as it generally calls on the shell to parse the single command string into seperate arguments and make the appropriate exec() calls. That means you can execute a sequence of commands, and actually run shell loops and other 'fancy' things too.

It also means you have a shell overhead, and extra children processes to content with.

But also then have to content with shell meta-characters (on top of any other meta-characters such as due to double quotes). For example $variable (environment variable) substitutions, escaping quotes in arguments, possible back-quote commands, semi-colons, parenthesis, braces, etc etc etc.. All of which can quickly become a security nightmare if some part of that command line is user provided (say from web page input!)

Without looking, this may be what the code was trying to do. Look for possible meta characters that could be a problem.

I have seen whole chapters in security programming to do with the problem of system(). It is often listed as the number one security failure, even before stack overrun attacks! It is best avoided.

Perl system() command makes this more secure by allowing you to separate the arguments yourself and thus avoid the need for shell parsing.
PHP seems to provide no similar high level function to execute a command without calling the shell! Strange seeing that PHP is a web centric language!

In summery. fork-exec is fast but awkward.
system() is handy, simple but can be security nightmare.
Anthony Thyssen -- Webmaster for ImageMagick Example Pages
https://imagemagick.org/Usage/
Post Reply