[libvirt] [PATCH] rpc: Fix crash on error paths of message dispatching
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
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
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
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
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
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
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.
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
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
participants (5)
-
Cole Robinson -
Eric Blake -
Guido Günther -
Jim Fehlig -
Peter Krempa