fjohnson/rotman — Issue #5: General security issues
Author: fjohnson
Created: 2011-11-23 16:33:42+00:00
State: open

I fixed the use of os.system in dicom_rules to call dcm2nii. Instead Popen is now used so there is no risk of shell commands being inserted and run.

Outstanding is still the vulnerability present where Erin's scripts expects a variable input for the directory to cd into. This prefix is set as the subject ID passed in via the sourceAndSubject file to scriptwrapper. This sourceAndSubject file is generated by dicom2analyze which in turn gets the subject ID from the XNAT Downloader. The XNAT Downloader gets the subject id from the catalog parser which derives it from the field in the catalog file.

If the subject_id is invalid then the downloader will fail and the scriptwrapper will never be called. Nevertheless it is something to keep in mind that if an invalid subject_id containing shell commands ever reached the scriptwrapper then Erin's scripts would be called with the prefix variable containing the shell commands.

Lets use 'dticalc' as an example
Lets say I set pfx='/tmp/one /tmp/two'

dticalc will first run

pfx=${1}
if [ ! -d ${pfx} ]; then echo "Error: \"${pfx}\" directory does not exist"; exit 1; fi
cd ${pfx}

but will not exit because the test condition will expand to too many arguments.

later one of the lines is:

rm ${pfx}_rotgrad_table_temp.txt

expanded this will be
/tmp/test/one /tmp/test/two_rotgrad_table_temp.txt

So at this point it should be obvious that any file could potentially be deleted. Realistically though you could probably just do proper quoting around ${pfx} during the directory check like so, so the check works correctly. I don't know enough about bash quoting to guarantee this however. Also, like I said, the pipeline will fail way before scriptwrapper ever gets called if the subject id is invalid.

if [ ! -d "${pfx}" ];

One very serious vulnerability that does exist is the evaluation of the environment variables passed to Erin's scripts.