Skip to content

Conversation

@mfakbar127
Copy link

Using escapeshellarg() instead escape hard filter.

Here is the test.

screenshot from 2019-01-17 07-14-40

<?php

function escape($string,$addQuotes = true)
{
        $string = str_replace(
            ['"', '`', '', '\\\''],
            ['\"', "'", "'", "'"],
            trim($string)
        );
        return $addQuotes ? '"'.$string.'"' : $string;
}

function escape_safe($string,$addQuotes = false)
{
 		$string = escapeshellarg($string);
        return $addQuotes ? '"'.$string.'"' : $string;
}

$cmd = '$(uname -a)';

$escape_cmd = escape($cmd);
$command = "/usr/bin/convert logo.png -resize " . $escape_cmd . " result.png";
echo $command . PHP_EOL;
echo exec($command);

echo "\n\n";

$escape_cmd = escape_safe($cmd);
$command = "/usr/bin/convert logo.png -resize " . $escape_cmd . " result.png";
echo $command . PHP_EOL;
echo exec($command);

@Pierstoval
Copy link
Member

Fine for me, can you fix the tests?

@mfakbar127
Copy link
Author

I dont understand, can you explain it ?

@Pierstoval
Copy link
Member

The PHPUnit test suite is failing, this is due to the fact that some tests must be updated according to the change you made.

Run composer install on the project and then execute vendor/bin/phpunit to see what exact tests are failing 🙂

@mfakbar127
Copy link
Author

In fact i dont install the project.
I just read source code of command.php, then i saw there is unsecure escape method in command class.
I think better to use escapeshellarg instead hard code escape.

@Pierstoval
Copy link
Member

Yeah, you're right, but this breaks the phpunit tests...

Nevermind, I'll find some time soon and fix this myself..

@mfakbar127
Copy link
Author

mfakbar127 commented Jan 17, 2019

i already fixed phpunit test case issuein my latest commit.
hope you can fix your imagemagick compile issue

@Orbitale Orbitale deleted a comment from coveralls Jan 17, 2019
@Orbitale Orbitale deleted a comment from coveralls Jan 17, 2019
@Pierstoval Pierstoval changed the base branch from master to shell-escape January 22, 2019 08:55
@Pierstoval Pierstoval merged commit 4b884e6 into Orbitale:shell-escape Jan 22, 2019
@Pierstoval
Copy link
Member

I will take this PR and update it, thanks @rhamaa

@Pierstoval Pierstoval mentioned this pull request Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants