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.

Sign up for GitHub

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

Jump to bottom

[feature] Add securemode, verifySIDPassword #408

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

[feature] Add securemode, verifySIDPassword #408

wants to merge 3 commits into from

Commits on Aug 17, 2022

  1. Shuffle GetPassPhrase{.h, .cpp} in makefile.am 

    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]>
    Howard McLauchlan committed Aug 17, 2022
    Configuration menu
    Copy the full SHA
    b55cc39 View commit details
    Browse the repository at this point in the history

  2. Add securemode/password-setting 

    *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]>
    Howard McLauchlan committed Aug 17, 2022
    Configuration menu
    Copy the full SHA
    fc3c2d3 View commit details
    Browse the repository at this point in the history

  3. Add verifySIDPassword 

    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]>
    Howard McLauchlan committed Aug 17, 2022
    Configuration menu
    Copy the full SHA
    e4452be View commit details
    Browse the repository at this point in the history