12:35 p.m.
When reading and dispatching of a message failed the message was freed
but wasn't removed from the message queue.
After that when the connection was about to be closed the pointer for
the message was still present in the queue and it was passed to
virNetMessageFree which tried to call the callback function from an
uninitialized pointer.
This patch removes the message from the queue before it's freed.
* rpc/virnetserverclient.c: virNetServerClientDispatchRead:
- avoid use after free of RPC messages
---
src/rpc/virnetserverclient.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index af0560e..446e1e9 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -987,6 +987,7 @@ readmore:
/* Decode the header so we can use it for routing decisions */
if (virNetMessageDecodeHeader(msg) < 0) {
+ virNetMessageQueueServe(&client->rx);
virNetMessageFree(msg);
client->wantClose = true;
return;
@@ -996,6 +997,7 @@ readmore:
* file descriptors */
if (msg->header.type == VIR_NET_CALL_WITH_FDS &&
virNetMessageDecodeNumFDs(msg) < 0) {
+ virNetMessageQueueServe(&client->rx);
virNetMessageFree(msg);
client->wantClose = true;
return; /* Error */
@@ -1005,6 +1007,7 @@ readmore:
for (i = msg->donefds ; i < msg->nfds ; i++) {
int rv;
if ((rv = virNetSocketRecvFD(client->sock, &(msg->fds[i]))) < 0)
{
+ virNetMessageQueueServe(&client->rx);
virNetMessageFree(msg);
client->wantClose = true;
return;
--
1.8.1.1
12:58 p.m.
On 01/28/2013 11:35 AM, Peter Krempa wrote:
When reading and dispatching of a message failed the message was
freed
but wasn't removed from the message queue.
After that when the connection was about to be closed the pointer for
the message was still present in the queue and it was passed to
virNetMessageFree which tried to call the callback function from an
uninitialized pointer.
This patch removes the message from the queue before it's freed.
Mention CVE-2013-0170 in the commit message, now that it is public:
https://bugzilla.redhat.com/show_bug.cgi?id=893450
* rpc/virnetserverclient.c: virNetServerClientDispatchRead:
- avoid use after free of RPC messages
---
src/rpc/virnetserverclient.c | 3 +++
1 file changed, 3 insertions(+)
ACK. Looks like we need this on {v0.10.2,v0.9.11,v0.9.6}-maint as well.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1:07 p.m.
On 01/28/2013 01:58 PM, Eric Blake wrote:
On 01/28/2013 11:35 AM, Peter Krempa wrote:
> When reading and dispatching of a message failed the message was freed
> but wasn't removed from the message queue.
>
> After that when the connection was about to be closed the pointer for
> the message was still present in the queue and it was passed to
> virNetMessageFree which tried to call the callback function from an
> uninitialized pointer.
>
> This patch removes the message from the queue before it's freed.
Mention CVE-2013-0170 in the commit message, now that it is public:
https://bugzilla.redhat.com/show_bug.cgi?id=893450
>
> * rpc/virnetserverclient.c: virNetServerClientDispatchRead:
> - avoid use after free of RPC messages
> ---
> src/rpc/virnetserverclient.c | 3 +++
> 1 file changed, 3 insertions(+)
ACK. Looks like we need this on {v0.10.2,v0.9.11,v0.9.6}-maint as well.
I'll handle the backports once the patch hits master.
Thanks,
Cole
1:08 p.m.
On 01/28/13 19:58, Eric Blake wrote:
On 01/28/2013 11:35 AM, Peter Krempa wrote:
> When reading and dispatching of a message failed the message was freed
> but wasn't removed from the message queue.
>
> After that when the connection was about to be closed the pointer for
> the message was still present in the queue and it was passed to
> virNetMessageFree which tried to call the callback function from an
> uninitialized pointer.
>
> This patch removes the message from the queue before it's freed.
Mention CVE-2013-0170 in the commit message, now that it is public:
https://bugzilla.redhat.com/show_bug.cgi?id=893450
>
> * rpc/virnetserverclient.c: virNetServerClientDispatchRead:
> - avoid use after free of RPC messages
> ---
> src/rpc/virnetserverclient.c | 3 +++
> 1 file changed, 3 insertions(+)
ACK. Looks like we need this on {v0.10.2,v0.9.11,v0.9.6}-maint as well.
Thanks. I added the CVE notice and pushed to upstream and the v0.10.2
and v0.9.11 maint branches. v0.9.6 is not vulnerable. The problem was
introduced in 0.9.7
Peter
8:05 p.m.
New subject: [libvirt] [PATCH] rpc: Fix crash on error paths of message dispatching
Peter Krempa wrote:
On 01/28/13 19:58, Eric Blake wrote:
> On 01/28/2013 11:35 AM, Peter Krempa wrote:
>> When reading and dispatching of a message failed the message was freed
>> but wasn't removed from the message queue.
>>
>> After that when the connection was about to be closed the pointer for
>> the message was still present in the queue and it was passed to
>> virNetMessageFree which tried to call the callback function from an
>> uninitialized pointer.
>>
>> This patch removes the message from the queue before it's freed.
>
> Mention CVE-2013-0170 in the commit message, now that it is public:
> https://bugzilla.redhat.com/show_bug.cgi?id=893450
>
>>
>> * rpc/virnetserverclient.c: virNetServerClientDispatchRead:
>> - avoid use after free of RPC messages
>> ---
>> src/rpc/virnetserverclient.c | 3 +++
>> 1 file changed, 3 insertions(+)
>
> ACK. Looks like we need this on {v0.10.2,v0.9.11,v0.9.6}-maint as well.
Thanks. I added the CVE notice and pushed to upstream and the v0.10.2
and v0.9.11 maint branches. v0.9.6 is not vulnerable. The problem was
introduced in 0.9.7
Hi Peter,
Looks like 0.9.6 was vulnerable since this made its way to the
v0.9.6-maint branch as well. Do you happen to know when this was
introduced?
Thanks,
Jim
11:01 a.m.
New subject: [libvirt] [PATCH] rpc: Fix crash on error paths of message dispatching
On 01/29/2013 07:05 PM, Jim Fehlig wrote:
>> Mention CVE-2013-0170 in the commit message, now that it is
public:
>> https://bugzilla.redhat.com/show_bug.cgi?id=893450
>>
>>>
>>> * rpc/virnetserverclient.c: virNetServerClientDispatchRead:
>>> - avoid use after free of RPC messages
>>> ---
>>> src/rpc/virnetserverclient.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>
>> ACK. Looks like we need this on {v0.10.2,v0.9.11,v0.9.6}-maint as well.
>
> Thanks. I added the CVE notice and pushed to upstream and the v0.10.2
> and v0.9.11 maint branches. v0.9.6 is not vulnerable. The problem was
> introduced in 0.9.7
Hi Peter,
Looks like 0.9.6 was vulnerable since this made its way to the
v0.9.6-maint branch as well. Do you happen to know when this was
introduced?
I did some more research:
The original problem was introduced in commit 4e00b1d (libvirt 0.9.3),
when we switched over to new RPC handling; there, we only had one faulty
error path. Later, commit 3ae0ab67 (libvirt 0.9.7) exacerbated the
problem, by adding two more faulty error paths. Peter's test case when
originally reporting the CVE was on one of the error paths added in
0.9.7, hence his claim that "the problem was introduced in 0.9.7"; but I
still think it is possible to trigger the remaining faulty error path
when targeting libvirt 0.9.3, and agree with Cole's backport to the
v0.9.6-maint branch.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:19 p.m.
New subject: [libvirt] [PATCH] rpc: Fix crash on error paths of message dispatching
Eric Blake wrote:
On 01/29/2013 07:05 PM, Jim Fehlig wrote:
>>> Mention CVE-2013-0170 in the commit message, now that it is public:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=893450
>>>
>>>
>>>> * rpc/virnetserverclient.c: virNetServerClientDispatchRead:
>>>> - avoid use after free of RPC messages
>>>> ---
>>>> src/rpc/virnetserverclient.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>> ACK. Looks like we need this on {v0.10.2,v0.9.11,v0.9.6}-maint as well.
>>>
>> Thanks. I added the CVE notice and pushed to upstream and the v0.10.2
>> and v0.9.11 maint branches. v0.9.6 is not vulnerable. The problem was
>> introduced in 0.9.7
>>
> Hi Peter,
>
> Looks like 0.9.6 was vulnerable since this made its way to the
> v0.9.6-maint branch as well. Do you happen to know when this was
> introduced?
>
I did some more research:
Eric,
Thank you for the research and explanation, it was not expected but very
much appreciated. I was actually having quite the RTFM moment after
asking this question...
Regards,
Jim
The original problem was introduced in commit 4e00b1d (libvirt
0.9.3),
when we switched over to new RPC handling; there, we only had one faulty
error path. Later, commit 3ae0ab67 (libvirt 0.9.7) exacerbated the
problem, by adding two more faulty error paths. Peter's test case when
originally reporting the CVE was on one of the error paths added in
0.9.7, hence his claim that "the problem was introduced in 0.9.7"; but I
still think it is possible to trigger the remaining faulty error path
when targeting libvirt 0.9.3, and agree with Cole's backport to the
v0.9.6-maint branch.
2:22 p.m.
Hi,
On Mon, Jan 28, 2013 at 07:35:38PM +0100, Peter Krempa wrote:
When reading and dispatching of a message failed the message was
freed
but wasn't removed from the message queue.
After that when the connection was about to be closed the pointer for
the message was still present in the queue and it was passed to
virNetMessageFree which tried to call the callback function from an
uninitialized pointer.
Debian stable is shipping 0.8.2. I checked and it seems this version
isn't affected siince we properly remove the message from the queue
before looking at it in daemon/libvirtd.c. I'd be great if somebody
could double check though!
Cheers,
-- Guido
This patch removes the message from the queue before it's freed.
* rpc/virnetserverclient.c: virNetServerClientDispatchRead:
- avoid use after free of RPC messages
---
src/rpc/virnetserverclient.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index af0560e..446e1e9 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -987,6 +987,7 @@ readmore:
/* Decode the header so we can use it for routing decisions */
if (virNetMessageDecodeHeader(msg) < 0) {
+ virNetMessageQueueServe(&client->rx);
virNetMessageFree(msg);
client->wantClose = true;
return;
@@ -996,6 +997,7 @@ readmore:
* file descriptors */
if (msg->header.type == VIR_NET_CALL_WITH_FDS &&
virNetMessageDecodeNumFDs(msg) < 0) {
+ virNetMessageQueueServe(&client->rx);
virNetMessageFree(msg);
client->wantClose = true;
return; /* Error */
@@ -1005,6 +1007,7 @@ readmore:
for (i = msg->donefds ; i < msg->nfds ; i++) {
int rv;
if ((rv = virNetSocketRecvFD(client->sock, &(msg->fds[i]))) <
0) {
+ virNetMessageQueueServe(&client->rx);
virNetMessageFree(msg);
client->wantClose = true;
return;
--
1.8.1.1
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3:21 p.m.
On 01/29/2013 01:22 PM, Guido Günther wrote:
Hi,
On Mon, Jan 28, 2013 at 07:35:38PM +0100, Peter Krempa wrote:
> When reading and dispatching of a message failed the message was freed
> but wasn't removed from the message queue.
>
> After that when the connection was about to be closed the pointer for
> the message was still present in the queue and it was passed to
> virNetMessageFree which tried to call the callback function from an
> uninitialized pointer.
Debian stable is shipping 0.8.2. I checked and it seems this version
isn't affected siince we properly remove the message from the queue
before looking at it in daemon/libvirtd.c. I'd be great if somebody
could double check though!
0.8.2 predates the RPC rewrite, and I concur with your assessment that
back then, the code was _always_ clearing the queue:
v0.8.2:daemon/libvirtd.c:qemudDispatchClientRead():
/* Grab the completed message */
struct qemud_client_message *msg =
qemudClientMessageQueueServe(&client->rx);
struct qemud_client_filter *filter;
/* Decode the header so we can use it for routing decisions */
if (remoteDecodeClientMessageHeader(msg) < 0) {
VIR_FREE(msg);
qemudDispatchClientFailure(client);
}
However, it does look like there might be a missing 'return' statement
after that error is reported, especially given that the next error
reporting a few lines later does an early return.
But the best way to determine if this version is actually vulnerable to
the CVE would be trying the exploit, and seeing if libvirtd survives
with proper error logging about an invalid client request; Peter may
have more details on how best to attempt that (although it may be better
to discuss those details off-list, even if the CVE is already public, so
that others are less likely to maliciously use the exploit).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:18 a.m.
Hi Eric,
On Tue, Jan 29, 2013 at 02:21:30PM -0700, Eric Blake wrote:
On 01/29/2013 01:22 PM, Guido Günther wrote:
> Hi,
> On Mon, Jan 28, 2013 at 07:35:38PM +0100, Peter Krempa wrote:
>> When reading and dispatching of a message failed the message was freed
>> but wasn't removed from the message queue.
>>
>> After that when the connection was about to be closed the pointer for
>> the message was still present in the queue and it was passed to
>> virNetMessageFree which tried to call the callback function from an
>> uninitialized pointer.
>
> Debian stable is shipping 0.8.2. I checked and it seems this version
> isn't affected siince we properly remove the message from the queue
> before looking at it in daemon/libvirtd.c. I'd be great if somebody
> could double check though!
0.8.2 predates the RPC rewrite, and I concur with your assessment that
back then, the code was _always_ clearing the queue:
v0.8.2:daemon/libvirtd.c:qemudDispatchClientRead():
/* Grab the completed message */
struct qemud_client_message *msg =
qemudClientMessageQueueServe(&client->rx);
struct qemud_client_filter *filter;
/* Decode the header so we can use it for routing decisions */
if (remoteDecodeClientMessageHeader(msg) < 0) {
VIR_FREE(msg);
qemudDispatchClientFailure(client);
}
However, it does look like there might be a missing 'return' statement
after that error is reported, especially given that the next error
reporting a few lines later does an early return.
Thanks for double checking. It indeed looks like there's a return
missing (cc:'ing the Debian bugreport to make this information permanent
there too).
Cheers,
-- Guido
But the best way to determine if this version is actually vulnerable
to
the CVE would be trying the exploit, and seeing if libvirtd survives
with proper error logging about an invalid client request; Peter may
have more details on how best to attempt that (although it may be better
to discuss those details off-list, even if the CVE is already public, so
that others are less likely to maliciously use the exploit).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4310
days inactive
4316
days old
9 comments
5 participants
participants (5)
-
Cole Robinson -
Eric Blake -
Guido Günther -
Jim Fehlig -
Peter Krempa