Skip to content

Safely invoke commands over SSH #18

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

Merged
merged 1 commit into from
Nov 15, 2017
Merged

Safely invoke commands over SSH #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/img/tester.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#~ along with this program. If not, see <http://www.gnu.org/licenses/>.

import os, sys
import pipes
Copy link
Contributor

@larstiq larstiq Oct 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we start caring about Python3 compatibility? I.e:

try:
    from pipes import quote
except ImportError:
    # Moved in Python3.3
    from shlex import quote

Copy link
Contributor Author

@martyone martyone Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to port it as a whole instead (in a single step).

import hashlib
import subprocess as sub
from multiprocessing import Process, TimeoutError
Expand Down Expand Up @@ -194,7 +195,7 @@ def ssh(self, command, user="root", ignore_error=False):
ssh_comm = copy(self.sshbase)
ssh_comm.extend(["-l%s" % user])
ssh_comm.extend(self.sopts)
ssh_comm.extend(command)
ssh_comm.extend([pipes.quote(arg) for arg in command])
try:
self.run(ssh_comm)
except sub.CalledProcessError:
Expand Down Expand Up @@ -588,7 +589,7 @@ def run_tests(self):
else:
print "trying to get any test results"
self.commands.scpfrom("/tmp/results/*", self.results_dir)
self.commands.ssh(['rm', '-rf', '/tmp/results/*'])
self.commands.ssh(['sh', '-c', 'rm -rf /tmp/results/*'])
Copy link
Contributor Author

@martyone martyone Oct 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice how the original arguments differ from the invocation of run() couple of lines above. Now ssh() and run() can be used equally just like popen with list of string.

Copy link
Contributor

@larstiq larstiq Oct 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This confuses me. We end up with

sh -c sh -c "'rm -rf /tmp/results/*'"

do we need a double sh -c to undo a pipes.quote level?

Copy link
Contributor Author

@martyone martyone Oct 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it will be

sh -c "sh -c 'rm -rf /tmp/results/*'"

We need it to get glob patterns resolved or more generally for some shell functionality.

Saying "to undo a pipes.quote level" is basically true but uncovers an implementation detail that the user of ssh() shouldn't care about. The user of ssh() needs to know that ssh() accepts a list of arguments to be directly stuffed into argv and nothing more. The user of ssh() doesn't care about unquotting; the user of ssh() sees the need for some shell functionality, so the command he executes is a shell interpreter with a shell script supplied as an argument.

This is well illistrated with the run() invocations above on lines 586 and 587 which I reffered to in my original comment. First invocation (cp -v) needs some shell functionality so it executes sh -c with the actuall command supplied in form of a shell script. Second invocation (rm -rf) has all arguments ready to be directly stuffed to argv.

Note that ideally the first invocation would have every non-literal input quoted as in

self.commands.run(['sh', '-c', "cp -v /tmp/" + pipes.quote(self.test_id) + "/results/* " + pipes.quote(self.results_dir)])

Thinking of a case when a malicious test_id is supplied by an attacker, e.g. 'foobar; rm -rf /'.

except:
pass

Expand Down
5 changes: 3 additions & 2 deletions src/img/worker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"""MIC2 mic-image-creator wrapper"""

import os
import pipes
import subprocess as sub
from multiprocessing import Process, TimeoutError
import time, datetime
Expand Down Expand Up @@ -250,7 +251,7 @@ def ssh(self, command):
"""
ssh_comm = copy(self.sshbase)
ssh_comm.extend(self.sopts)
ssh_comm.extend(command)
ssh_comm.extend([pipes.quote(arg) for arg in command])
self.run(ssh_comm)

def is_lvm(self, img):
Expand Down Expand Up @@ -351,7 +352,7 @@ def runmic(self, ssh=False, job_args=None):
mic_comm.append('--config=%s' % job_args.ksfile_name)
elif self.ict == "mic":
mic_comm.append('%s' % job_args.image_type)
mic_comm.append('"%s"' % job_args.ksfile_name)
mic_comm.append('%s' % job_args.ksfile_name)
Copy link
Contributor Author

@martyone martyone Oct 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice how mic_comm is passed either to ssh() or run() below, based on some condition. The extra quotes fixes the original ssh() invocation but otoh would break the run() invocation.


mic_comm.append('--arch=%s' % job_args.arch)
mic_comm.append('--outdir=%s' % job_args.outdir)
Expand Down
3 changes: 0 additions & 3 deletions src/img_web/app/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,6 @@ def submit(request):
else:
tokenmap["RELEASEPATTERN"] = ":/%s" % tokenvalue

if " " in tokenvalue:
tokenvalue = '"%s"' % tokenvalue

Copy link
Contributor Author

@martyone martyone Oct 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this would broke the local execution as commented before.

tokenmap[token.name] = tokenvalue

archtoken = jobdata['architecture']
Expand Down