Bug #9349

Tails Installer logging location is unsafe

Added by intrigeri 2015-05-06 08:15:29 . Updated 2015-08-11 10:42:29 .

Status:
Resolved
Priority:
Elevated
Assignee:
Category:
Installation
Target version:
Start date:
2015-05-06
Due date:
% Done:

100%

Feature Branch:
liveusb-creator:bugfix/9349-safer-error-logging
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Installer
Deliverable for:

Description

The way I understand the code, we’re logging to /tmp/liveusb-creator.log. That’s a well-known name in a world-writable location, so leaves room for some kinds of attacks e.g. arbitrary file modification. We should fix that before we upload this piece of software to Debian.


Subtasks


Related issues

Blocks Tails - Feature #8557: Have Tails Installer uploaded and accepted into Debian Resolved 2015-01-06

History

#1 Updated by intrigeri 2015-05-06 08:15:49

  • blocks Feature #8557: Have Tails Installer uploaded and accepted into Debian added

#2 Updated by intrigeri 2015-05-06 08:15:59

  • blocks #8538 added

#3 Updated by intrigeri 2015-06-05 18:05:47

  • Assignee deleted (intrigeri)
  • % Done changed from 0 to 50
  • QA Check set to Ready for QA
  • Feature Branch set to bugfix/9349-safer-error-logging

#4 Updated by intrigeri 2015-06-05 18:08:01

  • Feature Branch changed from bugfix/9349-safer-error-logging to liveusb-creator:bugfix/9349-safer-error-logging

#5 Updated by intrigeri 2015-06-05 18:10:02

  • Assignee set to anonym

#6 Updated by intrigeri 2015-06-05 18:11:46

  • Status changed from Confirmed to In Progress

#7 Updated by anonym 2015-06-18 04:37:42

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Ready for QA to Info Needed

Tested by hot-patching liveusb-creator in a running Tails session. The logging works fine, however, even if there’s no error, an empty log file is created (which is quite clear given the usage of NamedTemporaryFile() in _setup_error_log_file(), which is always run). This is different from before, so I think it’s worth noting. Personally I do not like programs that unnecessarily leave cruft like this behind.

We could delay the call of _setup_error_log_file() until write_log() actually is called (i.e. upon error), and have _error_log_filename set to None until then, but since we read this field outside of write_log() we get a dependency that perhaps is a bit ugly. Since write_log() is only used there, we could kill it, and this dependency would be eliminated:

--- a/liveusb/creator.py
+++ b/liveusb/creator.py
@@ -95,7 +95,7 @@ class LiveUSBCreator(object):
                 print >> sys.stderr, _("You must run this application as root")
                 sys.exit(1)
         self._setup_logger()
-        self._error_log_filename = self._setup_error_log_file()
+        self._error_log_filename = None

     def _setup_error_log_file(self):
         temp = tempfile.NamedTemporaryFile(mode='a', delete=False,
@@ -348,7 +348,10 @@ class LiveUSBCreator(object):
         err = unicode_to_utf8(err)
         self.output.write(out + '\n' + err + '\n')
         if proc.returncode:
-            self.write_log()
+            if not self._error_log_filename:
+                self._error_log_filename = self._setup_error_log_file()
+            with open(self._error_log_filename, 'a') as out:
+                out.write(self.output.getvalue())
             if not passive:
                 raise LiveUSBError(_(
                         "There was a problem executing the following command: `%(command)s`.\nA more detailed error log has been written to '%(filename)s'.")
@@ -518,10 +521,6 @@ class LiveUSBCreator(object):
                                          " previous LiveOS: %(message)s")
                                          % {'message': str(e)})

-    def write_log(self):
-        """ Write out our subprocess stdout/stderr to a log file """
-        with open(self._error_log_filename, 'a') as out:
-            out.write(self.output.getvalue())

     def get_liveos(self):
         return self.get_liveos_file_path(config['main_liveos_dir'])


What do you think?

#8 Updated by intrigeri 2015-06-18 07:25:25

  • Assignee changed from intrigeri to anonym

anonym wrote:
> Tested by hot-patching liveusb-creator in a running Tails session. The logging works fine, however, even if there’s no error, an empty log file is created (which is quite clear given the usage of NamedTemporaryFile() in _setup_error_log_file(), which is always run). This is different from before, so I think it’s worth noting. Personally I do not like programs that unnecessarily leave cruft like this behind.

Got it.

> We could delay the call of _setup_error_log_file() until write_log() actually is called (i.e. upon error), and have _error_log_filename set to None until then, but since we read this field outside of write_log() we get a dependency that perhaps is a bit ugly. Since write_log() is only used there, we could kill it, and this dependency would be eliminated:
> […]
> What do you think?

I dislike inlining stuff, that previously was in a dedicated function, like this. Besides, we might want to log to that file from other places in the future.

I propose to instead just delete the log file if it’s empty, when successfully exiting the program. What do you think?

#9 Updated by anonym 2015-06-27 06:01:51

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Info Needed to Dev Needed

intrigeri wrote:
> I propose to instead just delete the log file if it’s empty, when successfully exiting the program. What do you think?

Sure.

#10 Updated by intrigeri 2015-06-28 03:45:30

  • Target version changed from Tails_1.4.1 to Tails_1.5

Too late for having this reviewed’n’merged for 1.4.1.

#11 Updated by intrigeri 2015-07-13 05:41:59

  • Assignee changed from intrigeri to anonym
  • % Done changed from 50 to 60
  • QA Check changed from Dev Needed to Ready for QA

Implemented what we agreed upon => anonym (since you had a look at this code already, and you’ll be the RM again in a week), please review’n’merge.

#12 Updated by anonym 2015-07-24 03:30:24

  • Status changed from In Progress to Fix committed
  • Assignee deleted (anonym)
  • % Done changed from 60 to 100
  • QA Check changed from Ready for QA to Pass

intrigeri wrote:
> Implemented what we agreed upon => anonym (since you had a look at this code already, and you’ll be the RM again in a week), please review’n’merge.

Looks and works great! Merged!

#13 Updated by BitingBird 2015-08-11 10:42:29

  • Status changed from Fix committed to Resolved