Tails

#1 Updated by segfault 2018-01-22 23:44:31

• Assignee changed from segfault to anonym
• Target version changed from Tails_3.9 to Tails_3.6
• % Done changed from 0 to 50
• QA Check set to Ready for QA

#2 Updated by segfault 2018-01-23 00:33:03

• Assignee changed from anonym to segfault
• QA Check deleted (Ready for QA)

This is not ready for review yet, because hidden volumes and keyfiles are not yet supported in Disks.

#4 Updated by segfault 2018-01-23 01:19:18

• % Done changed from 50 to 30

#5 Updated by segfault 2018-01-25 16:01:12

• Subject changed from Support unlocking VeraCrypt partitions in Disks to Iteration 1: Support unlocking VeraCrypt partitions in Disks

#7 Updated by segfault 2018-02-10 19:09:14

• Assignee changed from segfault to anonym
• % Done changed from 30 to 70
• QA Check set to Ready for QA

I implemented the unlock dialog for TCRYPT volumes. Please review!

#8 Updated by anonym 2018-02-11 14:06:58

• Assignee changed from anonym to segfault
• QA Check changed from Ready for QA to Dev Needed

The only serious concerns I had was about memory leaks, but that’s no fixed in commit:ccacd3fa990cad99552216bdb36487efe56f93ad (nice catch that the first key file button should not be freed! :)). All my comments below concern commit:9a78b190f845f81c28eea2eb2dfefd1f17cd87be. It’s mostly nitpicking, which indicates you’ve done a great job. Congrats! :)

In on_expander_expanded():

if (gtk_expander_get_expanded (expander))
{
  gtk_widget_show (data->volume_type_label);
  gtk_widget_show (data->tcrypt_hidden_check_button);
  ...
else
{
  gtk_widget_hide (data->volume_type_label);
  gtk_widget_hide (data->tcrypt_hidden_check_button);
  ...

You can cut this code in half (and thus de-duplicate some mentions of identifiers) by eliminating the if statement and use gtk_widget_set_visible():

gboolean visible = gtk_expander_get_expanded (expander);
gtk_widget_set_visible (data->volume_type_label, visible);
gtk_widget_set_visible (data->tcrypt_hidden_check_button, visible);
...

In show_dialog():

// strtoul returns 0 if it gets an empty string, which is fine
data->pim = strtoul (gtk_entry_get_text (GTK_ENTRY (data->pim_entry)), &err, 10);

This is susceptible to integer overflow, right? We probably should use GtkAdjustment-blah here like is done elsewhere in GNOME Disks for integer input. I know it is pedantic; a PIM > MAX_UINT32 would take > ~1 year to unlock if it was possible. :) When dropping strtoul, remember to also stop including stdlib.h.

"Error unlocking encrypted device"

You told me the title of this dialog will express an “attempt”, so perhaps this error should say “Error while attempting to unlock potentially encrypted device” or something like that, for consistency. sajolida to the rescue? :P

#9 Updated by segfault 2018-02-11 14:12:17

• File attempt_to_unlock_veracrypt_truecrypt_volume.png added
• File attempt_to_unlock_veracrypt_volume.png added
• File attempt_to_unlock_volume.png added
• File unlocked_volume.png added
• Assignee changed from segfault to sajolida
• QA Check changed from Dev Needed to Info Needed

Something I forgot is to change the title of the unlock dialog to something reflecting that the device might not actually be encrypted. I would like to have your input on this sajolida.

Currently, the title is “Enter passphrase to unlock”. Kai Lüke, the GNOME Disks maintainer, suggested “attempt to unlock hidden volume” in an email to the devkit-devel list. I find the “hidden” part confusing, because it clashes with the name of the hidden volume feature of TrueCrypt/VeraCrypt. I would prefer something like “Attempt to unlock as VeraCrypt/TrueCrypt volume”, but this is a bit long and even causes the whole unlock dialog to be a lot wider than usual (see attached screenshot).

It looks better if the title is just “Attempt to unlock volume”, but then nothing in the dialog makes clear that the device is treated as a VeraCrypt/TrueCrypt volume.

A third option would be to skip the TrueCrypt part and just use “Attempt to unlock VeraCrypt volume”. This makes the dialog a bit wider, but too much to make it very ugly IMO. But it would leave out the fact that legacy TrueCrypt volumes are also supported. VeraCrypt is different from legacy TrueCrypt, and when using VeraCrypt you can only unlock TrueCrypt volumes when checking the “TrueCrypt Mode” checkbox. This is why in my udisks patches I used “VeraCrypt/TrueCrypt” in the user visible strings (visible in Disks when successfully unlocked a VeraCrypt/TrueCrypt volume, see screenshot).

What do you think?

#11 Updated by segfault 2018-02-12 15:56:20

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

I fixed some issues and, as suggested by sajolida, added a label to explain that the volume might be a VeraCrypt volume. This way we don’t have to change the title at all. We set the label to This volume might be a VeraCrypt volume as it contains random data. Alternative labels we also thought would be ok are: This volume might be a VeraCrypt volume (without further explanation), and If this volume is a VeraCrypt volume, enter your passphrase.

@sajolida: I forgot this when we talked about it, but I am not sure whether it is a good idea to completely drop the “TrueCrypt” part from the explanation, for the reasons I explained in comment #9.

I also found a bug in cryptsetup which sometimes causes TCRYPT volumes to fail to unlock (@anonym: this is the reason for the bug I told you about). I reported it here.

#12 Updated by anonym 2018-02-19 13:24:21

• Status changed from Confirmed to In Progress
• Assignee changed from anonym to segfault
• QA Check deleted (Ready for QA)

LGTM up to commit:2dd14c1ca90e807eaf95bfa3baeffe792c76a04a.

(I personally do not like to depend on the default values in the .ui file (except for strings), e.g. to not show the expander when we know we are unlocking a LUKS device. But I have no clear argument for doing the alternative (e.g. setting the values explicitly in the code) so I’ll shut up instead.)

#16 Updated by bertagaz 2018-03-14 11:32:21

• Target version changed from Tails_3.6 to Tails_3.7

#17 Updated by segfault 2018-03-30 10:48:54

• Status changed from In Progress to Resolved
• % Done changed from 70 to 100

The code is written, now we “only” need to upstream it (Feature #15221)