[libvirt] [PATCH 0/2] security: Fix the transaction model's list appending
The problem lies in how elements are appended into the transaction list - instead of making a deep copy of a string we were doing a shallow copy only. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1413773 Erik Skultety (2): security: DAC: fix the transaction model's list append security: SELinux: fix the transaction model's list append src/security/security_dac.c | 30 +++++++++++++++++++++--------- src/security/security_selinux.c | 30 +++++++++++++++++++++--------- 2 files changed, 42 insertions(+), 18 deletions(-) -- 2.10.2
The problem is in the way how the list item is created prior to appending it to the transaction list - the @path attribute is just a shallow copy instead of deep copy of the hostdev device's path. Unfortunately, the hostdev devices from which the @path is extracted, in order to add them into the transaction list, are only temporary and freed before the buildup of the qemu namespace, thus making the @path attribute in the transaction list NULL, causing 'permission denied' or 'double free' or 'unknown cause' errors. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1413773 Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/security/security_dac.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index d457e6a..d7a2de4 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -71,7 +71,7 @@ struct _virSecurityDACCallbackData { typedef struct _virSecurityDACChownItem virSecurityDACChownItem; typedef virSecurityDACChownItem *virSecurityDACChownItemPtr; struct _virSecurityDACChownItem { - const char *path; + char *path; const virStorageSource *src; uid_t uid; gid_t gid; @@ -95,22 +95,32 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list, uid_t uid, gid_t gid) { - virSecurityDACChownItemPtr item; + int ret = -1; + char *tmp = NULL; + virSecurityDACChownItemPtr item = NULL; if (VIR_ALLOC(item) < 0) return -1; - item->path = path; + if (VIR_STRDUP(tmp, path) < 0) + goto cleanup; + + item->path = tmp; item->src = src; item->uid = uid; item->gid = gid; - if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) { - VIR_FREE(item); - return -1; - } + if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) + goto cleanup; - return 0; + tmp = NULL; + item = NULL; + + ret = 0; + cleanup: + VIR_FREE(tmp); + VIR_FREE(item); + return ret; } static void @@ -122,8 +132,10 @@ virSecurityDACChownListFree(void *opaque) if (!list) return; - for (i = 0; i < list->nItems; i++) + for (i = 0; i < list->nItems; i++) { + VIR_FREE(list->items[i]->path); VIR_FREE(list->items[i]); + } VIR_FREE(list); } -- 2.10.2
On 01/17/2017 02:56 PM, Erik Skultety wrote:
The problem is in the way how the list item is created prior to appending it to the transaction list - the @path attribute is just a shallow copy instead of deep copy of the hostdev device's path. Unfortunately, the hostdev devices from which the @path is extracted, in order to add them into the transaction list, are only temporary and freed before the buildup of the qemu namespace, thus making the @path attribute in the transaction list NULL, causing 'permission denied' or 'double free' or 'unknown cause' errors.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1413773
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/security/security_dac.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index d457e6a..d7a2de4 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -71,7 +71,7 @@ struct _virSecurityDACCallbackData { typedef struct _virSecurityDACChownItem virSecurityDACChownItem; typedef virSecurityDACChownItem *virSecurityDACChownItemPtr; struct _virSecurityDACChownItem { - const char *path; + char *path; const virStorageSource *src; uid_t uid; gid_t gid; @@ -95,22 +95,32 @@ virSecurityDACChownListAppend(virSecurityDACChownListPtr list, uid_t uid, gid_t gid) { - virSecurityDACChownItemPtr item; + int ret = -1; + char *tmp = NULL; + virSecurityDACChownItemPtr item = NULL;
if (VIR_ALLOC(item) < 0) return -1;
- item->path = path; + if (VIR_STRDUP(tmp, path) < 0) + goto cleanup; + + item->path = tmp; item->src = src; item->uid = uid; item->gid = gid;
- if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) { - VIR_FREE(item); - return -1; - } + if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) + goto cleanup;
- return 0; + tmp = NULL; + item = NULL;
This 'item = NULL' is not needed. VIR_APPEND_ELEMENT sets @item to NULL upon successful return. But I agree that it is hard to spot.
+ + ret = 0; + cleanup: + VIR_FREE(tmp); + VIR_FREE(item); + return ret; }
static void @@ -122,8 +132,10 @@ virSecurityDACChownListFree(void *opaque) if (!list) return;
- for (i = 0; i < list->nItems; i++) + for (i = 0; i < list->nItems; i++) { + VIR_FREE(list->items[i]->path); VIR_FREE(list->items[i]); + } VIR_FREE(list); }
ACK and safe for the freeze. Michal
- if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) { - VIR_FREE(item); - return -1; - } + if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) + goto cleanup;
- return 0; + tmp = NULL; + item = NULL;
This 'item = NULL' is not needed. VIR_APPEND_ELEMENT sets @item to NULL upon successful return. But I agree that it is hard to spot.
Oh, missed that one, thanks for the suggestion. Erik
The problem is in the way how the list item is created prior to appending it to the transaction list - the @path argument is just a shallow copy instead of deep copy of the hostdev device's path. Unfortunately, the hostdev devices from which the @path is extracted, in order to add them into the transaction list, are only temporary and freed before the buildup of the qemu namespace, thus making the @path attribute in the transaction list NULL, causing 'permission denied' or 'double free' or 'unknown cause' errors. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1413773 Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/security/security_selinux.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f229b51..c799056 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -81,7 +81,7 @@ struct _virSecuritySELinuxCallbackData { typedef struct _virSecuritySELinuxContextItem virSecuritySELinuxContextItem; typedef virSecuritySELinuxContextItem *virSecuritySELinuxContextItemPtr; struct _virSecuritySELinuxContextItem { - const char *path; + char *path; const char *tcon; bool optional; }; @@ -111,21 +111,31 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, const char *tcon, bool optional) { - virSecuritySELinuxContextItemPtr item; + int ret = -1; + char *tmp = NULL; + virSecuritySELinuxContextItemPtr item = NULL; if (VIR_ALLOC(item) < 0) return -1; - item->path = path; + if (VIR_STRDUP(tmp, path) < 0) + goto cleanup; + + item->path = tmp; item->tcon = tcon; item->optional = optional; - if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) { - VIR_FREE(item); - return -1; - } + if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) + goto cleanup; - return 0; + tmp = NULL; + item = NULL; + + ret = 0; + cleanup: + VIR_FREE(tmp); + VIR_FREE(item); + return ret; } static void @@ -137,8 +147,10 @@ virSecuritySELinuxContextListFree(void *opaque) if (!list) return; - for (i = 0; i < list->nItems; i++) + for (i = 0; i < list->nItems; i++) { + VIR_FREE(list->items[i]->path); VIR_FREE(list->items[i]); + } VIR_FREE(list); } -- 2.10.2
On 01/17/2017 02:56 PM, Erik Skultety wrote:
The problem is in the way how the list item is created prior to appending it to the transaction list - the @path argument is just a shallow copy instead of deep copy of the hostdev device's path. Unfortunately, the hostdev devices from which the @path is extracted, in order to add them into the transaction list, are only temporary and freed before the buildup of the qemu namespace, thus making the @path attribute in the transaction list NULL, causing 'permission denied' or 'double free' or 'unknown cause' errors.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1413773
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/security/security_selinux.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index f229b51..c799056 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -81,7 +81,7 @@ struct _virSecuritySELinuxCallbackData { typedef struct _virSecuritySELinuxContextItem virSecuritySELinuxContextItem; typedef virSecuritySELinuxContextItem *virSecuritySELinuxContextItemPtr; struct _virSecuritySELinuxContextItem { - const char *path; + char *path; const char *tcon; bool optional; }; @@ -111,21 +111,31 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, const char *tcon, bool optional) { - virSecuritySELinuxContextItemPtr item; + int ret = -1; + char *tmp = NULL; + virSecuritySELinuxContextItemPtr item = NULL;
if (VIR_ALLOC(item) < 0) return -1;
- item->path = path; + if (VIR_STRDUP(tmp, path) < 0) + goto cleanup; + + item->path = tmp; item->tcon = tcon;
Unfortunately, while this was enough in the DAC driver, it is not enough here. @tcon may be dynamically allocated just for this call: virSecuritySELinuxRestoreFileLabel -> virSecuritySELinuxSetFilecon -> virSecuritySELinuxSetFileconHelper -> virSecuritySELinuxTransactionAppend -> virSecuritySELinuxContextListAppend However, I guess fixing that is trivial. ACK if you do so and safe for the freeze. Michal
@@ -111,21 +111,31 @@ virSecuritySELinuxContextListAppend(virSecuritySELinuxContextListPtr list, const char *tcon, bool optional) { - virSecuritySELinuxContextItemPtr item; + int ret = -1; + char *tmp = NULL; + virSecuritySELinuxContextItemPtr item = NULL;
if (VIR_ALLOC(item) < 0) return -1;
- item->path = path; + if (VIR_STRDUP(tmp, path) < 0) + goto cleanup; + + item->path = tmp; item->tcon = tcon;
Unfortunately, while this was enough in the DAC driver, it is not enough here. @tcon may be dynamically allocated just for this call:
virSecuritySELinuxRestoreFileLabel -> virSecuritySELinuxSetFilecon -> virSecuritySELinuxSetFileconHelper -> virSecuritySELinuxTransactionAppend -> virSecuritySELinuxContextListAppend
However, I guess fixing that is trivial. ACK if you do so and safe for the freeze.
Adjusted both patches according to your suggestions and pushed, thanks. Erik
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Erik Skultety -
Michal Privoznik