Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for VeraCrypt volumes and extend support for TCRYPT volumes #320

Merged
merged 11 commits into from Mar 20, 2018

Conversation

@segfault3
Copy link
Contributor

segfault3 commented Feb 10, 2018

This adds support for:

  • VeraCrypt volumes
  • TCRYPT keyfiles
  • TCRYPT hidden containers
  • TCRYPT system volumes
  • VeraCrypt PIM
@segfault3 segfault3 mentioned this pull request Feb 10, 2018
@segfault3

This comment has been minimized.

Copy link
Contributor Author

segfault3 commented Feb 10, 2018

The corresponding udisks pull request: storaged-project/udisks#495.

@vojtechtrefny

This comment has been minimized.

Copy link
Member

vojtechtrefny commented Feb 28, 2018

This is unfortunately an API change and we can't do that in minor version -- if you need the new options for bd_crypto_tc_open, you'll need to create a new function -- similar to bd_crypto_luks_open and bd_crypto_luks_open_blob.

@segfault3 segfault3 force-pushed the segfault3:add_veracrypt branch from 06ff56f to 074814d Feb 28, 2018
@segfault3

This comment has been minimized.

Copy link
Contributor Author

segfault3 commented Feb 28, 2018

I see. I moved the new code into a new function in 074814d.

@vojtechtrefny

This comment has been minimized.

Copy link
Member

vojtechtrefny commented Mar 8, 2018

Thank you. Maybe bd_crypto_tc_open2 is not the best name for the new function, but I don't have better idea, so I guess it's ok.

@vojtechtrefny

This comment has been minimized.

Copy link
Member

vojtechtrefny commented Mar 8, 2018

Jenkins, test this please.

@vojtechtrefny

This comment has been minimized.

Copy link
Member

vojtechtrefny commented Mar 9, 2018

The veracrypt_pim parameter is available only in cryptsetup 2, please add some checks/ifdefs for this parameter so it works on older version of cryptsetup too.

@TobiPeterG

This comment has been minimized.

Copy link

TobiPeterG commented Mar 9, 2018

How can I merge your changes on my system?

Copy link
Contributor

vpodzime left a comment

First of all -- THANKS a lot for these changes! There are a couple small things that can be made better, but other than that the changes are nice.

It would be nice if these changes didn't break git bisect. IOW, if the API wasn't broken even in the middle of the changes. @segfault3 please try to do that. If the rebase gets ridiculously complicated, feel free to give it up, though. :)

