labgrid-project / labgrid

Embedded systems control library for development, testing and installation
https://labgrid.readthedocs.io/
Other
327 stars 164 forks source link

write_file does not properly escape file names #1392

Open fhars opened 4 months ago

fhars commented 4 months ago

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
Emantor commented 4 months ago

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.

fhars commented 4 months ago

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*
Emantor commented 4 months ago

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.

fhars commented 4 months ago

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.

Emantor commented 4 months ago

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