Project

General

Profile

Bug #12560

Polish the new memory wiping implementation and design doc

Added by intrigeri over 2 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
High
Assignee:
-
Category:
-
Target version:
Start date:
05/18/2017
Due date:
% Done:

100%

Feature Branch:
bugfix/12560-polish-memory-wiping
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

… according to anonym's review (#12354#note-33) i.e.

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.

#### 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?

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.

[[!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"? I really think this design page should list any limitations of the memory poisoning approach.

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.

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.

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). I'm just double-checking that these mkdir calls still are relevant, and no residues from some Git history rewrite.

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: there's currently four, so that's almost 2*4 = 8 minutes of needless waiting. 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. 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).


Related issues

Related to Tails - Bug #12354: Fix shutdown and memory wipe regressions on 3.0~betaN Resolved 03/20/2017

Associated revisions

Revision 9a0aba17 (diff)
Added by intrigeri over 2 years ago

Memory erasure design doc: document one improvement (refs: #12560).

Revision 216cbd4d (diff)
Added by intrigeri over 2 years ago

Memory erasure design doc: make the history lesson (a bit) more optional.

refs: #12560

Revision 0f3a2d08 (diff)
Added by intrigeri over 2 years ago

Memory erasure doc: update a leftover from the kexec area (refs: #12560).

Revision 52bc17a9 (diff)
Added by intrigeri over 2 years ago

Memory erasure design doc: move the history lesson to the end of the page (refs: #12560).

Revision 0ac72180 (diff)
Added by intrigeri over 2 years ago

Memory erasure design doc: document limitations (refs: #12560).

Revision cb94376f (diff)
Added by intrigeri over 2 years ago

Drop obsolete comments (refs: #12560).

Revision 3cd3ad81 (diff)
Added by intrigeri over 2 years ago

Drop obsolete call to mkdir (refs: #12560).

Revision e726c9ec (diff)
Added by intrigeri over 2 years ago

Test suite: only test once that Tails, booted on DVD, eventually shuts down after wiping memory (refs: #12560).

Revision 5a422c4b
Added by anonym over 2 years ago

Merge remote-tracking branch 'origin/bugfix/12560-polish-memory-wiping' into testing

Fix-committed: #12560

History

#1 Updated by intrigeri over 2 years ago

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

#2 Updated by intrigeri over 2 years ago

  • Description updated (diff)

#3 Updated by intrigeri over 2 years ago

  • Status changed from Confirmed to In Progress

#4 Updated by intrigeri over 2 years ago

  • 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.

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?

216cbd4 and 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.

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! 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.

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 => 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 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 #12565 back, which makes me sad. Instead I did 90ba61a4696c27604b0f343338ab42fb260f3746 and 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 over 2 years ago

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

#6 Updated by intrigeri over 2 years ago

  • 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 over 2 years ago

  • Status changed from In Progress to 11
  • 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 => 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 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 #12565 back, which makes me sad.

Ah, that's right.

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

Yes!

Merged!

#8 Updated by intrigeri over 2 years ago

  • Status changed from 11 to Resolved

Also available in: Atom PDF