Ensure we don't read past the end of packet buffers

assignment-client/src/avatars/AvatarMixerClientData.cpp

@@ -155,6 +155,12 @@ int AvatarMixerClientData::parseData(ReceivedMessage& message, const SlaveShared
 void AvatarMixerClientData::processSetTraitsMessage(ReceivedMessage& message,
                                                     const SlaveSharedData& slaveSharedData,
                                                     Node& sendingNode) {
+    // Trying to read more bytes than available, bail
+    if (message.getBytesLeftToRead() < qint64(sizeof(AvatarTraits::TraitVersion))) {
+        qWarning() << "Refusing to process malformed traits packet from" << message.getSenderSockAddr();
+        return;
+    }
+
     // pull the trait version from the message
     AvatarTraits::TraitVersion packetTraitVersion;
     message.readPrimitive(&packetTraitVersion);
@@ -164,10 +170,22 @@ void AvatarMixerClientData::processSetTraitsMessage(ReceivedMessage& message,
     while (message.getBytesLeftToRead() > 0) {
         // for each trait in the packet, apply it if the trait version is newer than what we have
 
+        // Trying to read more bytes than available, bail
+        if (message.getBytesLeftToRead() < qint64(sizeof(AvatarTraits::TraitType))) {
+            qWarning() << "Refusing to process malformed traits packet from" << message.getSenderSockAddr();
+            return;
+        }
+
         AvatarTraits::TraitType traitType;
         message.readPrimitive(&traitType);
 
         if (AvatarTraits::isSimpleTrait(traitType)) {
+            // Trying to read more bytes than available, bail
+            if (message.getBytesLeftToRead() < qint64(sizeof(AvatarTraits::TraitWireSize))) {
+                qWarning() << "Refusing to process malformed traits packet from" << message.getSenderSockAddr();
+                return;
+            }
+
             AvatarTraits::TraitWireSize traitSize;
             message.readPrimitive(&traitSize);
 
@@ -190,13 +208,15 @@ void AvatarMixerClientData::processSetTraitsMessage(ReceivedMessage& message,
                 message.seek(message.getPosition() + traitSize);
             }
         } else {
-            AvatarTraits::TraitInstanceID instanceID = QUuid::fromRfc4122(message.readWithoutCopy(NUM_BYTES_RFC4122_UUID));
-
-            if (message.getBytesLeftToRead() == 0) {
-                qWarning() << "Received an instanced trait with no size from" << message.getSenderSockAddr();
-                break;
+            // Trying to read more bytes than available, bail
+            if (message.getBytesLeftToRead() < qint64(NUM_BYTES_RFC4122_UUID +
+                                                       sizeof(AvatarTraits::TraitWireSize))) {
+                qWarning() << "Refusing to process malformed traits packet from" << message.getSenderSockAddr();
+                return;
             }
 
+            AvatarTraits::TraitInstanceID instanceID = QUuid::fromRfc4122(message.readWithoutCopy(NUM_BYTES_RFC4122_UUID));
+
             AvatarTraits::TraitWireSize traitSize;
             message.readPrimitive(&traitSize);
 

libraries/avatars/src/AvatarHashMap.cpp

@@ -332,6 +332,12 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer<ReceivedMessage>
 void AvatarHashMap::processBulkAvatarTraits(QSharedPointer<ReceivedMessage> message, SharedNodePointer sendingNode) {
     AvatarTraits::TraitMessageSequence seq;
 
+    // Trying to read more bytes than available, bail
+    if (message->getBytesLeftToRead() < (qint64)sizeof(AvatarTraits::TraitMessageSequence)) {
+        qWarning() << "Malformed bulk trait packet, bailling";
+        return;
+    }
+
     message->readPrimitive(&seq);
 
     auto traitsAckPacket = NLPacket::create(PacketType::BulkAvatarTraitsAck, sizeof(AvatarTraits::TraitMessageSequence), true);
@@ -344,7 +350,14 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer<ReceivedMessage> mess
         nodeList->sendPacket(std::move(traitsAckPacket), *avatarMixer);
     }
 
-    while (message->getBytesLeftToRead()) {
+    while (message->getBytesLeftToRead() > 0) {
+        // Trying to read more bytes than available, bail
+        if (message->getBytesLeftToRead() < qint64(NUM_BYTES_RFC4122_UUID +
+                                                   sizeof(AvatarTraits::TraitType))) {
+            qWarning() << "Malformed bulk trait packet, bailling";
+            return;
+        }
+
         // read the avatar ID to figure out which avatar this is for
         auto avatarID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID));
 
@@ -360,6 +373,12 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer<ReceivedMessage> mess
         auto& lastProcessedVersions = _processedTraitVersions[avatarID];
 
         while (traitType != AvatarTraits::NullTrait && message->getBytesLeftToRead() > 0) {
+            // Trying to read more bytes than available, bail
+            if (message->getBytesLeftToRead() < qint64(sizeof(AvatarTraits::TraitVersion))) {
+                qWarning() << "Malformed bulk trait packet, bailling";
+                return;
+            }
+
             AvatarTraits::TraitVersion packetTraitVersion;
             message->readPrimitive(&packetTraitVersion);
 
@@ -367,8 +386,20 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer<ReceivedMessage> mess
             bool skipBinaryTrait = false;
 
             if (AvatarTraits::isSimpleTrait(traitType)) {
+                // Trying to read more bytes than available, bail
+                if (message->getBytesLeftToRead() < qint64(sizeof(AvatarTraits::TraitWireSize))) {
+                    qWarning() << "Malformed bulk trait packet, bailling";
+                    return;
+                }
+
                 message->readPrimitive(&traitBinarySize);
 
+                // Trying to read more bytes than available, bail
+                if (message->getBytesLeftToRead() < traitBinarySize) {
+                    qWarning() << "Malformed bulk trait packet, bailling";
+                    return;
+                }
+
                 // check if this trait version is newer than what we already have for this avatar
                 if (packetTraitVersion > lastProcessedVersions[traitType]) {
                     auto traitData = message->read(traitBinarySize);
@@ -379,11 +410,24 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer<ReceivedMessage> mess
                     skipBinaryTrait = true;
                 }
             } else {
+                // Trying to read more bytes than available, bail
+                if (message->getBytesLeftToRead() < qint64(NUM_BYTES_RFC4122_UUID +
+                                                           sizeof(AvatarTraits::TraitWireSize))) {
+                    qWarning() << "Malformed bulk trait packet, bailling";
+                    return;
+                }
+
                 AvatarTraits::TraitInstanceID traitInstanceID =
                     QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID));
 
                 message->readPrimitive(&traitBinarySize);
 
+                // Trying to read more bytes than available, bail
+                if (traitBinarySize < -1 || message->getBytesLeftToRead() < traitBinarySize) {
+                    qWarning() << "Malformed bulk trait packet, bailling";
+                    return;
+                }
+
                 auto& processedInstanceVersion = lastProcessedVersions.getInstanceValueRef(traitType, traitInstanceID);
                 if (packetTraitVersion > processedInstanceVersion) {
                     if (traitBinarySize == AvatarTraits::DELETED_TRAIT_SIZE) {

libraries/avatars/src/ClientTraitsHandler.cpp

@@ -147,7 +147,16 @@ int ClientTraitsHandler::sendChangedTraitsToMixer() {
 void ClientTraitsHandler::processTraitOverride(QSharedPointer<ReceivedMessage> message, SharedNodePointer sendingNode) {
     if (sendingNode->getType() == NodeType::AvatarMixer) {
         Lock lock(_traitLock);
-        while (message->getBytesLeftToRead()) {
+
+        while (message->getBytesLeftToRead() > 0) {
+            // Trying to read more bytes than available, bail
+            if (message->getBytesLeftToRead() < qint64(sizeof(AvatarTraits::TraitType) +
+                                                       sizeof(AvatarTraits::TraitVersion) +
+                                                       sizeof(AvatarTraits::TraitWireSize))) {
+                qWarning() << "Malformed trait override packet, bailling";
+                return;
+            }
+
             AvatarTraits::TraitType traitType;
             message->readPrimitive(&traitType);
 
@@ -157,6 +166,12 @@ void ClientTraitsHandler::processTraitOverride(QSharedPointer<ReceivedMessage> m
             AvatarTraits::TraitWireSize traitBinarySize;
             message->readPrimitive(&traitBinarySize);
 
+            // Trying to read more bytes than available, bail
+            if (traitBinarySize < -1 || message->getBytesLeftToRead() < traitBinarySize) {
+                qWarning() << "Malformed trait override packet, bailling";
+                return;
+            }
+
             // only accept an override if this is for a trait type we override
             // and the version matches what we last sent for skeleton
             if (traitType == AvatarTraits::SkeletonModelURL

libraries/networking/src/ReceivedMessage.cpp

@@ -9,9 +9,10 @@
 //  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
 //
 
-
 #include "ReceivedMessage.h"
 
+#include <algorithm>
+
 #include "QSharedPointer"
 
 int receivedMessageMetaTypeId = qRegisterMetaType<ReceivedMessage*>("ReceivedMessage*");
@@ -83,20 +84,26 @@ void ReceivedMessage::appendPacket(NLPacket& packet) {
 }
 
 qint64 ReceivedMessage::peek(char* data, qint64 size) {
-    memcpy(data, _data.constData() + _position, size);
-    return size;
+    size_t bytesLeft = _data.size() - _position;
+    size_t sizeRead = std::min((size_t)size, bytesLeft);
+    memcpy(data, _data.constData() + _position, sizeRead);
+    return sizeRead;
 }
 
 qint64 ReceivedMessage::read(char* data, qint64 size) {
-    memcpy(data, _data.constData() + _position, size);
-    _position += size;
-    return size;
+    size_t bytesLeft = _data.size() - _position;
+    size_t sizeRead = std::min((size_t)size, bytesLeft);
+    memcpy(data, _data.constData() + _position, sizeRead);
+    _position += sizeRead;
+    return sizeRead;
 }
 
 qint64 ReceivedMessage::readHead(char* data, qint64 size) {
-    memcpy(data, _headData.constData() + _position, size);
-    _position += size;
-    return size;
+    size_t bytesLeft = _headData.size() - _position;
+    size_t sizeRead = std::min((size_t)size, bytesLeft);
+    memcpy(data, _headData.constData() + _position, sizeRead);
+    _position += sizeRead;
+    return sizeRead;
 }
 
 QByteArray ReceivedMessage::peek(qint64 size) {