search

justinhunt / moodle-filter_poodll

The PoodLL Filter
6 stars 17 forks source link

Validate / normalize system arguments in convert_with_ffmpeg() #33

Closed tmuras closed 4 years ago

tmuras commented 4 years ago

There is a call to shell_exec() function convert_with_ffmpeg : https://github.com/justinhunt/moodle-filter_poodll/blob/poodll3/classes/poodlltools.php#L1295

The string that composes the command executed is constructed from variables from different sources. We should add normalization / validation of each of them to prevent any injections.

justinhunt commented 4 years ago

Thanks for checking it, Tomasz.

Currrently it is:

$command = $ffmpegpath . " -i " . $tempdir . $tempfilename . " " . $ffmpegopts . " " . $tempdir . $convfilename;

How do you think about this ..?

$command = $ffmpegpath . " -i " . escapeshellarg($tempdir . $tempfilename) . " " . $ffmpegopts . " " . escapeshellarg($tempdir . $convfilename);

In this we have not checked $ffmpegopts. But this is pulled straight from the config setting that the Moodle admin has set. I think we need to trust that input, though we might put a warning in the UI.

tmuras commented 4 years ago

Hi - thanks for a quick reply.

How about doing a basic check for $ffmpegopts options - we could check if the following characters are in the string and give an error if any is found: &, |, ;, `, $ or newline I don't think any of the above would be put into options for ffmpeg.

Or I think we can use escapeshellcmd($ffmpegopts).

justinhunt commented 4 years ago

Thanks for this. It should be resolved with : https://github.com/justinhunt/moodle-filter_poodll/commit/1cce07dcb11a043d84b93fa42c1ebc042c4e8dcf