@@ -309,6 +309,27 @@ gboolean bd_crypto_luks_resize (const gchar *luks_device, guint64 size, GError *
*/
gboolean bd_crypto_tc_open (const gchar *device, const gchar *name, const guint8* pass_data, gsize data_len, gboolean read_only, GError **error);

/**
* bd_crypto_tc_open2:

This comment has been minimized.

Copy link
@vpodzime

vpodzime Mar 9, 2018

Contributor

I suggest bd_crypto_tc_open_full as a better name for this function.

This comment has been minimized.

Copy link
@segfault3

segfault3 Mar 9, 2018

Author Contributor

Done in 94aca5c

This comment has been minimized.

Copy link
@segfault3

segfault3 Mar 9, 2018

Author Contributor

I squashed commit 94aca5c into 9a7f054 during the rebasing.

*
* Tech category: %BD_CRYPTO_TECH_TRUECRYPT-%BD_CRYPTO_TECH_MODE_OPEN_CLOSE
*/
gboolean bd_crypto_tc_open (const gchar *device, const gchar *name, const guint8* pass_data, gsize data_len, gboolean read_only, GError **error) {

This comment has been minimized.

Copy link
@vpodzime

vpodzime Mar 9, 2018

Contributor

I think the old function should just call the new more universal one.

This comment has been minimized.

Copy link
@segfault3

segfault3 Mar 9, 2018

Author Contributor

Makes sense. Done in 12841cf.

This comment has been minimized.

Copy link
@segfault3

segfault3 Mar 9, 2018

Author Contributor

I also squahed 12841cf into 9a7f054 during the rebasing.

@@ -959,14 +959,16 @@ gboolean bd_crypto_luks_resize (const gchar *luks_device, guint64 size, GError *
* @pass_data: (array length=data_len): a passphrase for the TrueCrypt/VeraCrypt volume (may contain arbitrary binary data)
* @data_len: length of the @pass_data buffer
* @read_only: whether to open as read-only or not (meaning read-write)
* @keyfiles: (array length=keyfiles_count) paths to the keyfiles for the TrueCrypt/VeraCrypt volume
* @keyfiles_count: length of the @keyfiles array

This comment has been minimized.

Copy link
@vpodzime

vpodzime Mar 9, 2018

Contributor

We do zero/NULL terminated arrays where possible.

This comment has been minimized.

Copy link
@segfault3

segfault3 Mar 9, 2018

Author Contributor

Fixed in 8f0f434.

This comment has been minimized.

Copy link
@segfault3

segfault3 Mar 9, 2018

Author Contributor

That is now done in commit 69a3223 after the rebasing.

@segfault3 segfault3 force-pushed the segfault3:add_veracrypt branch from 8f0f434 to 69a3223 Mar 9, 2018
@segfault3

This comment has been minimized.

Copy link
Contributor Author

segfault3 commented Mar 9, 2018

The veracrypt_pim parameter is available only in cryptsetup 2, please add some checks/ifdefs for this parameter so it works on older version of cryptsetup too.

Done in 08a09b7.

@segfault3

This comment has been minimized.

Copy link
Contributor Author

segfault3 commented Mar 9, 2018

First of all -- THANKS a lot for these changes!

I'm glad it's appreciated :)

There are a couple small things that can be made better, but other than that the changes are nice.

I think I addressed all of them now.

It would be nice if these changes didn't break git bisect. IOW, if the API wasn't broken even in the middle of the changes. @segfault3 please try to do that. If the rebase gets ridiculously complicated, feel free to give it up, though. :)

I rewrote git history and think that now none of the commit breaks the API. I squashed some commits to make the rebasing easier.

@vojtechtrefny

This comment has been minimized.

Copy link
Member

vojtechtrefny commented Mar 10, 2018

Jenkins, ok to test.

@@ -6,6 +6,10 @@

#define BD_CRYPTO_LUKS_METADATA_SIZE (2 MiB)

#define BD_CRYPTO_CHI_SQUARE_LOWER_LIMIT 136
#define BD_CRYPTO_CHI_SQUARE_UPPER_LIMIT 426
#define BD_CRYPTO_CHI_SQUARE_BYTES_TO_CHECK 512

This comment has been minimized.

Copy link
@vojtechtrefny

vojtechtrefny Mar 12, 2018

Member

This looks like something that might be better as parameters for bd_crypto_device_seems_encrypted. I don't know if there is a change that these will change in feature, but it might be better to not have to recompile libblockdev just because you want to change these limits.

This comment has been minimized.

Copy link
@segfault3

segfault3 Mar 12, 2018

Author Contributor

Mmh, I don't really see the use case for changing these values - especially not in the higher level applications which will use this function. Why do you think they should be changeable without recompiling?

This comment has been minimized.

Copy link
@vojtechtrefny

vojtechtrefny Mar 13, 2018

Member

I don't know, but the limits just look like something that users of libblockdev might want to change eventually. But if you think this isn't something that makes sense to change from e.g. udisks, we can keep it this way.

This comment has been minimized.

Copy link
@segfault3

segfault3 Mar 13, 2018

Author Contributor

I don't see why these values should be changed from udisks. We explained at https://tails.boum.org/blueprint/veracrypt/#detection why, with these values, the chi-squared test should only produce false negatives with negligible probability (1 in 10 billion). And devices with (non-encrypted) filesystem headers will never pass the test. The only reason I can think of to change these values is to handle strange filesystems, which don't have their header at least partly in the first 512 Bytes (I don't know of any such filesystem), but then I think we should handle this in libblockdev and not udisks or similar.

This comment has been minimized.

Copy link
@vojtechtrefny

vojtechtrefny Mar 14, 2018

Member

Ok, you convinced me, we can keep it this way.

Copy link
Member

vojtechtrefny left a comment

Looks good to me now.

*
* Tech category: %BD_CRYPTO_TECH_TRUECRYPT-%BD_CRYPTO_TECH_MODE_QUERY
*/
gboolean bd_crypto_device_seems_encrypted (const gchar *device, GError **error);

This comment has been minimized.

Copy link
@vojtechtrefny

vojtechtrefny Mar 14, 2018

Member

Please add the new functions to docs/libblockdev-sections.txt too.

This comment has been minimized.

Copy link
@segfault3

segfault3 Mar 14, 2018

Author Contributor

Done in b8d8bd7.

@segfault3

This comment has been minimized.

Copy link
Contributor Author

segfault3 commented Mar 14, 2018

I fixed a URL in 2d15e2d.

Copy link
Contributor

vpodzime left a comment

Thanks a ton @segfault3!

@vojtechtrefny vojtechtrefny merged commit ff922e1 into storaged-project:master Mar 20, 2018
19 of 21 checks passed
19 of 21 checks passed
Complete Jenkins Job (private) Build finished.
Details
ci/i686/f_rawhide Build and tests finished: failure
Details
All RPM builds (private) Build finished.
Details
All csmock tests (private) Build finished.
Details
ci/i686/debian_9 Build and tests finished: success
Details
ci/i686/f_27 Build and tests finished: success
Details
ci/i686/f_28 Build and tests finished: success
Details
ci/x86_64/centos_7 Build and tests finished: success
Details
ci/x86_64/debian_9 Build and tests finished: success
Details
ci/x86_64/f_27 Build and tests finished: success
Details
ci/x86_64/f_28 Build and tests finished: success
Details
ci/x86_64/f_rawhide Build and tests finished: success
Details
csmock/i686 csmock finished: success
Details
csmock/x86_64 csmock finished: success
Details
rpms/i686/f_27 RPM build finished: success
Details
rpms/i686/f_28 RPM build finished: success
Details
rpms/i686/f_rawhide RPM build finished: success
Details
rpms/x86_64/epel_7 RPM build finished: success
Details
rpms/x86_64/f_27 RPM build finished: success
Details
rpms/x86_64/f_28 RPM build finished: success
Details
rpms/x86_64/f_rawhide RPM build finished: success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.