This is mainly about /dev/sev and its default permissions 0600. Of
course, rule of 'tinfoil' would be that we can't trust anything, but the
probing code in QEMU is considered safe from security's perspective + we
can't create an udev rule for this at the moment, because ioctls and
filesystem permisions are cross checked in kernel and therefore a user
with read permisions could issue a 'privileged' operation on SEV which
is currently only limited to root.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
src/qemu/qemu_capabilities.c | 11 +++++++++++
src/util/virutil.c | 31 +++++++++++++++++++++++++++++--
2 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 5cf4b617c6..2e84c965e8 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -53,6 +53,10 @@
#include <stdarg.h>
#include <sys/utsname.h>
+#if WITH_CAPNG
+# include <cap-ng.h>
+#endif
+
#define VIR_FROM_THIS VIR_FROM_QEMU
VIR_LOG_INIT("qemu.qemu_capabilities");
@@ -4515,6 +4519,13 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
NULL);
virCommandAddEnvPassCommon(cmd->cmd);
virCommandClearCaps(cmd->cmd);
+
+#if WITH_CAPNG
+ /* QEMU might run into permission issues, e.g. /dev/sev (0600), override
+ * them just for the purpose of probing */
+ virCommandAllowCap(cmd->cmd, CAP_DAC_OVERRIDE);
+#endif
+
virCommandSetGID(cmd->cmd, cmd->runGid);
virCommandSetUID(cmd->cmd, cmd->runUid);
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 5251b66454..02de92061c 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1502,8 +1502,10 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
{
size_t i;
int capng_ret, ret = -1;
- bool need_setgid = false, need_setuid = false;
+ bool need_setgid = false;
+ bool need_setuid = false;
bool need_setpcap = false;
+ const char *capstr = NULL;
/* First drop all caps (unless the requested uid is "unchanged" or
* root and clearExistingCaps wasn't requested), then add back
@@ -1512,14 +1514,18 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
*/
if (clearExistingCaps || (uid != (uid_t)-1 && uid != 0))
- capng_clear(CAPNG_SELECT_BOTH);
+ capng_clear(CAPNG_SELECT_BOTH);
for (i = 0; i <= CAP_LAST_CAP; i++) {
+ capstr = capng_capability_to_name(i);
+
if (capBits & (1ULL << i)) {
capng_update(CAPNG_ADD,
CAPNG_EFFECTIVE|CAPNG_INHERITABLE|
CAPNG_PERMITTED|CAPNG_BOUNDING_SET,
i);
+
+ VIR_DEBUG("Added '%s' to child capabilities' set", capstr);
}
}
@@ -1579,6 +1585,27 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
goto cleanup;
}
+# ifdef PR_CAP_AMBIENT
+ /* we couldn't do this in the loop earlier above, because the capabilities
+ * were not applied yet, since in order to add a capability into the AMBIENT
+ * set, it has to be present in both the PERMITTED and INHERITABLE sets
+ * (capabilities(7))
+ */
+ for (i = 0; i <= CAP_LAST_CAP; i++) {
+ capstr = capng_capability_to_name(i);
+
+ if (capBits & (1ULL << i)) {
+ if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, i, 0, 0) < 0) {
+ virReportSystemError(errno,
+ _("prctl failed to enable '%s' in the "
+ "AMBIENT set"),
+ capstr);
+ goto cleanup;
+ }
+ }
+ }
+# endif
+
/* Set bounding set while we have CAP_SETPCAP. Unfortunately we cannot
* do this if we failed to get the capability above, so ignore the
* return value.
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jan 31, 2019 at 04:26:18PM +0100, Erik Skultety wrote:
> This is mainly about /dev/sev and its default permissions 0600. Of
> course, rule of 'tinfoil' would be that we can't trust anything, but the
> probing code in QEMU is considered safe from security's perspective + we
> can't create an udev rule for this at the moment, because ioctls and
> filesystem permisions are cross checked in kernel and therefore a user
> with read permisions could issue a 'privileged' operation on SEV which
> is currently only limited to root.
>
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
> src/qemu/qemu_capabilities.c | 11 +++++++++++
> src/util/virutil.c | 31 +++++++++++++++++++++++++++++--
> 2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 5cf4b617c6..2e84c965e8 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -53,6 +53,10 @@
> #include <stdarg.h>
> #include <sys/utsname.h>
>
> +#if WITH_CAPNG
> +# include <cap-ng.h>
> +#endif
> +
> #define VIR_FROM_THIS VIR_FROM_QEMU
>
> VIR_LOG_INIT("qemu.qemu_capabilities");
> @@ -4515,6 +4519,13 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
> NULL);
> virCommandAddEnvPassCommon(cmd->cmd);
> virCommandClearCaps(cmd->cmd);
> +
> +#if WITH_CAPNG
> + /* QEMU might run into permission issues, e.g. /dev/sev (0600), override
> + * them just for the purpose of probing */
> + virCommandAllowCap(cmd->cmd, CAP_DAC_OVERRIDE);
> +#endif
> +
> virCommandSetGID(cmd->cmd, cmd->runGid);
> virCommandSetUID(cmd->cmd, cmd->runUid);
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 5251b66454..02de92061c 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1502,8 +1502,10 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
> {
> size_t i;
> int capng_ret, ret = -1;
> - bool need_setgid = false, need_setuid = false;
> + bool need_setgid = false;
> + bool need_setuid = false;
> bool need_setpcap = false;
> + const char *capstr = NULL;
>
> /* First drop all caps (unless the requested uid is "unchanged" or
> * root and clearExistingCaps wasn't requested), then add back
> @@ -1512,14 +1514,18 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
> */
>
> if (clearExistingCaps || (uid != (uid_t)-1 && uid != 0))
> - capng_clear(CAPNG_SELECT_BOTH);
> + capng_clear(CAPNG_SELECT_BOTH);
>
> for (i = 0; i <= CAP_LAST_CAP; i++) {
> + capstr = capng_capability_to_name(i);
> +
> if (capBits & (1ULL << i)) {
> capng_update(CAPNG_ADD,
> CAPNG_EFFECTIVE|CAPNG_INHERITABLE|
> CAPNG_PERMITTED|CAPNG_BOUNDING_SET,
> i);
> +
> + VIR_DEBUG("Added '%s' to child capabilities' set", capstr);
> }
> }
>
> @@ -1579,6 +1585,27 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
> goto cleanup;
> }
>
> +# ifdef PR_CAP_AMBIENT
> + /* we couldn't do this in the loop earlier above, because the capabilities
> + * were not applied yet, since in order to add a capability into the AMBIENT
> + * set, it has to be present in both the PERMITTED and INHERITABLE sets
> + * (capabilities(7))
> + */
> + for (i = 0; i <= CAP_LAST_CAP; i++) {
> + capstr = capng_capability_to_name(i);
> +
> + if (capBits & (1ULL << i)) {
> + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, i, 0, 0) < 0) {
> + virReportSystemError(errno,
> + _("prctl failed to enable '%s' in the "
> + "AMBIENT set"),
> + capstr);
> + goto cleanup;
> + }
> + }
> + }
> +# endif
This is set a bit earlier than I set it in my PoC patch, but I'll assume
it still works given the comment you added.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Feb 01, 2019 at 10:31:52AM +0000, Daniel P. Berrangé wrote:
> On Thu, Jan 31, 2019 at 04:26:18PM +0100, Erik Skultety wrote:
> > This is mainly about /dev/sev and its default permissions 0600. Of
> > course, rule of 'tinfoil' would be that we can't trust anything, but the
> > probing code in QEMU is considered safe from security's perspective + we
> > can't create an udev rule for this at the moment, because ioctls and
> > filesystem permisions are cross checked in kernel and therefore a user
> > with read permisions could issue a 'privileged' operation on SEV which
> > is currently only limited to root.
> >
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> > src/qemu/qemu_capabilities.c | 11 +++++++++++
> > src/util/virutil.c | 31 +++++++++++++++++++++++++++++--
> > 2 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 5cf4b617c6..2e84c965e8 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -53,6 +53,10 @@
> > #include <stdarg.h>
> > #include <sys/utsname.h>
> >
> > +#if WITH_CAPNG
> > +# include <cap-ng.h>
> > +#endif
> > +
> > #define VIR_FROM_THIS VIR_FROM_QEMU
> >
> > VIR_LOG_INIT("qemu.qemu_capabilities");
> > @@ -4515,6 +4519,13 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
> > NULL);
> > virCommandAddEnvPassCommon(cmd->cmd);
> > virCommandClearCaps(cmd->cmd);
> > +
> > +#if WITH_CAPNG
> > + /* QEMU might run into permission issues, e.g. /dev/sev (0600), override
> > + * them just for the purpose of probing */
> > + virCommandAllowCap(cmd->cmd, CAP_DAC_OVERRIDE);
> > +#endif
> > +
> > virCommandSetGID(cmd->cmd, cmd->runGid);
> > virCommandSetUID(cmd->cmd, cmd->runUid);
> >
> > diff --git a/src/util/virutil.c b/src/util/virutil.c
> > index 5251b66454..02de92061c 100644
> > --- a/src/util/virutil.c
> > +++ b/src/util/virutil.c
> > @@ -1502,8 +1502,10 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
> > {
> > size_t i;
> > int capng_ret, ret = -1;
> > - bool need_setgid = false, need_setuid = false;
> > + bool need_setgid = false;
> > + bool need_setuid = false;
> > bool need_setpcap = false;
> > + const char *capstr = NULL;
> >
> > /* First drop all caps (unless the requested uid is "unchanged" or
> > * root and clearExistingCaps wasn't requested), then add back
> > @@ -1512,14 +1514,18 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
> > */
> >
> > if (clearExistingCaps || (uid != (uid_t)-1 && uid != 0))
> > - capng_clear(CAPNG_SELECT_BOTH);
> > + capng_clear(CAPNG_SELECT_BOTH);
> >
> > for (i = 0; i <= CAP_LAST_CAP; i++) {
> > + capstr = capng_capability_to_name(i);
> > +
> > if (capBits & (1ULL << i)) {
> > capng_update(CAPNG_ADD,
> > CAPNG_EFFECTIVE|CAPNG_INHERITABLE|
> > CAPNG_PERMITTED|CAPNG_BOUNDING_SET,
> > i);
> > +
> > + VIR_DEBUG("Added '%s' to child capabilities' set", capstr);
> > }
> > }
> >
> > @@ -1579,6 +1585,27 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
> > goto cleanup;
> > }
> >
> > +# ifdef PR_CAP_AMBIENT
> > + /* we couldn't do this in the loop earlier above, because the capabilities
> > + * were not applied yet, since in order to add a capability into the AMBIENT
> > + * set, it has to be present in both the PERMITTED and INHERITABLE sets
> > + * (capabilities(7))
> > + */
> > + for (i = 0; i <= CAP_LAST_CAP; i++) {
> > + capstr = capng_capability_to_name(i);
> > +
> > + if (capBits & (1ULL << i)) {
> > + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, i, 0, 0) < 0) {
> > + virReportSystemError(errno,
> > + _("prctl failed to enable '%s' in the "
> > + "AMBIENT set"),
> > + capstr);
> > + goto cleanup;
> > + }
> > + }
> > + }
> > +# endif
>
> This is set a bit earlier than I set it in my PoC patch, but I'll assume
> it still works given the comment you added.
I was trying to understand whether there was a particular reason why you added
it to the ambient set later, so my first lame attempt was to merge the 2 'for'
loops into 1, since they were identical apart from the prctl syscall which led
to an error. So I investigated and found the restriction I mentioned in the
comment so I moved it after the caps were first applied and it did work:
(trial-error)+-research method™
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Thanks,
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Feb 01, 2019 at 12:33:19PM +0100, Erik Skultety wrote:
> On Fri, Feb 01, 2019 at 10:31:52AM +0000, Daniel P. Berrangé wrote:
> > On Thu, Jan 31, 2019 at 04:26:18PM +0100, Erik Skultety wrote:
> > > This is mainly about /dev/sev and its default permissions 0600. Of
> > > course, rule of 'tinfoil' would be that we can't trust anything, but the
> > > probing code in QEMU is considered safe from security's perspective + we
> > > can't create an udev rule for this at the moment, because ioctls and
> > > filesystem permisions are cross checked in kernel and therefore a user
> > > with read permisions could issue a 'privileged' operation on SEV which
> > > is currently only limited to root.
> > >
> > > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > > ---
> > > src/qemu/qemu_capabilities.c | 11 +++++++++++
> > > src/util/virutil.c | 31 +++++++++++++++++++++++++++++--
> > > 2 files changed, 40 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > > index 5cf4b617c6..2e84c965e8 100644
> > > --- a/src/qemu/qemu_capabilities.c
> > > +++ b/src/qemu/qemu_capabilities.c
> > > @@ -53,6 +53,10 @@
> > > #include <stdarg.h>
> > > #include <sys/utsname.h>
> > >
> > > +#if WITH_CAPNG
> > > +# include <cap-ng.h>
> > > +#endif
> > > +
> > > #define VIR_FROM_THIS VIR_FROM_QEMU
> > >
> > > VIR_LOG_INIT("qemu.qemu_capabilities");
> > > @@ -4515,6 +4519,13 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd,
> > > NULL);
> > > virCommandAddEnvPassCommon(cmd->cmd);
> > > virCommandClearCaps(cmd->cmd);
> > > +
> > > +#if WITH_CAPNG
> > > + /* QEMU might run into permission issues, e.g. /dev/sev (0600), override
> > > + * them just for the purpose of probing */
> > > + virCommandAllowCap(cmd->cmd, CAP_DAC_OVERRIDE);
> > > +#endif
> > > +
> > > virCommandSetGID(cmd->cmd, cmd->runGid);
> > > virCommandSetUID(cmd->cmd, cmd->runUid);
> > >
> > > diff --git a/src/util/virutil.c b/src/util/virutil.c
> > > index 5251b66454..02de92061c 100644
> > > --- a/src/util/virutil.c
> > > +++ b/src/util/virutil.c
> > > @@ -1502,8 +1502,10 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
> > > {
> > > size_t i;
> > > int capng_ret, ret = -1;
> > > - bool need_setgid = false, need_setuid = false;
> > > + bool need_setgid = false;
> > > + bool need_setuid = false;
> > > bool need_setpcap = false;
> > > + const char *capstr = NULL;
> > >
> > > /* First drop all caps (unless the requested uid is "unchanged" or
> > > * root and clearExistingCaps wasn't requested), then add back
> > > @@ -1512,14 +1514,18 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
> > > */
> > >
> > > if (clearExistingCaps || (uid != (uid_t)-1 && uid != 0))
> > > - capng_clear(CAPNG_SELECT_BOTH);
> > > + capng_clear(CAPNG_SELECT_BOTH);
> > >
> > > for (i = 0; i <= CAP_LAST_CAP; i++) {
> > > + capstr = capng_capability_to_name(i);
> > > +
> > > if (capBits & (1ULL << i)) {
> > > capng_update(CAPNG_ADD,
> > > CAPNG_EFFECTIVE|CAPNG_INHERITABLE|
> > > CAPNG_PERMITTED|CAPNG_BOUNDING_SET,
> > > i);
> > > +
> > > + VIR_DEBUG("Added '%s' to child capabilities' set", capstr);
> > > }
> > > }
> > >
> > > @@ -1579,6 +1585,27 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups,
> > > goto cleanup;
> > > }
> > >
> > > +# ifdef PR_CAP_AMBIENT
> > > + /* we couldn't do this in the loop earlier above, because the capabilities
> > > + * were not applied yet, since in order to add a capability into the AMBIENT
> > > + * set, it has to be present in both the PERMITTED and INHERITABLE sets
> > > + * (capabilities(7))
> > > + */
> > > + for (i = 0; i <= CAP_LAST_CAP; i++) {
> > > + capstr = capng_capability_to_name(i);
> > > +
> > > + if (capBits & (1ULL << i)) {
> > > + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, i, 0, 0) < 0) {
> > > + virReportSystemError(errno,
> > > + _("prctl failed to enable '%s' in the "
> > > + "AMBIENT set"),
> > > + capstr);
> > > + goto cleanup;
> > > + }
> > > + }
> > > + }
> > > +# endif
> >
> > This is set a bit earlier than I set it in my PoC patch, but I'll assume
> > it still works given the comment you added.
>
> I was trying to understand whether there was a particular reason why you added
> it to the ambient set later, so my first lame attempt was to merge the 2 'for'
> loops into 1, since they were identical apart from the prctl syscall which led
> to an error. So I investigated and found the restriction I mentioned in the
> comment so I moved it after the caps were first applied and it did work:
> (trial-error)+-research method™
I added it really early at first and it didn't work, so I the put it right
at the end. I didn't bother to try it in the middle as I was lazy :-)
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.