Tails

#1 Updated by intrigeri 2017-05-18 15:35:19

• related to Bug #12354: Fix shutdown and memory wipe regressions on 3.0~betaN added

#2 Updated by intrigeri 2017-05-25 17:13:05

• Description updated

#3 Updated by intrigeri 2017-05-25 17:57:59

• Status changed from Confirmed to In Progress

Applied in changeset commit:9a0aba1715bae64de87fb1a1ce234b518d18565e.

#4 Updated by intrigeri 2017-05-25 17:58:18

• Status changed from In Progress to Confirmed
• Feature Branch set to bugfix/12560-polish-memory-wiping

anonym wrote:
> In wiki/src/contribute/design/memory_erasure.mdwn:
> > In order to protect against memory recovery such as cold boot attack,
> > most of the system RAM is overwritten when Tails is being shutdown or when the
> > boot medium is physically removed.
>
> I guess we could add “and immediately after applications exit” or something to that effect? That is an improvement over the old kexec-based approach when treated in isolation.

commit:9a0aba1

> > #### The big picture
>
> Can we put the story about the old design in another sub section, like “##### Obsolete kexec-based approach”, and the current one in “##### Current memory poisoning-based approach”? I.e. just insert these two headers ( and possibly adapt the edges of the affected paragraphs so the text still makes sense). I’d just like to make the history lesson more optional. :) Is it really needed for understanding the current design?

commit:216cbd4 and commit:52bc17a

> > Different kinds of events trigger the memory erasure process. All lead
> > to run the `tails-kexec` shutdown script.
>
> There’s no tails-kexec any more.

commit:0f3a2d0

> > [[!tails_gitweb features/erase_memory.feature desc=“Automated tests”]]
> > ensure that the most important parts of memory are erased this way.
>
> Can we describe “the most important parts of memory” a bit better than this (not necessarily at this place in the design page, btw)? Or do we want the .feature file to enumerate what we think is “important”?

The scenarios description in Gherkin already describe what we decided was important before I started my work, and they’ll always be up-to-date (as opposed to info duplicated in the design doc). So I’d rather not add anything about it to the design doc.

> I really think this design page should list any limitations of the memory poisoning approach.

Sure! commit:0ac7218

> In features/step_definitions/common_steps.rb:
> > -def mount_USB_drive(disk, fs)
> > +def mount_USB_drive(disk, fs_options = {})
> > + fs_options[:encrypted] ||= false
>
> Just a FYI: nil is treated as false when interpreted as a boolean, so this assignment is unnecessary. That said, I don’t really mind making the default more explicit, for readability.

OK, got it. In Python for example, dict[non-existing_key] would trigger an exception, so I prefer being explicit and not relying on the fact that Ruby would be more permissive regarding non-existing keys.

> In features/step_definitions/erase_memory.rb:
> > kernel_mem_reserved_k = 64*1024 # Duplicated in /usr/share/initramfs-tools/scripts/init-premount/memory_wipe
> > […]
> > admin_mem_reserved_k = 128*1024 # Duplicated in /usr/share/initramfs-tools/scripts/init-premount/memory_wipe
>
> These don’t duplicate anything any longer.

commit:cb94376

> In config/chroot_local-includes/usr/share/initramfs-tools/hooks/shutdown:
> > # systemd-shutdown itself
> > mkdir -p $DESTDIR/lib/systemd
> > copy_exec /lib/systemd/systemd-shutdown /shutdown
> >
> > # Our shutdown hook (run from inside the initramfs)
> > mkdir -p $DESTDIR/lib/systemd/system-shutdown
> > copy_file "regular file" \
> > @ /usr/local/lib/initramfs-pre-shutdown-hook \@
> > @ /lib/systemd/system-shutdown/tails@
>
> To me it looks like you are following the common pattern of mkdir -p:ing the target directories for the subsequent copy actions, but these directories are not used as the target (in fact they are not used at all).

The first one indeed seems useless => commit:3cd3ad8. Let’s see what Jenkins thinks.

But the second one is used (by the calls to copy_exec). Did I miss something, or should you take a look at /usr/share/initramfs-tools/hook-functions (since you insisted on using initramfs-tools :P)?

> In features/emergency_shutdown.feature:
> > Then I find very few patterns in the guest’s memory
> > And Tails eventually shuts down
>
> This means that we will always wait the full two minutes (the “Happy dumping!” timeout) for each of these scenarios:

Correct.

> there’s currently four, so that’s almost 2*4 = 8 minutes of needless waiting.

Indeed. We could probably decrease the “Happy dumping!” timeout a lot, and in turn decrease the timeout for Tails eventually (shuts down|restarts). But let’s see first because that might be unneeded:

> I’m tempted to suggest that we add a scenario dedicated to make sure that the shutdown happens without any of the memory erasure checks, and that we simply remove the Tails eventually shuts down step from these four scenarios.

