Re: [libvirt] [PATCH 3/3] security: Don't remember labels for TPM

2019-10-11 Thread Cole Robinson 

On 10/11/19 11:08 AM, Michal Privoznik wrote:

On 10/10/19 10:15 PM, Cole Robinson wrote:

On 10/1/19 11:00 AM, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1755803

The /dev/tpmN file can be opened only once, as implemented in
drivers/char/tpm/tpm-dev.c:tpm_open() from the kernel's tree. Any
other attempt to open the file fails. And since we're opening the
file ourselves and passing the FD to qemu we will not succeed
opening the file again when locking it for seclabel remembering.

Signed-off-by: Michal Privoznik 
---
  src/security/security_dac.c | 18 +-
  src/security/security_selinux.c | 10 +-
  2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 2733fa664f..347a7a5f63 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1653,14 +1653,14 @@ 
virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr,

  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-    ret = virSecurityDACSetChardevLabel(mgr, def,
- &tpm->data.passthrough.source,
-    false);
+    ret = virSecurityDACSetChardevLabelHelper(mgr, def,
+ &tpm->data.passthrough.source,
+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
-    ret = virSecurityDACSetChardevLabel(mgr, def,
-    &tpm->data.emulator.source,
-    false);
+    ret = virSecurityDACSetChardevLabelHelper(mgr, def,
+ &tpm->data.emulator.source,
+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_LAST:
  break;
@@ -1679,9 +1679,9 @@ 
virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr,

  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-    ret = virSecurityDACRestoreChardevLabel(mgr, def,
- &tpm->data.passthrough.source,
-    false);
+    ret = virSecurityDACRestoreChardevLabelHelper(mgr, def,
+ &tpm->data.passthrough.source,
+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
  /* swtpm will have removed the Unix socket upon termination */
diff --git a/src/security/security_selinux.c 
b/src/security/security_selinux.c

index e3be724a2b..0486bdd6b6 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1682,14 +1682,14 @@ 
virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr,

  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
  tpmdev = tpm->data.passthrough.source.data.file.path;
-    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, true);
+    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, false);

  if (rc < 0)
  return -1;
  if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) {
  rc = virSecuritySELinuxSetFilecon(mgr,
    cancel_path,
-  seclabel->imagelabel, 
true);
+  seclabel->imagelabel, 
false);

  VIR_FREE(cancel_path);
  if (rc < 0) {
  virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, 
tpm);
@@ -1701,7 +1701,7 @@ 
virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr,

  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
  tpmdev = tpm->data.emulator.source.data.nix.path;
-    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, true);
+    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, false);

  if (rc < 0)
  return -1;
  break;


Are the EMULATOR changes actually required? I think it just uses a 
unix socket and doesn't touch the host kernel tpm subsystem


Well, not per se, because the socket is created on domain startup and 
then unlink()-ed on domain shutdown. That's why it doesn't make sense to 
try remember anything about the socket.


Okay so it's basically a separate issue from the patch series. In that 
case use of virSecurityDACRestoreChardevLabelHelper for EMULATOR is a 
bit misleading, and instead virSecurityDACRestoreChardevLabel should 
probably grow some smarts to set remember=false for any unix socket path 
it sees. But it's a minor distinction because I don't think it matters 
in practice given that sockets are unlinked like you say


Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] security: Don't remember labels for TPM

2019-10-11 Thread Michal Privoznik 

On 10/10/19 10:15 PM, Cole Robinson wrote:

On 10/1/19 11:00 AM, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1755803

The /dev/tpmN file can be opened only once, as implemented in
drivers/char/tpm/tpm-dev.c:tpm_open() from the kernel's tree. Any
other attempt to open the file fails. And since we're opening the
file ourselves and passing the FD to qemu we will not succeed
opening the file again when locking it for seclabel remembering.

Signed-off-by: Michal Privoznik 
---
  src/security/security_dac.c | 18 +-
  src/security/security_selinux.c | 10 +-
  2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 2733fa664f..347a7a5f63 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1653,14 +1653,14 @@ 
virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr,

  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-    ret = virSecurityDACSetChardevLabel(mgr, def,
-
&tpm->data.passthrough.source,

-    false);
+    ret = virSecurityDACSetChardevLabelHelper(mgr, def,
+  
&tpm->data.passthrough.source,

+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
-    ret = virSecurityDACSetChardevLabel(mgr, def,
-    &tpm->data.emulator.source,
-    false);
+    ret = virSecurityDACSetChardevLabelHelper(mgr, def,
+  
&tpm->data.emulator.source,

+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_LAST:
  break;
@@ -1679,9 +1679,9 @@ 
virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr,

  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-    ret = virSecurityDACRestoreChardevLabel(mgr, def,
-
&tpm->data.passthrough.source,

-    false);
+    ret = virSecurityDACRestoreChardevLabelHelper(mgr, def,
+  
&tpm->data.passthrough.source,

+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
  /* swtpm will have removed the Unix socket upon termination */
diff --git a/src/security/security_selinux.c 
b/src/security/security_selinux.c

index e3be724a2b..0486bdd6b6 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1682,14 +1682,14 @@ 
virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr,

  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
  tpmdev = tpm->data.passthrough.source.data.file.path;
-    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, true);
+    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, false);

  if (rc < 0)
  return -1;
  if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) {
  rc = virSecuritySELinuxSetFilecon(mgr,
    cancel_path,
-  seclabel->imagelabel, 
true);
+  seclabel->imagelabel, 
false);

  VIR_FREE(cancel_path);
  if (rc < 0) {
  virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, 
tpm);
@@ -1701,7 +1701,7 @@ 
virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr,

  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
  tpmdev = tpm->data.emulator.source.data.nix.path;
-    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, true);
+    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, false);

  if (rc < 0)
  return -1;
  break;


Are the EMULATOR changes actually required? I think it just uses a unix 
socket and doesn't touch the host kernel tpm subsystem


Well, not per se, because the socket is created on domain startup and 
then unlink()-ed on domain shutdown. That's why it doesn't make sense to 
try remember anything about the socket.


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] security: Don't remember labels for TPM

2019-10-10 Thread Cole Robinson 

On 10/1/19 11:00 AM, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1755803

The /dev/tpmN file can be opened only once, as implemented in
drivers/char/tpm/tpm-dev.c:tpm_open() from the kernel's tree. Any
other attempt to open the file fails. And since we're opening the
file ourselves and passing the FD to qemu we will not succeed
opening the file again when locking it for seclabel remembering.

Signed-off-by: Michal Privoznik 
---
  src/security/security_dac.c | 18 +-
  src/security/security_selinux.c | 10 +-
  2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 2733fa664f..347a7a5f63 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1653,14 +1653,14 @@ virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr,
  
  switch (tpm->type) {

  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-ret = virSecurityDACSetChardevLabel(mgr, def,
-&tpm->data.passthrough.source,
-false);
+ret = virSecurityDACSetChardevLabelHelper(mgr, def,
+  
&tpm->data.passthrough.source,
+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
-ret = virSecurityDACSetChardevLabel(mgr, def,
-&tpm->data.emulator.source,
-false);
+ret = virSecurityDACSetChardevLabelHelper(mgr, def,
+  &tpm->data.emulator.source,
+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_LAST:
  break;
@@ -1679,9 +1679,9 @@ virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr 
mgr,
  
  switch (tpm->type) {

  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-ret = virSecurityDACRestoreChardevLabel(mgr, def,
-&tpm->data.passthrough.source,
-false);
+ret = virSecurityDACRestoreChardevLabelHelper(mgr, def,
+  
&tpm->data.passthrough.source,
+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
  /* swtpm will have removed the Unix socket upon termination */
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index e3be724a2b..0486bdd6b6 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1682,14 +1682,14 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr 
mgr,
  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
  tpmdev = tpm->data.passthrough.source.data.file.path;
-rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, 
true);
+rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, 
false);
  if (rc < 0)
  return -1;
  
  if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) {

  rc = virSecuritySELinuxSetFilecon(mgr,
cancel_path,
-  seclabel->imagelabel, true);
+  seclabel->imagelabel, false);
  VIR_FREE(cancel_path);
  if (rc < 0) {
  virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, tpm);
@@ -1701,7 +1701,7 @@ virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr 
mgr,
  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
  tpmdev = tpm->data.emulator.source.data.nix.path;
-rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, 
true);
+rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, seclabel->imagelabel, 
false);
  if (rc < 0)
  return -1;
  break;


Are the EMULATOR changes actually required? I think it just uses a unix 
socket and doesn't touch the host kernel tpm subsystem


- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list