Skip to content

Set set-user-ID for clevis-luks-udisks2#45

Closed
martinezjavier wants to merge 1 commit intolatchset:masterfrom
martinezjavier:clevis-udisks2-setuid
Closed

Set set-user-ID for clevis-luks-udisks2#45
martinezjavier wants to merge 1 commit intolatchset:masterfrom
martinezjavier:clevis-udisks2-setuid

Conversation

@martinezjavier
Copy link
Copy Markdown
Contributor

The clevis-luks-udisks2 program expects to be run as root. But since it's
executed from a Desktop Application Autostart file, it's run by the user
that started the session. So in order to allow it to be run as root, set
the SETUID bit to the executable during the make install target.

This is safe since clevis-luks-udisks2 drops its privileges, and sets the
process credentials to the clevis user for key recovering and to the user
that executed the binary for the unlocking. So the root privilege is only
used to read the LUKS metadata from the encrypted volume.

Fixes: #28

Signed-off-by: Javier Martinez Canillas javierm@redhat.com

@martinezjavier martinezjavier force-pushed the clevis-udisks2-setuid branch from b118e6c to 4711181 Compare April 12, 2018 13:45
The clevis-luks-udisks2 program expects to be run as root. But since it's
executed from a Desktop Application Autostart file, it's run by the user
that started the session. So in order to allow it to be run as root, set
the SUID bit to the executable during the make install target.

This is safe since clevis-luks-udisks2 drops its privileges, and sets the
process credentials to the clevis user for key recovering and to the user
that executed the binary for the unlocking. So the root privilege is only
used to read the LUKS metadata from the encrypted volume.

Fixes: latchset#28

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
@martinezjavier martinezjavier force-pushed the clevis-udisks2-setuid branch from 4711181 to 6842776 Compare April 13, 2018 10:37
@npmccallum
Copy link
Copy Markdown
Contributor

I didn't do this originally because I wasn't sure of the implications of doing it. For example, what if someone is installing the package as non-root in a home directory. I'm probably going to merge it anyway. But have you given any thought to this?

@martinezjavier
Copy link
Copy Markdown
Contributor Author

@npmccallum the only thought I've given is what I've mentioned in the commit message about the privilege being drop for most operations but yes, it's always scary to have binaries with the SUID bit set...

Do you have other idea on how we could fix issue #28 ?

@martinezjavier
Copy link
Copy Markdown
Contributor Author

@npmccallum another option is to don't do this on make install but instead leave to each distribution to set the SUID on package install (i.e: on a %posttran scriptlet in Fedora).

@martinezjavier
Copy link
Copy Markdown
Contributor Author

Actually, I think we shouldn't even require this to be run as root and instead run it as the clevis user and add it to the disk group so it can read the block devices.

@martinezjavier
Copy link
Copy Markdown
Contributor Author

@npmccallum have you given more thought about this issue? I've been asked many times about automatic unlocking in the user session for encrypted removable devices, so this is something that users are very interested to get fixed.

@npmccallum
Copy link
Copy Markdown
Contributor

@martinezjavier I'm confused. What's broken? Distributions already set the suid bit in their packages.

We can't run the whole thing as the clevis user because the DBus calls have to come from the desktop session user. So we still need at least 2 users (1 performs key recovery, 1 does unlocking). But I would be thrilled to drop suid = root and do suid = clevis.

@martinezjavier
Copy link
Copy Markdown
Contributor Author

@npmccallum now I'm the one that's confused. I was pretty sure that I tracked down #28 to clevis-luks-udisks2 not having the suid bit set and it worked for me after setting it.

But I see that the Fedora package does set it indeed. So I'll close this PR then if is up to each distro to do this at the package level.

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.

2 participants