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

luks2: Take digest length to be compared from expected digest #12

Merged
merged 1 commit into from
May 7, 2024

Conversation

nmeum
Copy link
Contributor

@nmeum nmeum commented Dec 23, 2023

LUKS1 was limited to a 20 byte digest length. With LUKS2 this limitation was lifted. However, it seems that for LUKS volumes which were converted from LUKS1 to LUKS2, the digest length it still 20 bytes. This commit proposes using the length of the expected digest to determine the digit length in use. For this purpose, it only compares the first n bytes of the generated digest with the expected digest.

This fixes compatibility of luks.go with volumes converted from LUKS1.

Fixes: #11

See also: anatol/booster#202


Note that I only briefly skimmed over the LUKS format specification, hence I am no expert and don't know if there is a better way to obtain the desired digest length. I did, however, confirm that this does fix opening converted LUKS2 volumes.

LUKS1 was limited to a 20 byte digest length. With LUKS2 this limitation
was lifted. However, it seems that for LUKS volumes which were converted
from LUKS1 to LUKS2, the digest length it still 20 bytes. This commit
proposes using the length of the expected digest to determine the digit
length in use. For this purpose, it only compares the first n bytes of
the generated digest with the expected digest.

This fixes compatibility of luks.go with volumes converted from LUKS1.

Fixes: anatol#11

See also: anatol/booster#202
@nmeum
Copy link
Contributor Author

nmeum commented May 5, 2024

@anatol any chance you could take a look at this?

We have this backported for booster in Alpine for a while without any complaints so far.

@anatol
Copy link
Owner

anatol commented May 6, 2024

Thank you for finding the root cause for the problem. Yes #11 needs to be fixed.

A few questions wrt this patch:

  • could it cause a security issue?
  • are there additional ways to check that the volume is LUKS1->LUKS2 converted, so we can add the checks to the codebase
  • this functionality needs a test case similar to this one. I assume the way to convert a partition is to use cryptsetup convert --type luks2 /dev/$dev?

@nmeum
Copy link
Contributor Author

nmeum commented May 6, 2024

could it cause a security issue?

I don't think so, as far as I can tell, the if statement mostly results in a nicer error message to be emitted during a failed unlock.

are there additional ways to check that the volume is LUKS1->LUKS2 converted, so we can add the checks to the codebase

Not sure, the LUKS specification is a bit lacking in that respect.

Could poke at the cryptsetup implementation a bit more but IIRC they also just check the digest length.

this functionality needs a test case similar to this one. I assume the way to convert a partition is to use cryptsetup convert --type luks2 /dev/$dev?

Yep, see the instructions for reproducing this issue provided in #11.

@anatol anatol merged commit f656928 into anatol:master May 7, 2024
@anatol
Copy link
Owner

anatol commented May 7, 2024

Looks good then, I'll add tests for this functionality.

Could poke at the cryptsetup implementation

If you can provide the line where such comparison happens in the upstream code then it would be worthwhile to mention it in the luks.go codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to open LUKS2 device converted from LUKS1
2 participants