From 7f2ca33774a55361ae69b0b5d696ebeb3a92c58f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 21 Feb 2017 22:49:35 +0100 Subject: [PATCH 01/14] tests: fix importing template in non-default pool --- qubes/tests/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qubes/tests/__init__.py b/qubes/tests/__init__.py index 6705bd4b..f68f3bac 100644 --- a/qubes/tests/__init__.py +++ b/qubes/tests/__init__.py @@ -523,7 +523,8 @@ class SystemTestsMixin(object): label='black') for name, volume in template_vm.volumes.items(): if volume.pool != template.volumes[name].pool: - template_vm.storage.init_volume(name, volume.config) + template_vm.storage.init_volume(name, + template.volumes[name].config) self.app.default_template = template_vm def init_networking(self): From 3ecc0a9bcbe11ef042f292317f062427ffce5b22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 23 Feb 2017 00:18:54 +0100 Subject: [PATCH 02/14] tests: improve devices API unit test Check fired events - inspired by qvm-device test. --- qubes/tests/devices.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/qubes/tests/devices.py b/qubes/tests/devices.py index e2e8448f..1136199b 100644 --- a/qubes/tests/devices.py +++ b/qubes/tests/devices.py @@ -111,6 +111,7 @@ class TC_00_DeviceCollection(qubes.tests.QubesTestCase): def test_013_list_attached_persistent(self): self.assertEqual(set([]), set(self.collection.attached())) + self.assertEventFired(self.emitter, 'device-list-attached:testclass') self.collection.attach(self.device) self.assertEqual({self.device}, set(self.collection.attached())) self.assertEqual({self.device}, @@ -128,9 +129,11 @@ class TC_00_DeviceCollection(qubes.tests.QubesTestCase): set(self.collection.attached(persistent=True))) self.assertEqual({self.device}, set(self.collection.attached(persistent=False))) + self.assertEventFired(self.emitter, 'device-list-attached:testclass') def test_015_list_available(self): self.assertEqual({self.device}, set(self.collection)) + self.assertEventFired(self.emitter, 'device-list:testclass') class TC_01_DeviceManager(qubes.tests.QubesTestCase): From 13fc8103630a6150502d588992db7bc2226f0c8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 23 Feb 2017 00:30:35 +0100 Subject: [PATCH 03/14] tests: some more fixes for core3 API --- qubes/tests/integ/vm_qrexec_gui.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/qubes/tests/integ/vm_qrexec_gui.py b/qubes/tests/integ/vm_qrexec_gui.py index 573b4cb8..ce336292 100644 --- a/qubes/tests/integ/vm_qrexec_gui.py +++ b/qubes/tests/integ/vm_qrexec_gui.py @@ -970,6 +970,7 @@ class TC_00_AppVMMixin(qubes.tests.SystemTestsMixin): # now free the fragmented memory and trigger compaction alloc1.stdin.write(b"\n") + alloc1.stdin.flush() alloc1.wait() self.testvm1.run("echo 1 > /proc/sys/vm/compact_memory", user="root") @@ -1006,9 +1007,11 @@ class TC_00_AppVMMixin(qubes.tests.SystemTestsMixin): class TC_10_Generic(qubes.tests.SystemTestsMixin, qubes.tests.QubesTestCase): def setUp(self): super(TC_10_Generic, self).setUp() + self.init_default_template() self.vm = self.app.add_new_vm( qubes.vm.appvm.AppVM, name=self.make_vm_name('vm'), + label='red', template=self.app.default_template) self.vm.create_on_disk() self.save_and_reload_db() @@ -1029,7 +1032,7 @@ class TC_10_Generic(qubes.tests.SystemTestsMixin, qubes.tests.QubesTestCase): f.write('echo service output\n') self.addCleanup(os.unlink, "/etc/qubes-rpc/test.AnyvmDeny") - self.vm.start(verbose=False) + self.vm.start() p = self.vm.run("/usr/lib/qubes/qrexec-client-vm dom0 test.AnyvmDeny", passio_popen=True, passio_stderr=True) (stdout, stderr) = p.communicate() From 1363251438718a3c11f3a2f2470ae3e7efc3f05b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 23 Feb 2017 00:45:58 +0100 Subject: [PATCH 04/14] Revert "Revert "backup: use 'scrypt' tool for backup encryption and integrity protection"" This reverts commit 0f1672dc6393e61649d096debc1405d22ed6fc65. Bring it back. Lets not revert the whole feature just because required package exists only in qubes-builder, not in some online repository. Also, this revert didn't go as planned - there was a reference to a 'passphrase' local variable, but it wasn't assigned any value. Cc: @woju --- qubes/backup.py | 414 ++++++++++++++++++++++++++-------------- rpm_spec/core-dom0.spec | 1 + 2 files changed, 269 insertions(+), 146 deletions(-) diff --git a/qubes/backup.py b/qubes/backup.py index a4c79ea6..e5d5619d 100644 --- a/qubes/backup.py +++ b/qubes/backup.py @@ -22,8 +22,8 @@ from __future__ import unicode_literals import itertools import logging - import functools +import termios from qubes.utils import size_to_human import sys @@ -51,7 +51,9 @@ QUEUE_FINISHED = "FINISHED" HEADER_FILENAME = 'backup-header' DEFAULT_CRYPTO_ALGORITHM = 'aes-256-cbc' -DEFAULT_HMAC_ALGORITHM = 'SHA512' +# 'scrypt' is not exactly HMAC algorithm, but a tool we use to +# integrity-protect the data +DEFAULT_HMAC_ALGORITHM = 'scrypt' DEFAULT_COMPRESSION_FILTER = 'gzip' CURRENT_BACKUP_FORMAT_VERSION = '4' # Maximum size of error message get from process stderr (including VM process) @@ -219,6 +221,63 @@ class SendWorker(Process): self.log.debug("Finished sending thread") +def launch_proc_with_pty(args, stdin=None, stdout=None, stderr=None, echo=True): + """Similar to pty.fork, but handle stdin/stdout according to parameters + instead of connecting to the pty + + :return tuple (subprocess.Popen, pty_master) + """ + + def set_ctty(ctty_fd, master_fd): + os.setsid() + os.close(master_fd) + fcntl.ioctl(ctty_fd, termios.TIOCSCTTY, 0) + if not echo: + termios_p = termios.tcgetattr(ctty_fd) + # termios_p.c_lflags + termios_p[3] &= ~termios.ECHO + termios.tcsetattr(ctty_fd, termios.TCSANOW, termios_p) + (pty_master, pty_slave) = os.openpty() + p = subprocess.Popen(args, stdin=stdin, stdout=stdout, stderr=stderr, + preexec_fn=lambda: set_ctty(pty_slave, pty_master)) + os.close(pty_slave) + return p, os.fdopen(pty_master, 'w+') + + +def launch_scrypt(action, input_name, output_name, passphrase): + ''' + Launch 'scrypt' process, pass passphrase to it and return + subprocess.Popen object. + + :param action: 'enc' or 'dec' + :param input_name: input path or '-' for stdin + :param output_name: output path or '-' for stdout + :param passphrase: passphrase + :return: subprocess.Popen object + ''' + command_line = ['scrypt', action, input_name, output_name] + (p, pty) = launch_proc_with_pty(command_line, + stdin=subprocess.PIPE if input_name == '-' else None, + stdout=subprocess.PIPE if output_name == '-' else None, + stderr=subprocess.PIPE, + echo=False) + if action == 'enc': + prompts = ('Please enter passphrase: ', 'Please confirm passphrase: ') + else: + prompts = ('Please enter passphrase: ',) + for prompt in prompts: + actual_prompt = p.stderr.read(len(prompt)) + if actual_prompt != prompt: + raise qubes.exc.QubesException( + 'Unexpected prompt from scrypt: {}'.format(actual_prompt)) + pty.write(passphrase.encode('utf-8') + b'\n') + pty.flush() + # save it here, so garbage collector would not close it (which would kill + # the child) + p.pty = pty + return p + + class Backup(object): class FileToBackup(object): def __init__(self, file_path, subdir=None, name=None): @@ -489,15 +548,18 @@ class Backup(object): backup_id=self.backup_id, ) backup_header.save(header_file_path) + # Start encrypt, scrypt will also handle integrity + # protection + scrypt_passphrase = u'{filename}!{passphrase}'.format( + filename=HEADER_FILENAME, passphrase=self.passphrase) + scrypt = launch_scrypt( + 'enc', header_file_path, header_file_path + '.hmac', + scrypt_passphrase) - hmac = subprocess.Popen( - ["openssl", "dgst", "-" + self.hmac_algorithm, - "-hmac", self.passphrase], - stdin=open(header_file_path, "r"), - stdout=open(header_file_path + ".hmac", "w")) - if hmac.wait() != 0: + if scrypt.wait() != 0: raise qubes.exc.QubesException( - "Failed to compute hmac of header file") + "Failed to compute hmac of header file: " + + scrypt.stderr.read()) return HEADER_FILENAME, HEADER_FILENAME + ".hmac" @@ -651,60 +713,43 @@ class Backup(object): self.log.debug(" ".join(tar_cmdline)) - # Tips: Popen(bufsize=0) - # Pipe: tar-sparse | encryptor [| hmac] | tar | backup_target - # Pipe: tar-sparse [| hmac] | tar | backup_target + # Pipe: tar-sparse | scrypt | tar | backup_target # TODO: log handle stderr tar_sparse = subprocess.Popen( - tar_cmdline, stdin=subprocess.PIPE) + tar_cmdline) self.processes_to_kill_on_cancel.append(tar_sparse) # Wait for compressor (tar) process to finish or for any # error of other subprocesses i = 0 + pipe = open(backup_pipe, 'rb') run_error = "paused" - encryptor = None - if self.encrypted: - # Start encrypt - # If no cipher is provided, - # the data is forwarded unencrypted !!! - encryptor = subprocess.Popen([ - "openssl", "enc", - "-e", "-" + self.crypto_algorithm, - "-pass", "pass:" + passphrase], - stdin=open(backup_pipe, 'rb'), - stdout=subprocess.PIPE) - pipe = encryptor.stdout - else: - pipe = open(backup_pipe, 'rb') while run_error == "paused": - - # Start HMAC - hmac = subprocess.Popen([ - "openssl", "dgst", "-" + self.hmac_algorithm, - "-hmac", passphrase], - stdin=subprocess.PIPE, - stdout=subprocess.PIPE) - # Prepare a first chunk - chunkfile = backup_tempfile + "." + "%03d" % i + chunkfile = backup_tempfile + ".%03d.enc" % i i += 1 - chunkfile_p = open(chunkfile, 'wb') + + # Start encrypt, scrypt will also handle integrity + # protection + scrypt_passphrase = \ + u'{backup_id}!{filename}!{passphrase}'.format( + backup_id=self.backup_id, + filename=os.path.relpath(chunkfile[:-4], + self.tmpdir), + passphrase=self.passphrase) + scrypt = launch_scrypt( + "enc", "-", chunkfile, scrypt_passphrase) run_error = handle_streams( pipe, - {'hmac_data': hmac.stdin, - 'backup_target': chunkfile_p, - }, - {'hmac': hmac, - 'vmproc': vmproc, + {'backup_target': scrypt.stdin}, + {'vmproc': vmproc, 'addproc': tar_sparse, - 'streamproc': encryptor, + 'scrypt': scrypt, }, self.chunk_size, self._add_vm_progress ) - chunkfile_p.close() self.log.debug( "12 returned: {}".format(run_error)) @@ -714,12 +759,7 @@ class Backup(object): tar_sparse.terminate() except OSError: pass - try: - hmac.terminate() - except OSError: - pass tar_sparse.wait() - hmac.wait() to_send.put(QUEUE_ERROR) send_proc.join() shutil.rmtree(self.tmpdir) @@ -735,29 +775,16 @@ class Backup(object): "Failed to perform backup: error in " + run_error) + scrypt.stdin.close() + scrypt.wait() + self.log.debug("scrypt return code: {}".format( + scrypt.poll())) + # Send the chunk to the backup target self._queue_put_with_check( send_proc, vmproc, to_send, os.path.relpath(chunkfile, self.tmpdir)) - # Close HMAC - hmac.stdin.close() - hmac.wait() - self.log.debug("HMAC proc return code: {}".format( - hmac.poll())) - - # Write HMAC data next to the chunk file - hmac_data = hmac.stdout.read() - self.log.debug( - "Writing hmac to {}.hmac".format(chunkfile)) - with open(chunkfile + ".hmac", 'w') as hmac_file: - hmac_file.write(hmac_data) - - # Send the HMAC to the backup target - self._queue_put_with_check( - send_proc, vmproc, to_send, - os.path.relpath(chunkfile, self.tmpdir) + ".hmac") - if tar_sparse.poll() is None or run_error == "size_limit": run_error = "paused" else: @@ -1333,6 +1360,8 @@ def get_supported_hmac_algo(hmac_algorithm=None): # Start with provided default if hmac_algorithm: yield hmac_algorithm + if hmac_algorithm != 'scrypt': + yield 'scrypt' proc = subprocess.Popen(['openssl', 'list-message-digest-algorithms'], stdout=subprocess.PIPE) for algo in proc.stdout.readlines(): @@ -1552,6 +1581,10 @@ class BackupRestore(object): def _verify_hmac(self, filename, hmacfile, algorithm=None): def load_hmac(hmac_text): + if filter(lambda x: ord(x) not in range(128), + hmac_text): + raise qubes.exc.QubesException( + "Invalid content of {}".format(hmacfile)) hmac_text = hmac_text.strip().split("=") if len(hmac_text) > 1: hmac_text = hmac_text[1].strip() @@ -1570,6 +1603,17 @@ class BackupRestore(object): "ERROR: expected hmac for {}, but got {}". format(filename, hmacfile)) + if algorithm == 'scrypt': + # in case of 'scrypt' _verify_hmac is only used for backup header + assert filename == HEADER_FILENAME + self._verify_and_decrypt(hmacfile, HEADER_FILENAME + '.dec') + if open(os.path.join(self.tmpdir, filename)).read() != \ + open(os.path.join(self.tmpdir, filename + '.dec')).read(): + raise qubes.exc.QubesException( + 'Invalid hmac on {}'.format(filename)) + else: + return True + hmac_proc = subprocess.Popen( ["openssl", "dgst", "-" + algorithm, "-hmac", passphrase], stdin=open(os.path.join(self.tmpdir, filename), 'rb'), @@ -1595,6 +1639,80 @@ class BackupRestore(object): "Is the passphrase correct?". format(filename, load_hmac(hmac_stdout.decode('ascii')))) + def _verify_and_decrypt(self, filename, output=None): + assert filename.endswith('.enc') or filename.endswith('.hmac') + fullname = os.path.join(self.tmpdir, filename) + (origname, _) = os.path.splitext(filename) + if output: + fulloutput = os.path.join(self.tmpdir, output) + else: + fulloutput = os.path.join(self.tmpdir, origname) + if origname == HEADER_FILENAME: + passphrase = u'{filename}!{passphrase}'.format( + filename=origname, + passphrase=self.passphrase) + else: + passphrase = u'{backup_id}!{filename}!{passphrase}'.format( + backup_id=self.header_data.backup_id, + filename=origname, + passphrase=self.passphrase) + p = launch_scrypt('dec', fullname, fulloutput, passphrase) + (_, stderr) = p.communicate() + if p.returncode != 0: + os.unlink(fulloutput) + raise qubes.exc.QubesException('failed to decrypt {}: {}'.format( + fullname, stderr)) + # encrypted file is no longer needed + os.unlink(fullname) + return origname + + def _retrieve_backup_header_files(self, files, allow_none=False): + (retrieve_proc, filelist_pipe, error_pipe) = \ + self._start_retrieval_process( + files, len(files), 1024 * 1024) + filelist = filelist_pipe.read() + retrieve_proc_returncode = retrieve_proc.wait() + if retrieve_proc in self.processes_to_kill_on_cancel: + self.processes_to_kill_on_cancel.remove(retrieve_proc) + extract_stderr = error_pipe.read(MAX_STDERR_BYTES) + + # wait for other processes (if any) + for proc in self.processes_to_kill_on_cancel: + if proc.wait() != 0: + raise qubes.exc.QubesException( + "Backup header retrieval failed (exit code {})".format( + proc.wait()) + ) + + if retrieve_proc_returncode != 0: + if not filelist and 'Not found in archive' in extract_stderr: + if allow_none: + return None + else: + raise qubes.exc.QubesException( + "unable to read the qubes backup file {0} ({1}): {2}".format( + self.backup_location, + retrieve_proc.wait(), + extract_stderr + )) + actual_files = filelist.splitlines() + if sorted(actual_files) != sorted(files): + raise qubes.exc.QubesException( + 'unexpected files in archive: got {!r}, expeced {!r}'.format( + actual_files, files + )) + for f in files: + if not os.path.exists(os.path.join(self.tmpdir, f)): + if allow_none: + return None + else: + raise qubes.exc.QubesException( + 'Unable to retrieve file {} from backup {}: {}'.format( + f, self.backup_location, extract_stderr + ) + ) + return files + def _retrieve_backup_header(self): """Retrieve backup header and qubes.xml. Only backup header is analyzed, qubes.xml is left as-is @@ -1611,82 +1729,47 @@ class BackupRestore(object): header_data.version = 1 return header_data - (retrieve_proc, filelist_pipe, error_pipe) = \ - self._start_retrieval_process( - ['backup-header', 'backup-header.hmac', - 'qubes.xml.000', 'qubes.xml.000.hmac'], 4, 1024 * 1024) + header_files = self._retrieve_backup_header_files( + ['backup-header', 'backup-header.hmac'], allow_none=True) - expect_tar_error = False - - filename = filelist_pipe.readline().strip().decode('ascii') - hmacfile = filelist_pipe.readline().strip().decode('ascii') - # tar output filename before actually extracting it, so wait for the - # next one before trying to access it - if not self.backup_vm: - filelist_pipe.readline().strip() - - self.log.debug("Got backup header and hmac: {}, {}".format( - filename, hmacfile)) - - if not filename or filename == "EOF" or \ - not hmacfile or hmacfile == "EOF": - retrieve_proc.wait() - proc_error_msg = error_pipe.read(MAX_STDERR_BYTES) - raise qubes.exc.QubesException( - "Premature end of archive while receiving " - "backup header. Process output:\n" + proc_error_msg) - file_ok = False - hmac_algorithm = DEFAULT_HMAC_ALGORITHM - for hmac_algo in get_supported_hmac_algo(hmac_algorithm): - try: - if self._verify_hmac(filename, hmacfile, hmac_algo): - file_ok = True - hmac_algorithm = hmac_algo - break - except qubes.exc.QubesException: - # Ignore exception here, try the next algo - pass - if not file_ok: - raise qubes.exc.QubesException( - "Corrupted backup header (hmac verification " - "failed). Is the password correct?") - if os.path.basename(filename) == HEADER_FILENAME: - filename = os.path.join(self.tmpdir, filename) - header_data = BackupHeader(open(filename, 'rb').read()) - os.unlink(filename) - else: - # if no header found, create one with guessed HMAC algo + if not header_files: + # R2-Beta3 didn't have backup header, so if none is found, + # assume it's version=2 and use values present at that time header_data = BackupHeader( version=2, - hmac_algorithm=hmac_algorithm, # place explicitly this value, because it is what format_version # 2 have + hmac_algorithm='SHA1', crypto_algorithm='aes-256-cbc', # TODO: set encrypted to something... ) - # when tar do not find expected file in archive, it exit with - # code 2. This will happen because we've requested backup-header - # file, but the archive do not contain it. Ignore this particular - # error. - if not self.backup_vm: - expect_tar_error = True + else: + filename = HEADER_FILENAME + hmacfile = HEADER_FILENAME + '.hmac' + self.log.debug("Got backup header and hmac: {}, {}".format( + filename, hmacfile)) - if retrieve_proc.wait() != 0 and not expect_tar_error: - raise qubes.exc.QubesException( - "unable to read the qubes backup file {0} ({1}): {2}".format( - self.backup_location, - retrieve_proc.wait(), - error_pipe.read(MAX_STDERR_BYTES) - )) - if retrieve_proc in self.processes_to_kill_on_cancel: - self.processes_to_kill_on_cancel.remove(retrieve_proc) - # wait for other processes (if any) - for proc in self.processes_to_kill_on_cancel: - if proc.wait() != 0: + file_ok = False + hmac_algorithm = DEFAULT_HMAC_ALGORITHM + for hmac_algo in get_supported_hmac_algo(hmac_algorithm): + try: + if self._verify_hmac(filename, hmacfile, hmac_algo): + file_ok = True + break + except qubes.exc.QubesException as e: + self.log.debug( + 'Failed to verify {} using {}: {}'.format( + hmacfile, hmac_algo, str(e))) + # Ignore exception here, try the next algo + pass + if not file_ok: raise qubes.exc.QubesException( - "Backup header retrieval failed (exit code {})".format( - proc.wait()) - ) + "Corrupted backup header (hmac verification " + "failed). Is the password correct?") + filename = os.path.join(self.tmpdir, filename) + header_data = BackupHeader(open(filename, 'r').read()) + os.unlink(filename) + return header_data def _start_inner_extraction_worker(self, queue, relocate): @@ -1719,6 +1802,9 @@ class BackupRestore(object): elif format_version in [3, 4]: extractor_params['compression_filter'] = \ self.header_data.compression_filter + if format_version == 4: + # encryption already handled + extractor_params['encrypted'] = False extract_proc = ExtractWorker3(**extractor_params) else: raise NotImplementedError( @@ -1737,7 +1823,14 @@ class BackupRestore(object): offline_mode=True) return backup_app else: - self._verify_hmac("qubes.xml.000", "qubes.xml.000.hmac") + if self.header_data.version in [2, 3]: + self._retrieve_backup_header_files( + ['qubes.xml.000', 'qubes.xml.000.hmac']) + self._verify_hmac("qubes.xml.000", "qubes.xml.000.hmac") + else: + self._retrieve_backup_header_files(['qubes.xml.000.enc']) + self._verify_and_decrypt('qubes.xml.000.enc') + queue = Queue() queue.put("qubes.xml.000") queue.put(QUEUE_FINISHED) @@ -1785,6 +1878,7 @@ class BackupRestore(object): try: filename = None + hmacfile = None nextfile = None while True: if self.canceled: @@ -1808,30 +1902,58 @@ class BackupRestore(object): if not filename or filename == "EOF": break - hmacfile = filelist_pipe.readline().strip() - - if self.canceled: - break # if reading archive directly with tar, wait for next filename - # tar prints filename before processing it, so wait for # the next one to be sure that whole file was extracted if not self.backup_vm: nextfile = filelist_pipe.readline().strip() - self.log.debug("Getting hmac:" + hmacfile) - if not hmacfile or hmacfile == "EOF": - # Premature end of archive, either of tar1_command or - # vmproc exited with error - break + if self.header_data.version in [2, 3]: + if not self.backup_vm: + hmacfile = nextfile + nextfile = filelist_pipe.readline().strip() + else: + hmacfile = filelist_pipe.readline().strip() + + if self.canceled: + break + + self.log.debug("Getting hmac:" + hmacfile) + if not hmacfile or hmacfile == "EOF": + # Premature end of archive, either of tar1_command or + # vmproc exited with error + break + else: # self.header_data.version == 4 + if not filename.endswith('.enc'): + raise qubes.exc.QubesException( + 'Invalid file extension found in archive: {}'. + format(filename)) if not any(map(lambda x: filename.startswith(x), vms_dirs)): self.log.debug("Ignoring VM not selected for restore") os.unlink(os.path.join(self.tmpdir, filename)) - os.unlink(os.path.join(self.tmpdir, hmacfile)) + if hmacfile: + os.unlink(os.path.join(self.tmpdir, hmacfile)) continue - if self._verify_hmac(filename, hmacfile): - to_extract.put(os.path.join(self.tmpdir, filename)) + if self.header_data.version in [2, 3]: + self._verify_hmac(filename, hmacfile) + else: + # _verify_and_decrypt will write output to a file with + # '.enc' extension cut off. This is safe because: + # - `scrypt` tool will override output, so if the file was + # already there (received from the VM), it will be removed + # - incoming archive extraction will refuse to override + # existing file, so if `scrypt` already created one, + # it can not be manipulated by the VM + # - when the file is retrieved from the VM, it appears at + # the final form - if it's visible, VM have no longer + # influence over its content + # + # This all means that if the file was correctly verified + # + decrypted, we will surely access the right file + filename = self._verify_and_decrypt(filename) + to_extract.put(os.path.join(self.tmpdir, filename)) if self.canceled: raise BackupCanceledError("Restore canceled", diff --git a/rpm_spec/core-dom0.spec b/rpm_spec/core-dom0.spec index abece061..1cf8ef79 100644 --- a/rpm_spec/core-dom0.spec +++ b/rpm_spec/core-dom0.spec @@ -86,6 +86,7 @@ Requires: gnome-packagekit Requires: cronie Requires: bsdtar Requires: python3-jinja2 +Requires: scrypt # for qubes-hcl-report Requires: dmidecode Requires: PyQt4 From 45709b510aa72d40b9da8f63c1d9f51b60763f68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 23 Feb 2017 00:55:43 +0100 Subject: [PATCH 05/14] backup: minor fixes after bringing back scrypt support --- qubes/backup.py | 49 +++++++++++++++++++------------------ qubes/tests/integ/backup.py | 2 +- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/qubes/backup.py b/qubes/backup.py index e5d5619d..a50d0351 100644 --- a/qubes/backup.py +++ b/qubes/backup.py @@ -44,6 +44,7 @@ import qubes import qubes.core2migration import qubes.storage import qubes.storage.file +import qubes.vm.templatevm QUEUE_ERROR = "ERROR" @@ -65,6 +66,7 @@ BLKSIZE = 512 _re_alphanum = re.compile(r'^[A-Za-z0-9-]*$') + class BackupCanceledError(qubes.exc.QubesException): def __init__(self, msg, tmpdir=None): super(BackupCanceledError, self).__init__(msg) @@ -241,7 +243,7 @@ def launch_proc_with_pty(args, stdin=None, stdout=None, stderr=None, echo=True): p = subprocess.Popen(args, stdin=stdin, stdout=stdout, stderr=stderr, preexec_fn=lambda: set_ctty(pty_slave, pty_master)) os.close(pty_slave) - return p, os.fdopen(pty_master, 'w+') + return p, os.fdopen(pty_master, 'wb+', buffering=0) def launch_scrypt(action, input_name, output_name, passphrase): @@ -262,9 +264,9 @@ def launch_scrypt(action, input_name, output_name, passphrase): stderr=subprocess.PIPE, echo=False) if action == 'enc': - prompts = ('Please enter passphrase: ', 'Please confirm passphrase: ') + prompts = (b'Please enter passphrase: ', b'Please confirm passphrase: ') else: - prompts = ('Please enter passphrase: ',) + prompts = (b'Please enter passphrase: ',) for prompt in prompts: actual_prompt = p.stderr.read(len(prompt)) if actual_prompt != prompt: @@ -618,6 +620,7 @@ class Backup(object): passio_popen=True, passio_stderr=True) vmproc.stdin.write((self.target_dir. replace("\r", "").replace("\n", "") + "\n").encode()) + vmproc.stdin.flush() backup_stdout = vmproc.stdin self.processes_to_kill_on_cancel.append(vmproc) else: @@ -701,7 +704,7 @@ class Backup(object): # also use our tar writer when renaming file assert not stat.S_ISDIR(file_stat.st_mode),\ "Renaming directories not supported" - tar_cmdline = ['python', '-m', 'qubes.tarwriter', + tar_cmdline = ['python3', '-m', 'qubes.tarwriter', '--override-name=%s' % ( os.path.join(file_info.subdir, os.path.basename( file_info.name))), @@ -752,7 +755,7 @@ class Backup(object): ) self.log.debug( - "12 returned: {}".format(run_error)) + "Wait_backup_feedback returned: {}".format(run_error)) if self.canceled: try: @@ -978,7 +981,6 @@ class ExtractWorker2(Process): try: self.__run__() except Exception as e: - exc_type, exc_value, exc_traceback = sys.exc_info() # Cleanup children for process in [self.decompressor_process, self.decryptor_process, @@ -989,7 +991,7 @@ class ExtractWorker2(Process): except OSError: pass process.wait() - self.log.error("ERROR: " + unicode(e)) + self.log.error("ERROR: " + str(e)) raise def handle_dir_relocations(self, dirname): @@ -1127,12 +1129,6 @@ class ExtractWorker2(Process): 'vmproc': self.vmproc, 'addproc': self.tar2_process, } - common_args = { - 'backup_target': pipe, - 'hmac': None, - 'vmproc': self.vmproc, - 'addproc': self.tar2_process - } if self.encrypted: # Start decrypt self.decryptor_process = subprocess.Popen( @@ -1365,6 +1361,7 @@ def get_supported_hmac_algo(hmac_algorithm=None): proc = subprocess.Popen(['openssl', 'list-message-digest-algorithms'], stdout=subprocess.PIPE) for algo in proc.stdout.readlines(): + algo = algo.decode('ascii') if '=>' in algo: continue yield algo.strip() @@ -1533,10 +1530,13 @@ class BackupRestore(object): vmproc = self.backup_vm.run_service('qubes.Restore', passio_popen=True, passio_stderr=True) vmproc.stdin.write( - self.backup_location.replace("\r", "").replace("\n", "") + "\n") + (self.backup_location.replace("\r", "").replace("\n", + "") + "\n").encode()) + vmproc.stdin.flush() # Send to tar2qfile the VMs that should be extracted - vmproc.stdin.write(" ".join(filelist) + "\n") + vmproc.stdin.write((" ".join(filelist) + "\n").encode()) + vmproc.stdin.flush() self.processes_to_kill_on_cancel.append(vmproc) backup_stdin = vmproc.stdout @@ -1581,8 +1581,7 @@ class BackupRestore(object): def _verify_hmac(self, filename, hmacfile, algorithm=None): def load_hmac(hmac_text): - if filter(lambda x: ord(x) not in range(128), - hmac_text): + if any(ord(x) not in range(128) for x in hmac_text): raise qubes.exc.QubesException( "Invalid content of {}".format(hmacfile)) hmac_text = hmac_text.strip().split("=") @@ -1695,10 +1694,10 @@ class BackupRestore(object): retrieve_proc.wait(), extract_stderr )) - actual_files = filelist.splitlines() + actual_files = filelist.decode('ascii').splitlines() if sorted(actual_files) != sorted(files): raise qubes.exc.QubesException( - 'unexpected files in archive: got {!r}, expeced {!r}'.format( + 'unexpected files in archive: got {!r}, expected {!r}'.format( actual_files, files )) for f in files: @@ -1767,7 +1766,7 @@ class BackupRestore(object): "Corrupted backup header (hmac verification " "failed). Is the password correct?") filename = os.path.join(self.tmpdir, filename) - header_data = BackupHeader(open(filename, 'r').read()) + header_data = BackupHeader(open(filename, 'rb').read()) os.unlink(filename) return header_data @@ -1895,7 +1894,7 @@ class BackupRestore(object): if nextfile is not None: filename = nextfile else: - filename = filelist_pipe.readline().strip() + filename = filelist_pipe.readline().decode('ascii').strip() self.log.debug("Getting new file:" + filename) @@ -1906,14 +1905,16 @@ class BackupRestore(object): # tar prints filename before processing it, so wait for # the next one to be sure that whole file was extracted if not self.backup_vm: - nextfile = filelist_pipe.readline().strip() + nextfile = filelist_pipe.readline().decode('ascii').strip() if self.header_data.version in [2, 3]: if not self.backup_vm: hmacfile = nextfile - nextfile = filelist_pipe.readline().strip() + nextfile = filelist_pipe.readline().\ + decode('ascii').strip() else: - hmacfile = filelist_pipe.readline().strip() + hmacfile = filelist_pipe.readline().\ + decode('ascii').strip() if self.canceled: break diff --git a/qubes/tests/integ/backup.py b/qubes/tests/integ/backup.py index 15d37c4d..5d85134e 100644 --- a/qubes/tests/integ/backup.py +++ b/qubes/tests/integ/backup.py @@ -508,7 +508,7 @@ class TC_10_BackupVMMixin(BackupTestsMixin): p = self.backupvm.run("ls /var/tmp/backup*/qubes-backup*", passio_popen=True) (backup_path, _) = p.communicate() - backup_path = backup_path.strip() + backup_path = backup_path.decode().strip() self.restore_backup(source=backup_path, appvm=self.backupvm) From c5a8135fdbc2a1039e9858970b3eaeb8f5ca12ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 23 Feb 2017 03:07:31 +0100 Subject: [PATCH 06/14] fix misplaced comment --- linux/system-config/block-snapshot | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/linux/system-config/block-snapshot b/linux/system-config/block-snapshot index d35eed2f..a922989e 100755 --- a/linux/system-config/block-snapshot +++ b/linux/system-config/block-snapshot @@ -290,11 +290,11 @@ case "$command" in # Commit template changes domain=$(cat "$HOTPLUG_STORE-domain") if [ "$domain" ]; then - # Dont stop on errors if [ -r /var/lib/qubes/qubes-test.xml -a \ "${domain#test-}" != "$domain" ]; then export QUBES_XML_PATH=/var/lib/qubes/qubes-test.xml fi + # Dont stop on errors /usr/bin/qvm-template-commit --offline-mode "$domain" || true fi fi From a6c7da606127a3759aa9edbf669117846b43b37f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 23 Feb 2017 03:07:57 +0100 Subject: [PATCH 07/14] tests: be even more defensive on cleaning up VMs Don't fail even if qubes-test.xml do not load at all because of syntax error - for example empty file. --- qubes/tests/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qubes/tests/__init__.py b/qubes/tests/__init__.py index f68f3bac..2d63df7f 100644 --- a/qubes/tests/__init__.py +++ b/qubes/tests/__init__.py @@ -700,7 +700,7 @@ class SystemTestsMixin(object): try: cls.remove_vms(vm for vm in qubes.Qubes(xmlpath).domains if vm.name.startswith(prefix)) - except qubes.exc.QubesException: + except (qubes.exc.QubesException, lxml.etree.XMLSyntaxError): # If qubes-test.xml is broken that much it doesn't even load, # simply remove it. VMs will be cleaned up the hard way. # TODO logging? From 751415434c6409ed284743c3ceeadf189c28ea9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Sat, 25 Feb 2017 02:28:54 +0100 Subject: [PATCH 08/14] backup: make hmac verification more defensive Check HMAC file size, read it as binary or with 'ascii' encoding only. --- qubes/backup.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/qubes/backup.py b/qubes/backup.py index a50d0351..1d73cc98 100644 --- a/qubes/backup.py +++ b/qubes/backup.py @@ -61,6 +61,8 @@ CURRENT_BACKUP_FORMAT_VERSION = '4' MAX_STDERR_BYTES = 1024 # header + qubes.xml max size HEADER_QUBES_XML_MAX_SIZE = 1024 * 1024 +# hmac file max size - regardless of backup format version! +HMAC_MAX_SIZE = 4096 BLKSIZE = 512 @@ -1597,6 +1599,11 @@ class BackupRestore(object): passphrase = self.passphrase.encode('utf-8') self.log.debug("Verifying file {}".format(filename)) + if os.stat(os.path.join(self.tmpdir, hmacfile)).st_size > \ + HMAC_MAX_SIZE: + raise qubes.exc.QubesException('HMAC file {} too large'.format( + hmacfile)) + if hmacfile != filename + ".hmac": raise qubes.exc.QubesException( "ERROR: expected hmac for {}, but got {}". @@ -1606,8 +1613,9 @@ class BackupRestore(object): # in case of 'scrypt' _verify_hmac is only used for backup header assert filename == HEADER_FILENAME self._verify_and_decrypt(hmacfile, HEADER_FILENAME + '.dec') - if open(os.path.join(self.tmpdir, filename)).read() != \ - open(os.path.join(self.tmpdir, filename + '.dec')).read(): + if open(os.path.join(self.tmpdir, filename), 'rb').read() != \ + open(os.path.join(self.tmpdir, filename + '.dec'), + 'rb').read(): raise qubes.exc.QubesException( 'Invalid hmac on {}'.format(filename)) else: @@ -1625,7 +1633,7 @@ class BackupRestore(object): else: self.log.debug("Loading hmac for file {}".format(filename)) hmac = load_hmac(open(os.path.join(self.tmpdir, hmacfile), - 'r').read()) + 'r', encoding='ascii').read()) if len(hmac) > 0 and load_hmac(hmac_stdout.decode('ascii')) == hmac: os.unlink(os.path.join(self.tmpdir, hmacfile)) From c4549735963c023e8bf60a6dda5d71d92d8cc4b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 27 Feb 2017 20:56:16 +0100 Subject: [PATCH 09/14] qubes/mgmt: use keyword arguments in events QubesOS/qubes-issues#853 --- qubes/mgmt.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qubes/mgmt.py b/qubes/mgmt.py index 7ee77e90..95ef2105 100644 --- a/qubes/mgmt.py +++ b/qubes/mgmt.py @@ -112,7 +112,7 @@ class QubesMgmt(object): @not_in_api def fire_event_for_permission(self, **kwargs): return self.src.fire_event_pre('mgmt-permission:{}'.format(self.method), - self.dest, self.arg, **kwargs) + dest=self.dest, arg=self.arg, **kwargs) @not_in_api def fire_event_for_filter(self, iterable, **kwargs): From f4616fc36649ec351f9d4221f224e1732a333718 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 27 Feb 2017 21:42:06 +0100 Subject: [PATCH 10/14] qubesd: make qubesd socket qubes-group owned QubesOS/qubes-issues#853 --- qubes/tools/qubesd.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qubes/tools/qubesd.py b/qubes/tools/qubesd.py index 4217a3af..916a1c72 100644 --- a/qubes/tools/qubesd.py +++ b/qubes/tools/qubesd.py @@ -4,6 +4,7 @@ import asyncio import functools import io import os +import shutil import signal import struct import traceback @@ -156,6 +157,7 @@ def main(args=None): old_umask = os.umask(0o007) server = loop.run_until_complete(loop.create_unix_server( functools.partial(QubesDaemonProtocol, app=args.app), QUBESD_SOCK)) + shutil.chown(QUBESD_SOCK, group='qubes') os.umask(old_umask) del old_umask From 6ab7032b11bbdb0a8ed29c940d1775865a094b9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 27 Feb 2017 21:42:30 +0100 Subject: [PATCH 11/14] qubes/mgmt: encode VM name without quotes That's how it is in the specification. QubesOS/qubes-issues#853 --- qubes/mgmt.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qubes/mgmt.py b/qubes/mgmt.py index 95ef2105..02202bd8 100644 --- a/qubes/mgmt.py +++ b/qubes/mgmt.py @@ -137,7 +137,7 @@ class QubesMgmt(object): domains = self.fire_event_for_filter(self.app.domains) return ''.join('{} class={} state={}\n'.format( - self.repr(vm), + vm.name, vm.__class__.__name__, vm.get_power_state()) for vm in sorted(domains)) From 216907580736b749d28d6f9950d89700191cef54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 27 Feb 2017 21:43:14 +0100 Subject: [PATCH 12/14] qubesd: fix response message header Type is not 16 bit big-endian. Encode it as 8bit code and \x00 as delimiter explicitly. QubesOS/qubes-issues#853 --- qubes/tools/qubesd.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qubes/tools/qubesd.py b/qubes/tools/qubesd.py index 916a1c72..e2b9bfc1 100644 --- a/qubes/tools/qubesd.py +++ b/qubes/tools/qubesd.py @@ -19,7 +19,7 @@ QUBESD_SOCK = '/var/run/qubesd.sock' class QubesDaemonProtocol(asyncio.Protocol): buffer_size = 65536 - header = struct.Struct('!H') + header = struct.Struct('Bx') def __init__(self, *args, app, debug=False, **kwargs): super().__init__(*args, **kwargs) From 3e0f22593805ccc3321073f1507c04a1d2af16eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 27 Feb 2017 21:57:56 +0100 Subject: [PATCH 13/14] qubes: allow 'property' object comparing with str This will allow checking if a given name is valid property name, using simple `name in vm.property_list()`. QubesOS/qubes-issues#853 --- qubes/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qubes/__init__.py b/qubes/__init__.py index ff56dd91..697605ce 100644 --- a/qubes/__init__.py +++ b/qubes/__init__.py @@ -306,6 +306,8 @@ class property(object): # pylint: disable=redefined-builtin,invalid-name return NotImplemented def __eq__(self, other): + if isinstance(other, str): + return self.__name__ == other return isinstance(other, property) and self.__name__ == other.__name__ From f7d73893d78b1dfeff7e784fc2cfdf04579180c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Wed, 1 Mar 2017 21:50:06 +0100 Subject: [PATCH 14/14] qubes/storage: py3k related fixes --- qubes/storage/domain.py | 9 +++++---- qubes/vm/qubesvm.py | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/qubes/storage/domain.py b/qubes/storage/domain.py index 9e511f01..255e1a39 100644 --- a/qubes/storage/domain.py +++ b/qubes/storage/domain.py @@ -55,7 +55,7 @@ class DomainPool(Pool): # /qubes-block-devices/foo/{desc,mode,size} we need to merge this devices = {} for untrusted_device_path in untrusted_qubes_devices: - if not all(c in safe_set for c in untrusted_device_path): + if not all(chr(c) in safe_set for c in untrusted_device_path): msg = ("%s vm's device path name contains unsafe characters. " "Skipping it.") self.vm.log.warning(msg % self.vm.name) @@ -63,7 +63,8 @@ class DomainPool(Pool): # name can be trusted because it was checked as a part of # untrusted_device_path check above - _, _, name, untrusted_atr = untrusted_device_path.split('/', 4) + _, _, name, untrusted_atr = untrusted_device_path.\ + decode('ascii').split('/', 4) if untrusted_atr in allowed_attributes.keys(): atr = untrusted_atr @@ -75,8 +76,8 @@ class DomainPool(Pool): untrusted_value = qdb.read(untrusted_device_path) allowed_characters = allowed_attributes[atr] - if all(c in allowed_characters for c in untrusted_value): - value = untrusted_value + if all(chr(c) in allowed_characters for c in untrusted_value): + value = untrusted_value.decode('ascii') else: msg = ("{!s} vm's device path {!s} contains unsafe characters") self.vm.log.error(msg.format(self.vm.name, atr)) diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index abf16597..6af8add1 100644 --- a/qubes/vm/qubesvm.py +++ b/qubes/vm/qubesvm.py @@ -536,7 +536,7 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): result += [volume] break - return result + self.volumes.values() + return result + list(self.volumes.values()) @property def libvirt_domain(self):