In DnD transport V3 check packet and payload size to prevent OOB read

or write.

diff --git a/open-vm-tools/services/plugins/dndcp/dnd/dndCommon.c b/open-vm-tools/services/plugins/dndcp/dnd/dndCommon.c
index a5e7361bd835c32152b65a79ef53c23415c06a1e..c6e1eb8ea20e17d9baa97190d6ef020d5096a7d1 100644 (file)
--- a/open-vm-tools/services/plugins/dndcp/dnd/dndCommon.c
+++ b/open-vm-tools/services/plugins/dndcp/dnd/dndCommon.c
@@ -696,6 +696,8 @@ DnD_TransportBufGetPacket(DnDTransportBuffer *buf,           // IN/OUT
  * DnD_TransportBufAppendPacket --
  *
  *    Put a received packet into transport layer buffer.
+ *    This function should be called after validate the packet and packetSize!
+ *    See: RpcV3Util::OnRecvPacket()
  *
  * Results:
  *    TRUE if success, FALSE otherwise.
@@ -712,17 +714,6 @@ DnD_TransportBufAppendPacket(DnDTransportBuffer *buf,          // IN/OUT
                              size_t packetSize)                // IN
 {
    ASSERT(buf);
-   ASSERT(packetSize == (packet->payloadSize + DND_TRANSPORT_PACKET_HEADER_SIZE) &&
-          packetSize <= DND_MAX_TRANSPORT_PACKET_SIZE &&
-          (packet->payloadSize + packet->offset) <= packet->totalSize &&
-          packet->totalSize <= DNDMSG_MAX_ARGSZ);
-
-   if (packetSize != (packet->payloadSize + DND_TRANSPORT_PACKET_HEADER_SIZE) ||
-       packetSize > DND_MAX_TRANSPORT_PACKET_SIZE ||
-       (packet->payloadSize + packet->offset) > packet->totalSize ||
-       packet->totalSize > DNDMSG_MAX_ARGSZ) {
-      goto error;
-   }
 
    /*
     * If seqNum does not match, it means either this is the first packet, or there

diff --git a/open-vm-tools/services/plugins/dndcp/dndGuest/rpcV3Util.cpp b/open-vm-tools/services/plugins/dndcp/dndGuest/rpcV3Util.cpp
index 973f683fc582ed4afcb57baf9dce014e6dcf91a0..af6624a1151ade71f14dea22743febbd5ffd6a2b 100644 (file)
--- a/open-vm-tools/services/plugins/dndcp/dndGuest/rpcV3Util.cpp
+++ b/open-vm-tools/services/plugins/dndcp/dndGuest/rpcV3Util.cpp
@@ -305,18 +305,16 @@ RpcV3Util::OnRecvPacket(uint32 srcId,
 {
    DnDTransportPacketHeader *packetV3 = (DnDTransportPacketHeader *)packet;
    ASSERT(packetV3);
-   if (packetSize <= 0 ||
-       packetSize != (packetV3->payloadSize + DND_TRANSPORT_PACKET_HEADER_SIZE) ||
-       packetSize > DND_MAX_TRANSPORT_PACKET_SIZE) {
-      LOG(0, ("%s: Received invalid data.\n", __FUNCTION__));
-      return;
+   if (packetSize <= 0 || packetSize > DND_MAX_TRANSPORT_PACKET_SIZE ||
+       packetV3->payloadSize > DND_MAX_TRANSPORT_PACKET_PAYLOAD_SIZE ||
+       (packetV3->payloadSize + DND_TRANSPORT_PACKET_HEADER_SIZE) != packetSize) {
+      goto invalid_packet;
    }
 
    switch (packetV3->type) {
    case DND_TRANSPORT_PACKET_TYPE_SINGLE:
       if (packetV3->payloadSize != packetV3->totalSize) {
-         LOG(0, ("%s: received invalid packet.\n", __FUNCTION__));
-         return;
+         goto invalid_packet;
       }
       /* This is a single packet. Forward to rpc layer for further processing. */
       mRpc->HandleMsg(NULL, packetV3->payload, packetV3->payloadSize);
@@ -356,6 +354,32 @@ RpcV3Util::OnRecvPacket(uint32 srcId,
          break;
       }
    case DND_TRANSPORT_PACKET_TYPE_PAYLOAD:
+      /*
+       * If seqNum does not match, it means either this is the first packet, or there
+       * is a timeout in another side. The buffer will be reset in all cases later.
+       * For the first packet, the totalSize should not larger than DNDMSG_MAX_ARGSZ.
+       * For the rest packets, the totalSize should be the same as the first packet.
+       */
+      if (mRecvBuf.seqNum != packetV3->seqNum) {
+         if (packetV3->totalSize > DNDMSG_MAX_ARGSZ) {
+            goto invalid_packet;
+         }
+      } else {
+         if (packetV3->totalSize != mRecvBuf.totalSize) {
+            goto invalid_packet;
+         }
+      }
+
+      /*
+       * The totalSize has been validated.
+       * We need to make sure the  payloadSize and offset are in right range.
+       */
+      if (packetV3->payloadSize > packetV3->totalSize ||
+          packetV3->offset > packetV3->totalSize ||
+          (packetV3->payloadSize + packetV3->offset) > packetV3->totalSize) {
+         goto invalid_packet;
+      }
+
       /* Received next packet for big binary buffer. */
       if (!DnD_TransportBufAppendPacket(&mRecvBuf, packetV3, packetSize)) {
          LOG(0, ("%s: DnD_TransportBufAppendPacket failed.\n", __FUNCTION__));
@@ -391,6 +415,9 @@ RpcV3Util::OnRecvPacket(uint32 srcId,
       LOG(0, ("%s: unknown packet.\n", __FUNCTION__));
       break;
    }
+
+invalid_packet:
+   LOG(0, ("%s: received invalid data.\n", __FUNCTION__));
 }