[libvirt] [PATCH] esx_util.c: avoid NULL deref for invalid inputs
The trouble here is that "goto failure" would run this code: failure: VIR_FREE(*datastoreName); VIR_FREE(*directoryName); VIR_FREE(*fileName); result = -1; goto cleanup; And thus if any of those 3 input variables was NULL, that deref would probably provoke a segfault. By the way, that interface should be changed, because it encourages risky behavior: int esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, const char *datastoreRelatedPath, char **datastoreName, char **directoryName, char **fileName) Note how it takes a buffer, datastoreRelatedPath, but with no length parameter. Then this function writes into the buffer with this code: if (sscanf(datastoreRelatedPath, "[%a[^]%]] %a[^\n]", datastoreName, &directoryAndFileName) != 2) { which cannot even check for buffer overrun because it doesn't know the length. You might want to add a new parameter, buf_len: int esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, const char *datastoreRelatedPath, size_t buf_len, char **datastoreName, char **directoryName, char **fileName)
From aba304e07835c123bd63f1d5d6777a898916349f Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 15 Dec 2009 19:08:49 +0100 Subject: [PATCH] esx_util.c: avoid NULL deref for invalid inputs
* src/esx/esx_util.c (esxUtil_ParseDatastoreRelatedPath): Return right away, rather than trying to clean up and dereference NULL pointers, if any input is invalid. --- src/esx/esx_util.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 3e53921..8703ac2 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -277,7 +277,7 @@ esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, directoryName == NULL || *directoryName != NULL || fileName == NULL || *fileName != NULL) { ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Invalid argument"); - goto failure; + return -1; } /* @@ -299,7 +299,7 @@ esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Datastore related path '%s' doesn't have expected format " "'[<datastore>] <path>'", datastoreRelatedPath); - goto failure; + return -1; } /* Split <path> into <directory>/<file>, where <directory> is optional */ -- 1.6.6.rc2.275.g51e2d
2009/12/15 Jim Meyering <jim@meyering.net>:
The trouble here is that "goto failure" would run this code:
failure: VIR_FREE(*datastoreName); VIR_FREE(*directoryName); VIR_FREE(*fileName); result = -1; goto cleanup;
And thus if any of those 3 input variables was NULL, that deref would probably provoke a segfault.
By the way, that interface should be changed, because it encourages risky behavior:
int esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, const char *datastoreRelatedPath, char **datastoreName, char **directoryName, char **fileName)
Note how it takes a buffer, datastoreRelatedPath, but with no length parameter. Then this function writes into the buffer with this code:
if (sscanf(datastoreRelatedPath, "[%a[^]%]] %a[^\n]", datastoreName, &directoryAndFileName) != 2) {
No, it doesn't write, it's sscanf and not printf. It reads from datastoreRelatedPath.
which cannot even check for buffer overrun because it doesn't know the length.
You might want to add a new parameter, buf_len:
int esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, const char *datastoreRelatedPath, size_t buf_len, char **datastoreName, char **directoryName, char **fileName)
From aba304e07835c123bd63f1d5d6777a898916349f Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 15 Dec 2009 19:08:49 +0100 Subject: [PATCH] esx_util.c: avoid NULL deref for invalid inputs
* src/esx/esx_util.c (esxUtil_ParseDatastoreRelatedPath): Return right away, rather than trying to clean up and dereference NULL pointers, if any input is invalid. --- src/esx/esx_util.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 3e53921..8703ac2 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -277,7 +277,7 @@ esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, directoryName == NULL || *directoryName != NULL || fileName == NULL || *fileName != NULL) { ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Invalid argument"); - goto failure; + return -1; }
Another one of this pattern. Okay.
/* @@ -299,7 +299,7 @@ esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Datastore related path '%s' doesn't have expected format " "'[<datastore>] <path>'", datastoreRelatedPath); - goto failure; + return -1; }
This fix is not correct, it may leak memory allocated by sscanf. goto failure is correct and safe here, because the first if block in this function has check the char ** pointers to be not-NULL, so the VIR_FREEs after the failure label is safe.
/* Split <path> into <directory>/<file>, where <directory> is optional */ -- 1.6.6.rc2.275.g51e2d
Partly NACK. Thanks for finding this problems! Matthias
Matthias Bolte wrote:
2009/12/15 Jim Meyering <jim@meyering.net>:
The trouble here is that "goto failure" would run this code:
failure: VIR_FREE(*datastoreName); VIR_FREE(*directoryName); VIR_FREE(*fileName); result = -1; goto cleanup;
And thus if any of those 3 input variables was NULL, that deref would probably provoke a segfault.
By the way, that interface should be changed, because it encourages risky behavior:
int esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, const char *datastoreRelatedPath, char **datastoreName, char **directoryName, char **fileName)
Note how it takes a buffer, datastoreRelatedPath, but with no length parameter. Then this function writes into the buffer with this code:
if (sscanf(datastoreRelatedPath, "[%a[^]%]] %a[^\n]", datastoreName, &directoryAndFileName) != 2) {
No, it doesn't write, it's sscanf and not printf. It reads from datastoreRelatedPath.
Whoops ;-) Shame on me.
which cannot even check for buffer overrun because it doesn't know the length.
You might want to add a new parameter, buf_len:
int esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, const char *datastoreRelatedPath, size_t buf_len, char **datastoreName, char **directoryName, char **fileName)
From aba304e07835c123bd63f1d5d6777a898916349f Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 15 Dec 2009 19:08:49 +0100 Subject: [PATCH] esx_util.c: avoid NULL deref for invalid inputs
* src/esx/esx_util.c (esxUtil_ParseDatastoreRelatedPath): Return right away, rather than trying to clean up and dereference NULL pointers, if any input is invalid. --- src/esx/esx_util.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 3e53921..8703ac2 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -277,7 +277,7 @@ esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, directoryName == NULL || *directoryName != NULL || fileName == NULL || *fileName != NULL) { ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Invalid argument"); - goto failure; + return -1; }
Another one of this pattern. Okay.
/* @@ -299,7 +299,7 @@ esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Datastore related path '%s' doesn't have expected format " "'[<datastore>] <path>'", datastoreRelatedPath); - goto failure; + return -1; }
This fix is not correct, it may leak memory allocated by sscanf. goto failure is correct and safe here, because the first if block in this function has check the char ** pointers to be not-NULL, so the VIR_FREEs after the failure label is safe.
Good catch. I'll adjust and resend.
Jim Meyering wrote:
if (sscanf(datastoreRelatedPath, "[%a[^]%]] %a[^\n]", datastoreName, &directoryAndFileName) != 2) { ... I suppose you know that %a[...] is non-standard (it's a glibc-specific addition) and that this esx code need only compile on systems with glibc.
I confirm that with my change that would have leaked. Though note that it would do so only when parsing exactly one token.
This fix is not correct, it may leak memory allocated by sscanf. goto failure is correct and safe here, because the first if block in this function has check the char ** pointers to be not-NULL, so the VIR_FREEs after the failure label is safe.
Good catch. I'll adjust and resend.
I've folded in this correction: diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 8703ac2..35a48e0 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -299,7 +299,7 @@ esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Datastore related path '%s' doesn't have expected format " "'[<datastore>] <path>'", datastoreRelatedPath); - return -1; + goto failure; } /* Split <path> into <directory>/<file>, where <directory> is optional */ Here's the result:
From f34c4395b9b16965705f9158ac90e59c37b04507 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 15 Dec 2009 19:08:49 +0100 Subject: [PATCH] esx_util.c: avoid NULL deref for invalid inputs
* src/esx/esx_util.c (esxUtil_ParseDatastoreRelatedPath): Return right away for invalid inputs, rather than using them (which would dereference NULL pointers) in clean-up code. --- src/esx/esx_util.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 3e53921..35a48e0 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -277,7 +277,7 @@ esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, directoryName == NULL || *directoryName != NULL || fileName == NULL || *fileName != NULL) { ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Invalid argument"); - goto failure; + return -1; } /* -- 1.6.6.rc2.275.g51e2d
2009/12/16 Jim Meyering <jim@meyering.net>:
Jim Meyering wrote:
if (sscanf(datastoreRelatedPath, "[%a[^]%]] %a[^\n]", datastoreName, &directoryAndFileName) != 2) { ... I suppose you know that %a[...] is non-standard (it's a glibc-specific addition) and that this esx code need only compile on systems with glibc.
No, I wasn't aware of that. The ESX driver is a libvirt client side driver and should be compilable on non-linux platforms too. I think I can replace the two sscanf calls with some strchr/strstr and strndup calls.
I confirm that with my change that would have leaked. Though note that it would do so only when parsing exactly one token.
This fix is not correct, it may leak memory allocated by sscanf. goto failure is correct and safe here, because the first if block in this function has check the char ** pointers to be not-NULL, so the VIR_FREEs after the failure label is safe.
Good catch. I'll adjust and resend.
I've folded in this correction:
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 8703ac2..35a48e0 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -299,7 +299,7 @@ esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Datastore related path '%s' doesn't have expected format " "'[<datastore>] <path>'", datastoreRelatedPath); - return -1; + goto failure; }
/* Split <path> into <directory>/<file>, where <directory> is optional */
Here's the result:
From f34c4395b9b16965705f9158ac90e59c37b04507 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 15 Dec 2009 19:08:49 +0100 Subject: [PATCH] esx_util.c: avoid NULL deref for invalid inputs
* src/esx/esx_util.c (esxUtil_ParseDatastoreRelatedPath): Return right away for invalid inputs, rather than using them (which would dereference NULL pointers) in clean-up code. --- src/esx/esx_util.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c index 3e53921..35a48e0 100644 --- a/src/esx/esx_util.c +++ b/src/esx/esx_util.c @@ -277,7 +277,7 @@ esxUtil_ParseDatastoreRelatedPath(virConnectPtr conn, directoryName == NULL || *directoryName != NULL || fileName == NULL || *fileName != NULL) { ESX_ERROR(conn, VIR_ERR_INTERNAL_ERROR, "Invalid argument"); - goto failure; + return -1; }
/* -- 1.6.6.rc2.275.g51e2d
ACK. Matthias
Matthias Bolte wrote:
From f34c4395b9b16965705f9158ac90e59c37b04507 Mon Sep 17 00:00:00 2001 Subject: [PATCH] esx_util.c: avoid NULL deref for invalid inputs
* src/esx/esx_util.c (esxUtil_ParseDatastoreRelatedPath): Return right away for invalid inputs, rather than using them (which would dereference NULL pointers) in clean-up code.
ACK.
Thanks for the reviews. I've pushed that as well as the esx_vi.c change.
participants (2)
-
Jim Meyering -
Matthias Bolte