5:32 a.m.
The bitmap parsing code might cause a crash of the application using it.
Fix it and add tests so that it doesn't happen again.
Peter Krempa (3):
virbitmap: Refactor virBitmapParse to avoid access beyond bounds of
array
virbitmaptest: Fix function header formatting
virbitmaptest: Add test for out of bounds condition
src/util/virbitmap.c | 38 +++++++++++++-------------------
tests/virbitmaptest.c | 60 ++++++++++++++++++++++++++++++++++++++++++++-------
2 files changed, 67 insertions(+), 31 deletions(-)
--
1.8.3.2
5:32 a.m.
New subject: [libvirt] [PATCH 1/3] virbitmap: Refactor virBitmapParse to avoid access beyond bounds of array
The virBitmapParse function was calling virBitmapIsSet() function that
requires the caller to check the bounds of the bitmap without checking
them. This resulted into crashes when parsing a bitmap string that was
exceeding the bounds used as argument.
This patch refactors the function to use virBitmapSetBit without
checking if the bit is set (this function does the checks internally)
and then counts the bits in the bitmap afterwards (instead of keeping
track while parsing the string).
This patch also changes the "parse_error" label to a more common
"error".
The refactor should also get rid of the need to call sa_assert on the
returned variable as the callpath should allow coverity to infer the
possible return values.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=997367
Thanks to Alex Jia for tracking down the issue.
---
src/util/virbitmap.c | 38 +++++++++++++++-----------------------
1 file changed, 15 insertions(+), 23 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index 7e1cd02..47c678e 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -297,7 +297,6 @@ virBitmapParse(const char *str,
virBitmapPtr *bitmap,
size_t bitmapSize)
{
- int ret = 0;
bool neg = false;
const char *cur;
char *tmp;
@@ -330,12 +329,12 @@ virBitmapParse(const char *str,
}
if (!c_isdigit(*cur))
- goto parse_error;
+ goto error;
if (virStrToLong_i(cur, &tmp, 10, &start) < 0)
- goto parse_error;
+ goto error;
if (start < 0)
- goto parse_error;
+ goto error;
cur = tmp;
@@ -343,35 +342,29 @@ virBitmapParse(const char *str,
if (*cur == ',' || *cur == 0 || *cur == terminator) {
if (neg) {
- if (virBitmapIsSet(*bitmap, start)) {
- ignore_value(virBitmapClearBit(*bitmap, start));
- ret--;
- }
+ if (virBitmapClearBit(*bitmap, start) < 0)
+ goto error;
} else {
- if (!virBitmapIsSet(*bitmap, start)) {
- ignore_value(virBitmapSetBit(*bitmap, start));
- ret++;
- }
+ if (virBitmapSetBit(*bitmap, start) < 0)
+ goto error;
}
} else if (*cur == '-') {
if (neg)
- goto parse_error;
+ goto error;
cur++;
virSkipSpaces(&cur);
if (virStrToLong_i(cur, &tmp, 10, &last) < 0)
- goto parse_error;
+ goto error;
if (last < start)
- goto parse_error;
+ goto error;
cur = tmp;
for (i = start; i <= last; i++) {
- if (!virBitmapIsSet(*bitmap, i)) {
- ignore_value(virBitmapSetBit(*bitmap, i));
- ret++;
- }
+ if (virBitmapSetBit(*bitmap, i) < 0)
+ goto error;
}
virSkipSpaces(&cur);
@@ -384,14 +377,13 @@ virBitmapParse(const char *str,
} else if (*cur == 0 || *cur == terminator) {
break;
} else {
- goto parse_error;
+ goto error;
}
}
- sa_assert(ret >= 0);
- return ret;
+ return virBitmapCountBits(*bitmap);
-parse_error:
+error:
virBitmapFree(*bitmap);
*bitmap = NULL;
return -1;
--
1.8.3.2
7:11 a.m.
New subject: [libvirt] [PATCH 1/3] virbitmap: Refactor virBitmapParse to avoid access beyond bounds of array
On 08/16/2013 04:32 AM, Peter Krempa wrote:
The virBitmapParse function was calling virBitmapIsSet() function
that
requires the caller to check the bounds of the bitmap without checking
them. This resulted into crashes when parsing a bitmap string that was
exceeding the bounds used as argument.
This patch refactors the function to use virBitmapSetBit without
checking if the bit is set (this function does the checks internally)
and then counts the bits in the bitmap afterwards (instead of keeping
track while parsing the string).
This patch also changes the "parse_error" label to a more common
"error".
The refactor should also get rid of the need to call sa_assert on the
returned variable as the callpath should allow coverity to infer the
possible return values.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=997367
Thanks to Alex Jia for tracking down the issue.
---
src/util/virbitmap.c | 38 +++++++++++++++-----------------------
1 file changed, 15 insertions(+), 23 deletions(-)
This patch is the fix for CVE-2013-5651.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:32 a.m.
New subject: [libvirt] [PATCH 2/3] virbitmaptest: Fix function header formatting
---
tests/virbitmaptest.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
index 9c7329e..8cfd8b5 100644
--- a/tests/virbitmaptest.c
+++ b/tests/virbitmaptest.c
@@ -1,4 +1,6 @@
/*
+ * virbitmaptest.c: Test the bitmap code
+ *
* Copyright (C) 2013 Red Hat, Inc.
* Copyright (C) 2012 Fujitsu.
*
@@ -24,7 +26,8 @@
#include "virbitmap.h"
-static int test1(const void *data ATTRIBUTE_UNUSED)
+static int
+test1(const void *data ATTRIBUTE_UNUSED)
{
virBitmapPtr bitmap;
int size;
@@ -78,7 +81,8 @@ testBit(virBitmapPtr bitmap,
return -1;
}
-static int test2(const void *data ATTRIBUTE_UNUSED)
+static int
+test2(const void *data ATTRIBUTE_UNUSED)
{
const char *bitsString1 = "1-32,50,88-99,1021-1023";
char *bitsString2 = NULL;
@@ -140,7 +144,8 @@ error:
return ret;
}
-static int test3(const void *data ATTRIBUTE_UNUSED)
+static int
+test3(const void *data ATTRIBUTE_UNUSED)
{
virBitmapPtr bitmap = NULL;
int ret = -1;
@@ -167,7 +172,8 @@ error:
}
/* test for virBitmapNextSetBit, virBitmapNextClearBit */
-static int test4(const void *data ATTRIBUTE_UNUSED)
+static int
+test4(const void *data ATTRIBUTE_UNUSED)
{
const char *bitsString = "0, 2-4, 6-10, 12, 14-18, 20, 22, 25";
int size = 40;
@@ -261,7 +267,8 @@ error:
}
/* test for virBitmapNewData/ToData */
-static int test5(const void *v ATTRIBUTE_UNUSED)
+static int
+test5(const void *v ATTRIBUTE_UNUSED)
{
char data[] = {0x01, 0x02, 0x00, 0x00, 0x04};
unsigned char *data2 = NULL;
@@ -309,7 +316,8 @@ error:
/* test for virBitmapFormat */
-static int test6(const void *v ATTRIBUTE_UNUSED)
+static int
+test6(const void *v ATTRIBUTE_UNUSED)
{
virBitmapPtr bitmap = NULL;
char *str = NULL;
@@ -390,7 +398,8 @@ error:
return ret;
}
-static int test7(const void *v ATTRIBUTE_UNUSED)
+static int
+test7(const void *v ATTRIBUTE_UNUSED)
{
virBitmapPtr bitmap;
size_t i;
@@ -429,7 +438,8 @@ error:
return -1;
}
-static int test8(const void *v ATTRIBUTE_UNUSED)
+static int
+test8(const void *v ATTRIBUTE_UNUSED)
{
virBitmapPtr bitmap = NULL;
char data[108] = {0x00,};
--
1.8.3.2
5:32 a.m.
New subject: [libvirt] [PATCH 3/3] virbitmaptest: Add test for out of bounds condition
Previous patch fixed an issue where when parsing a bitmap from the a
string the bounds of the bitmap weren't checked. That flaw resulted into
crashes. This test tests that case to avoid it in the future.
---
tests/virbitmaptest.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
index 8cfd8b5..c56d6fa 100644
--- a/tests/virbitmaptest.c
+++ b/tests/virbitmaptest.c
@@ -464,6 +464,38 @@ cleanup:
return ret;
}
+
+/* test out of bounds conditions on virBitmapParse */
+static int
+test9(const void *opaque ATTRIBUTE_UNUSED)
+{
+ int ret = -1;
+ virBitmapPtr bitmap;
+
+ if (virBitmapParse("100000000", 0, &bitmap, 20) != -1)
+ goto cleanup;
+
+ if (bitmap)
+ goto cleanup;
+
+ if (virBitmapParse("1-1000000000", 0, &bitmap, 20) != -1)
+ goto cleanup;
+
+ if (bitmap)
+ goto cleanup;
+
+ if (virBitmapParse("1-10^10000000000", 0, &bitmap, 20) != -1)
+ goto cleanup;
+
+ if (bitmap)
+ goto cleanup;
+
+ ret = 0;
+cleanup:
+ return ret;
+
+}
+
static int
mymain(void)
{
@@ -485,6 +517,8 @@ mymain(void)
ret = -1;
if (virtTestRun("test8", 1, test8, NULL) < 0)
ret = -1;
+ if (virtTestRun("test9", 1, test9, NULL) < 0)
+ ret = -1;
return ret;
}
--
1.8.3.2
6:55 a.m.
New subject: [libvirt] [PATCH 3/3] virbitmaptest: Add test for out of bounds condition
On 08/16/2013 04:32 AM, Peter Krempa wrote:
Previous patch fixed an issue where when parsing a bitmap from the a
s/where/where,/
s/the a/a/
string the bounds of the bitmap weren't checked. That flaw
resulted into
s/string/string,/
crashes. This test tests that case to avoid it in the future.
---
tests/virbitmaptest.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
6 a.m.
New subject: [libvirt] [PATCH 3/3] virbitmaptest: Add test for out of bounds condition
On 08/16/2013 06:32 AM, Peter Krempa wrote:
Previous patch fixed an issue where when parsing a bitmap from the a
string the bounds of the bitmap weren't checked. That flaw resulted into
crashes. This test tests that case to avoid it in the future.
---
tests/virbitmaptest.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
index 8cfd8b5..c56d6fa 100644
--- a/tests/virbitmaptest.c
+++ b/tests/virbitmaptest.c
@@ -464,6 +464,38 @@ cleanup:
return ret;
}
(just getting back from PTO :-))
Coverity found 3 RESOURCE_LEAK issues - all related though... Looks
like you're missing a "virBitmapFree(bitmap);"
+
+/* test out of bounds conditions on virBitmapParse */
+static int
+test9(const void *opaque ATTRIBUTE_UNUSED)
+{
+ int ret = -1;
+ virBitmapPtr bitmap;
+
+ if (virBitmapParse("100000000", 0, &bitmap, 20) != -1)
+ goto cleanup;
+
(1) Event alloc_arg: "virBitmapParse(char const *, char, virBitmapPtr
*, size_t)" allocates memory that is stored into "bitmap". [details]
+ if (bitmap)
+ goto cleanup;
+
+ if (virBitmapParse("1-1000000000", 0, &bitmap, 20) != -1)
+ goto cleanup;
+
+ if (bitmap)
+ goto cleanup;
+
+ if (virBitmapParse("1-10^10000000000", 0, &bitmap, 20) != -1)
+ goto cleanup;
+
+ if (bitmap)
+ goto cleanup;
+
+ ret = 0;
+cleanup:
+ return ret;
+
494 cleanup:
(5) Event leaked_storage: Variable "bitmap" going out of scope leaks
the storage it points to.
Also see events: [alloc_arg]
495 return ret;
John
+}
+
static int
mymain(void)
{
@@ -485,6 +517,8 @@ mymain(void)
ret = -1;
if (virtTestRun("test8", 1, test8, NULL) < 0)
ret = -1;
+ if (virtTestRun("test9", 1, test9, NULL) < 0)
+ ret = -1;
return ret;
}
7:01 a.m.
New subject: [libvirt] [PATCH 3/3] virbitmaptest: Add test for out of bounds condition
On 08/19/13 13:00, John Ferlan wrote:
On 08/16/2013 06:32 AM, Peter Krempa wrote:
> Previous patch fixed an issue where when parsing a bitmap from the a
> string the bounds of the bitmap weren't checked. That flaw resulted into
> crashes. This test tests that case to avoid it in the future.
> ---
> tests/virbitmaptest.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
> index 8cfd8b5..c56d6fa 100644
> --- a/tests/virbitmaptest.c
> +++ b/tests/virbitmaptest.c
> @@ -464,6 +464,38 @@ cleanup:
> return ret;
> }
>
(just getting back from PTO :-))
Coverity found 3 RESOURCE_LEAK issues - all related though... Looks
like you're missing a "virBitmapFree(bitmap);"
Right, unfortunately they are false positives :)
> +
> +/* test out of bounds conditions on virBitmapParse */
> +static int
> +test9(const void *opaque ATTRIBUTE_UNUSED)
> +{
> + int ret = -1;
> + virBitmapPtr bitmap;
> +
> + if (virBitmapParse("100000000", 0, &bitmap, 20) != -1)
> + goto cleanup;
> +
(1) Event alloc_arg: "virBitmapParse(char const *, char, virBitmapPtr
*, size_t)" allocates memory that is stored into "bitmap". [details]
All of those should fail, and only on a broken test they actually leak
the pointer. Such situation shouldn't ever happen in upstream (famous
last words? :) ).
> + if (bitmap)
> + goto cleanup;
> +
> + if (virBitmapParse("1-1000000000", 0, &bitmap, 20) != -1)
> + goto cleanup;
> +
> + if (bitmap)
> + goto cleanup;
> +
> + if (virBitmapParse("1-10^10000000000", 0, &bitmap, 20) != -1)
> + goto cleanup;
> +
> + if (bitmap)
> + goto cleanup;
> +
> + ret = 0;
> +cleanup:
> + return ret;
> +
494 cleanup:
(5) Event leaked_storage: Variable "bitmap" going out of scope leaks
the storage it points to.
Also see events: [alloc_arg]
But hopefully coverity will be smart enough that if I add a
virBitmapFree() here it won't complain any more.
495 return ret;
John
> +}
> +
> static int
> mymain(void)
> {
> @@ -485,6 +517,8 @@ mymain(void)
> ret = -1;
> if (virtTestRun("test8", 1, test8, NULL) < 0)
> ret = -1;
> + if (virtTestRun("test9", 1, test9, NULL) < 0)
> + ret = -1;
>
> return ret;
> }
>
Peter
5:39 a.m.
On Fri, Aug 16, 2013 at 12:32:06PM +0200, Peter Krempa wrote:
The bitmap parsing code might cause a crash of the application using
it.
Fix it and add tests so that it doesn't happen again.
Peter Krempa (3):
virbitmap: Refactor virBitmapParse to avoid access beyond bounds of
array
virbitmaptest: Fix function header formatting
virbitmaptest: Add test for out of bounds condition
src/util/virbitmap.c | 38 +++++++++++++-------------------
tests/virbitmaptest.c | 60 ++++++++++++++++++++++++++++++++++++++++++++-------
2 files changed, 67 insertions(+), 31 deletions(-)
ACK to all 3. I was just going to suggest adding tests to Alex,
when I saw your followup.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
7:41 a.m.
On 08/16/13 12:39, Daniel P. Berrange wrote:
On Fri, Aug 16, 2013 at 12:32:06PM +0200, Peter Krempa wrote:
> The bitmap parsing code might cause a crash of the application using it.
> Fix it and add tests so that it doesn't happen again.
>
> Peter Krempa (3):
> virbitmap: Refactor virBitmapParse to avoid access beyond bounds of
> array
> virbitmaptest: Fix function header formatting
> virbitmaptest: Add test for out of bounds condition
>
> src/util/virbitmap.c | 38 +++++++++++++-------------------
> tests/virbitmaptest.c | 60 ++++++++++++++++++++++++++++++++++++++++++++-------
> 2 files changed, 67 insertions(+), 31 deletions(-)
ACK to all 3. I was just going to suggest adding tests to Alex,
when I saw your followup.
Thanks, I've fixed the commit message of patch 3/3 according to Eric's
suggestion and added info about the commit introducing the problem to
1/3 and pushed.
Peter
8:15 a.m.
On 08/16/13 14:41, Peter Krempa wrote:
On 08/16/13 12:39, Daniel P. Berrange wrote:
> On Fri, Aug 16, 2013 at 12:32:06PM +0200, Peter Krempa wrote:
>> The bitmap parsing code might cause a crash of the application using it.
>> Fix it and add tests so that it doesn't happen again.
>>
>> Peter Krempa (3):
>> virbitmap: Refactor virBitmapParse to avoid access beyond bounds of
>> array
>> virbitmaptest: Fix function header formatting
>> virbitmaptest: Add test for out of bounds condition
>>
>> src/util/virbitmap.c | 38 +++++++++++++-------------------
>> tests/virbitmaptest.c | 60 ++++++++++++++++++++++++++++++++++++++++++++-------
>> 2 files changed, 67 insertions(+), 31 deletions(-)
>
> ACK to all 3. I was just going to suggest adding tests to Alex,
> when I saw your followup.
Thanks, I've fixed the commit message of patch 3/3 according to Eric's
suggestion and added info about the commit introducing the problem to
1/3 and pushed.
I've also backported the fix (1/3) to the maint branches:
v1.1.1-maint (thanks to Jan Tomko for doing that for me)
v1.1.0-maint
v1.0.6-maint
v1.0.5-maint
v1.0.4-maint and
v0.10.2-maint
I couldn't successfully build v1.0.3-maint and thus didn't bother with
the other maint branches.
Peter
4137
days inactive
4151
days old
10 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan -
Peter Krempa