Actually, we already have one for DVD boot: Scenario: Tails shuts down on DVD boot medium removal. So I did commit:e726c9e, and now only 1 of these 4 scenarios (the USB one) is affected by the issue you’re rightfully raising. Good enough?

> In fact, I think you should move these four scenario to erase_memory.feature as well — I know it can be argued both ways, but I slightly prefer what I suggest (so no strong opinion here, just ignore this move if you disagree).

We could indeed move the 4 “Tails erases memory” scenarios to erase_memory.feature.
It would surely feel nicer… but it would also bring Bug #12565 back, which makes me sad. Instead I did commit:90ba61a4696c27604b0f343338ab42fb260f3746 and commit:cabe947a2f6c13de6cfc679dce712d58e66f3cd1, so at least emergency_shutdown.feature feels nicer now. Good enough?

I’ll push my stuff to Jenkins and will reassign to you only once I’ve seen good results there.

Thanks for this review! Your attention to detail is appreciated :)

#5 Updated by intrigeri 2017-05-25 17:58:35

• Status changed from Confirmed to In Progress
• % Done changed from 0 to 20

#6 Updated by intrigeri 2017-05-26 05:11:45

• Assignee changed from intrigeri to anonym
• % Done changed from 20 to 50
• QA Check set to Ready for QA

The only failure I’ve seen on Jenkins seems totally unrelated.

#7 Updated by anonym 2017-05-30 08:31:13

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

intrigeri wrote:
> anonym wrote:
> > > [[!tails_gitweb features/erase_memory.feature desc=“Automated tests”]]
> > > ensure that the most important parts of memory are erased this way.
> >
> > Can we describe “the most important parts of memory” a bit better than this (not necessarily at this place in the design page, btw)? Or do we want the .feature file to enumerate what we think is “important”?
>
> The scenarios sdsdsdsdescription in Gherkin already describe what we decided was important before I started my work, and they’ll always be up-to-date (as opposed to info duplicated in the design doc). So I’d rather not add anything about it to the design doc.

Ack. I think we should start doing this whenever possible.

> > In config/chroot_local-includes/usr/share/initramfs-tools/hooks/shutdown:
> > > # systemd-shutdown itself
> > > mkdir -p $DESTDIR/lib/systemd
> > > copy_exec /lib/systemd/systemd-shutdown /shutdown
> > >
> > > # Our shutdown hook (run from inside the initramfs)
> > > mkdir -p $DESTDIR/lib/systemd/system-shutdown
> > > copy_file "regular file" \
> > > @ /usr/local/lib/initramfs-pre-shutdown-hook \@
> > > @ /lib/systemd/system-shutdown/tails@
> >
> > To me it looks like you are following the common pattern of mkdir -p:ing the target directories for the subsequent copy actions, but these directories are not used as the target (in fact they are not used at all).
>
> The first one indeed seems useless => commit:3cd3ad8. Let’s see what Jenkins thinks.
>
> But the second one is used (by the calls to copy_exec). Did I miss something, or should you take a look at /usr/share/initramfs-tools/hook-functions (since you insisted on using initramfs-tools :P)?

No, you did not miss anything — you have now made sure that only the relevant ones remain, so I’m happy!

> > In features/emergency_shutdown.feature:
> > > Then I find very few patterns in the guest’s memory
> > > And Tails eventually shuts down
> >
> > This means that we will always wait the full two minutes (the “Happy dumping!” timeout) for each of these scenarios:
>
> Correct.
>
> > there’s currently four, so that’s almost 2*4 = 8 minutes of needless waiting.
>
> Indeed. We could probably decrease the “Happy dumping!” timeout a lot, and in turn decrease the timeout for Tails eventually (shuts down|restarts). But let’s see first because that might be unneeded:
>
> > I’m tempted to suggest that we add a scenario dedicated to make sure that the shutdown happens without any of the memory erasure checks, and that we simply remove the Tails eventually shuts down step from these four scenarios.
>
> Actually, we already have one for DVD boot: Scenario: Tails shuts down on DVD boot medium removal. So I did commit:e726c9e, and now only 1 of these 4 scenarios (the USB one) is affected by the issue you’re rightfully raising. Good enough?

Ok, good enough!

> > In fact, I think you should move these four scenario to erase_memory.feature as well — I know it can be argued both ways, but I slightly prefer what I suggest (so no strong opinion here, just ignore this move if you disagree).
>
> We could indeed move the 4 “Tails erases memory” scenarios to erase_memory.feature.
> It would surely feel nicer… but it would also bring Bug #12565 back, which makes me sad.

Ah, that’s right.

> Instead I did commit:90ba61a4696c27604b0f343338ab42fb260f3746 and commit:cabe947a2f6c13de6cfc679dce712d58e66f3cd1, so at least emergency_shutdown.feature feels nicer now. Good enough?

Yes!

Merged!

#8 Updated by intrigeri 2017-06-12 16:10:08

• Status changed from Fix committed to Resolved