labgrid icon indicating copy to clipboard operation
labgrid copied to clipboard

write_file does not properly escape file names

Open fhars opened this issue 1 year ago • 5 comments

Somewhere along the way from write_files through ProcessWrapper, subprocess.Popen, ssh to the final cp command, there seems to be a shell (I suspect ssh) that expands file names, so things fail if filenames contain spaces. Or other interesting things:

$ date; labgrid-client -c test-config/remote.yaml  -p my_place write-files -t '/; touch I_Was_Here' 'fnord'; ls -l ~/I_Was_Here
Mo 6. Mai 13:51:07 CEST 2024
Selected role main from configuration file
-rw-r--r-- 1 hars hars 0  6. Mai 13:51 /home/hars/I_Was_Here

fhars avatar May 06 '24 11:05 fhars

Yep, looks like we are missing a shlex.quote(), we should probably place this in the USBStorageDriver to ensure that passed in folders are properly escaped.

Emantor avatar May 06 '24 12:05 Emantor

Not just the directories:

touch 'fnord; touch I_Was_Here_Too'; labgrid-client -c test-config/remote.yaml  -p mp write-files -t '/; touch I_Was_Here' 'fnord; touch I_Was_Here_Too'; ls -l ~/I_Was*

fhars avatar May 06 '24 12:05 fhars

I meant that both arguments to write_files need to be quoted, regardless of them being files or commands.

Hm, I do wonder who ends up interpreting the commands, AFAICS we always use Popen with shell=False (the default).

Edit: It is indeed the expansion done by SSH.

Emantor avatar May 06 '24 13:05 Emantor

From ssh(1):

If supplied, the arguments will be appended to the command, separated by spaces, before it is sent to the server to be executed.

fhars avatar May 06 '24 13:05 fhars

Yep, please open a PR if you find the time.

Emantor avatar May 06 '24 13:05 Emantor