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

[feature] Add securemode, verifySIDPassword #408

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hMcLauchlan
Copy link

@hMcLauchlan hMcLauchlan commented Aug 18, 2022

Hello!

Thank you for working on this utility. Please find in this PR some patches which we've carried on top of master to enable our use cases (particularly around setting passwords on SEDs). They should be generic enough to be mainline'd, but I'm happy to make changes as appropriate.

This PR does two main things:

  • Fixup and modify (where appropriate) Feature/secure mode #271, bringing it up-to-date with master.
  • Add a utility verifySIDPassword, which does what it sounds like it does.

Please see commit messages for more details. Thanks!

Edit: Forgot to post my test plan:

[root@host~]# ./sedutil-cli -s -v -n --setSIDPassword /dev/nvme0n1
Log level set to DBG 
sedutil version : 
No password hashing incompatible with secure mode
Disabling password hashing in secure mode (-s)


Please enter password ***********************************Unknown Feature in Discovery 0 response 402
Unknown Feature in Discovery 0 response 402
Performing setSIDPassword 


Please enter the new password ***

Please confirm the new password ***   
[root@host ~]# ./sedutil-cli -s -v -n --verifySIDPassword /dev/nvme0
Log level set to DBG 
sedutil version : 
No password hashing incompatible with secure mode
Disabling password hashing in secure mode (-s)


Please enter password ***Unknown Feature in Discovery 0 response 402
Unknown Feature in Discovery 0 response 402
Performing verifySIDPassword 
Successfully verified SIDPassword
[root@host ~]# ./sedutil-cli -s -v -n --setSIDPassword /dev/nvme0n1 
Log level set to DBG 
sedutil version : 
No password hashing incompatible with secure mode
Disabling password hashing in secure mode (-s)


Please enter password ***Unknown Feature in Discovery 0 response 402
Unknown Feature in Discovery 0 response 402
Performing setSIDPassword 


Please enter the new password ***********************************

Please confirm the new password ***********************************
[root@host~]# ./sedutil-cli -s -v -n --verifySIDPassword /dev/nvme0
Log level set to DBG 
sedutil version : 
No password hashing incompatible with secure mode
Disabling password hashing in secure mode (-s)


Please enter password ***********************************Unknown Feature in Discovery 0 response 402
Unknown Feature in Discovery 0 response 402
Performing verifySIDPassword 
Successfully verified SIDPassword

Howard McLauchlan added 3 commits August 16, 2022 22:40
Moving GetPassPhrase.o down into SEDUTIL_LINUX_CODE, since we need it
for the securemode/password-setting work.

This is fine since AFAICT linuxpba_SOURCES eventually gets consumed in
conjunction with SEDUTIL_LINUX_CODE, so that binary will get access to
the TU.

Keeping this as seperate commit since it might be a little bit subtle /
weird, depending on what Make is doing behind the scenes.

Signed-off-by: Howard McLauchlan <[email protected]>
*BASICALLY CLEANED UP VERSION OF Drive-Trust-Alliance#271

This commit does what the linked PR says, and also fixes a few bugs in
that original PR. I'm not sure what the right way to give credit is and
it was very painful to resurrect CVE's original patches and roll my own
on top, so the disclaimer here is that it's like 95% his code :).

A few notable things:
* We don't need to modify the makefiles, since we split that out in the
  prior commit.
* We fixed his original makefile, which didn't quite work: that change
  is folded naturally into prior commit.
* The generated makefiles don't need to change, because since CVE's
  original patchset, GetPassPhrase.o was introduced organically to the
  codebase, and ergo the makefiles.

The most interesting thing here is we allow hashing to be forced off by
`-n` even during secure mode.

The key issue we ran into was that if a drive is originally set with no
hashing, then hash'd invocations in the future will fail(obviously). As
implemented, CVE's original patches will silently debug output, and then
turn on hashing without telling the user.

Not a domain expert in why hashing is necessary here, but in either
case, I think we should support the case where a password was originally
set without hashing, by allowing hashing to be turned off _if_ specified
explicitly.

We also do some sneaky business by ensuring -n is evaluated after -s, so
-n will always override -s, if provided.

Signed-off-by: Howard McLauchlan <[email protected]>
This is a utility I added to aid debugging, but generally seems like a
good idea. Before adding this, the only way we had of checking "do i
have the right password" was to try to change it, which does seem a
little weird. The pattern of change_password -> verify_password seems a
little more sane to me.

The implementation of this command is a little scuffed though. Under the
hood, we attempt to start a session with the passed in password. There
are many reasons for a session start to fail, and verifySIDPassword
faithfully returns them. On success, however, the password must be
correct.

Signed-off-by: Howard McLauchlan <[email protected]>
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.

1 participant