Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

write_file does not properly escape file names #1392

Open
fhars opened this issue May 6, 2024 · 5 comments · May be fixed by #1450 or #1530
Open

write_file does not properly escape file names #1392

fhars opened this issue May 6, 2024 · 5 comments · May be fixed by #1450 or #1530
Labels
bug

Comments

@fhars
Copy link

fhars commented May 6, 2024

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
Copy link
Member

Emantor commented May 6, 2024

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 Emantor added the bug label May 6, 2024
@fhars
Copy link
Author

fhars commented May 6, 2024

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
Copy link
Member

Emantor commented May 6, 2024 •
edited
Loading

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
Copy link
Author

fhars commented May 6, 2024

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
Copy link
Member

Emantor commented May 6, 2024 •
edited
Loading

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

fhars pushed a commit to fhars/labgrid that referenced this issue Jul 25, 2024
escape file names passed through ssh
ca8929f 
fixes labgrid-project#1392
@fhars fhars linked a pull request Jul 25, 2024 that will close this issue
Open
fhars pushed a commit to fhars/labgrid that referenced this issue Jul 25, 2024
escape file names passed through ssh
05f7760 
fixes labgrid-project#1392

Signed-off-by: Florian Hars <[email protected]>
Emantor added a commit to Emantor/labgrid that referenced this issue Oct 21, 2024
@Emantor
driver/usbstoragedriver: wrap commands
343abb2 
Use the wrap_command() function to escape command arguments in case a remote
operation over SSH is done.

Fixes labgrid-project#1392
Alternative to labgrid-project#1450

Signed-off-by: Rouven Czerwinski <[email protected]>
@Emantor Emantor linked a pull request Oct 21, 2024 that will close this issue
Draft
1 task
Emantor added a commit to Emantor/labgrid that referenced this issue Oct 21, 2024
@Emantor
driver/usbstoragedriver: wrap commands
e3e40b5 
Use the wrap_command() function to escape command arguments in case a remote
operation over SSH is done.

Fixes labgrid-project#1392
Alternative to labgrid-project#1450

Signed-off-by: Rouven Czerwinski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
2 participants
@fhars @Emantor