[SECURITY] Check the basic messages for validity (announce/invoke) be…

…fore passing on. (#1383) There was no validity check on the size of the message of a COMRPC messages before it was passed on for further parsing. This meant that if a message was to short, in debug, the ASSSERT of the further down the line parsing, would fail as the message did not have the proper length to extract certain pieces of information from it. These issues, and more might follow, are the result of QA testing with malformed COMRPC messages.
rdkcentral · Jul 31, 2023 · 9407982 · 9407982
1 parent 6b36786
commit 9407982
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 32 deletions.
diff --git a/Source/com/Administrator.h b/Source/com/Administrator.h
@@ -372,9 +372,13 @@ namespace RPC {
         {
             Core::ProxyType<InvokeMessage> message(data);
             ASSERT(message.IsValid() == true);
-            _administrator.Invoke(channel, message);
-            channel->ReportResponse(data);
-
+            if (message->Parameters().IsValid() == false) {
+                SYSLOG(Logging::Error, (_T("COMRPC Announce message incorrectly formatted!")));
+            }
+            else {
+                _administrator.Invoke(channel, message);
+                channel->ReportResponse(data);
+            }
         }
 
     private:

diff --git a/Source/com/Communicator.h b/Source/com/Communicator.h
@@ -1321,26 +1321,33 @@ namespace RPC {
 
             public:
                 void Procedure(Core::IPCChannel& channel, Core::ProxyType<Core::IIPC>& data) override {
+
                     Core::ProxyType<AnnounceMessage> message(data);
 
                     ASSERT(message.IsValid() == true);
                     ASSERT(dynamic_cast<Client*>(&channel) != nullptr);
 
-                    Core::ProxyType<Client> proxyChannel(static_cast<Client&>(channel));
+                    // Validate if the frame coming in is properly formatted..
+                    if (message->Parameters().IsValid() == false) {
+                        SYSLOG(Logging::Error, (_T("COMRPC Announce message incorrectly formatted!")));
+                    }
+                    else {
+                        Core::ProxyType<Client> proxyChannel(static_cast<Client&>(channel));
 
-                    string jsonDefaultMessagingSettings;
-                    string jsonDefaultWarningReportingSettings;
+                        string jsonDefaultMessagingSettings;
+                        string jsonDefaultWarningReportingSettings;
 
-                    #if defined(WARNING_REPORTING_ENABLED)
-                    jsonDefaultWarningReportingSettings = WarningReporting::WarningReportingUnit::Instance().Defaults();
-                    #endif
+                        #if defined(WARNING_REPORTING_ENABLED)
+                        jsonDefaultWarningReportingSettings = WarningReporting::WarningReportingUnit::Instance().Defaults();
+                        #endif
 
-                    void* result = _parent.Announce(proxyChannel, message->Parameters(), message->Response());
+                        void* result = _parent.Announce(proxyChannel, message->Parameters(), message->Response());
 
-                    message->Response().Set(instance_cast<void*>(result), proxyChannel->Extension().Id(), _parent.ProxyStubPath(), jsonDefaultMessagingSettings, jsonDefaultWarningReportingSettings);
+                        message->Response().Set(instance_cast<void*>(result), proxyChannel->Extension().Id(), _parent.ProxyStubPath(), jsonDefaultMessagingSettings, jsonDefaultWarningReportingSettings);
 
-                    // We are done, report completion
-                    channel.ReportResponse(data);
+                        // We are done, report completion
+                        channel.ReportResponse(data);
+                    }
                 }
 
             private:

diff --git a/Source/com/Messages.h b/Source/com/Messages.h
@@ -78,6 +78,9 @@ namespace RPC {
             ~Input() = default;
 
         public:
+            inline bool IsValid() const {
+                return (_data.Size() >= (sizeof(Core::instance_id) + sizeof(uint32_t) + sizeof(uint8_t)));
+            }
             inline void Clear()
             {
                 _data.Clear();
@@ -276,6 +279,9 @@ namespace RPC {
             }
 
         public:
+            bool IsValid() const {
+                return (_data.Size() >= STRINGS_OFFSET + 2);
+            }
             bool IsOffer() const
             {
                 return (Type() == OFFER);

diff --git a/Source/core/IPCConnector.h b/Source/core/IPCConnector.h
@@ -198,32 +198,35 @@ namespace Core {
                         }
                     }
 
-                    ASSERT((_offset - 8) <= _length);
+                    if (_offset >= 8) {
 
-                    if ((_offset - 8) < _length) {
+                        ASSERT((_offset - 8) <= _length);
 
-                        // There could be multiple packages in this frame, do not read/handle more than what fits in the frame.
-                        uint32_t tmp = _length - (_offset - 8);
-                        uint16_t handled(static_cast<uint32_t>(maxLength - result) > tmp ? static_cast<uint16_t>(tmp) : (maxLength - result));
+                        if ((_offset - 8) < _length) {
 
-                        if (_current != nullptr) {
-                            handled = _current->Deserialize(&stream[result], handled, _offset - 8);
-                        }
+                            // There could be multiple packages in this frame, do not read/handle more than what fits in the frame.
+                            uint32_t tmp = _length - (_offset - 8);
+                            uint16_t handled(static_cast<uint32_t>(maxLength - result) > tmp ? static_cast<uint16_t>(tmp) : (maxLength - result));
 
-                        _offset += handled;
-                        result += handled;
-                    }
+                            if (_current != nullptr) {
+                                handled = _current->Deserialize(&stream[result], handled, _offset - 8);
+                            }
 
-                    ASSERT((_offset - 8) <= _length);
+                            _offset += handled;
+                            result += handled;
+                        }
 
-                    if ((_offset - 8) == _length) {
-                        if (_current != nullptr) {
-                            IMessage* ready = _current;
-                            _current = nullptr;
-                            Deserialized(*ready);
+                        ASSERT((_offset - 8) <= _length);
+
+                        if ((_offset - 8) == _length) {
+                            if (_current != nullptr) {
+                                IMessage* ready = _current;
+                                _current = nullptr;
+                                Deserialized(*ready);
+                            }
+                            _offset = 0;
+                            _length = 0;
                         }
-                        _offset = 0;
-                        _length = 0;
                     }
                 }
                 return (result);