Replace invalid characters in WebChannel messages

Turns out JavaScript's JSON.stringify is not guaranteed to produce valid UTF-16
strings. It is possible in JavaScript to produce string objects which contain
invalid code units (unmatched surrogate pairs) and JSON.stringify will simply
copy this data to it's output. However, such a string cannot be losslessly
converted to UTF-8 and this leads to fun errors in WebChannelIPCTransport.

This patch

  - Adds a test for the scenario above.

  - Changes WebChannelIPCTransport to replace these invalid code units with the
    Unicode replacement character U+FFFD.

  - Changes WebChannelIPCTransportHost to validate the data it gets from the
    renderer. Not validating the data defeats the whole point of Chromium's
    fancy multi-process architecture: the renderer is not to be trusted.

  - Changes WebChannelIPCTransport to throw JavaScript exceptions for various
    errors (missing argument, wrong type, invalid JSON). Seems like the polite
    thing to do.

Task-number: QTBUG-61969
Change-Id: I83275a0eaed77109dc458b80e27217108dde9f7b
Reviewed-by: Michal Klocek <michal.klocek@qt.io>

5 files changed, 69 insertions, 18 deletions

diff --git a/src/core/renderer/web_channel_ipc_transport.cpp b/src/core/renderer/web_channel_ipc_transport.cpp
index bb544168f..ef00bcef3 100644
--- a/src/core/renderer/web_channel_ipc_transport.cpp
+++ b/src/core/renderer/web_channel_ipc_transport.cpp
@@ -64,7 +64,7 @@ public:
     static void Uninstall(blink::WebLocalFrame *frame, uint worldId);
 private:
     WebChannelTransport() {}
-    bool NativeQtSendMessage(gin::Arguments *args);
+    void NativeQtSendMessage(gin::Arguments *args);
 
     // gin::WrappableBase
     gin::ObjectTemplateBuilder GetObjectTemplateBuilder(v8::Isolate *isolate) override;
@@ -118,37 +118,45 @@ void WebChannelTransport::Uninstall(blink::WebLocalFrame *frame, uint worldId)
     qtObject->Delete(gin::StringToV8(isolate, "webChannelTransport"));
 }
 
-bool WebChannelTransport::NativeQtSendMessage(gin::Arguments *args)
+void WebChannelTransport::NativeQtSendMessage(gin::Arguments *args)
 {
     blink::WebLocalFrame *frame = blink::WebLocalFrame::FrameForCurrentContext();
     if (!frame || !frame->View())
-        return false;
+        return;
 
     content::RenderFrame *renderFrame = content::RenderFrame::FromWebFrame(frame);
     if (!renderFrame)
-        return false;
+        return;
+
+    v8::Local<v8::Value> jsonValue;
+    if (!args->GetNext(&jsonValue)) {
+        args->ThrowTypeError("Missing argument");
+        return;
+    }
 
-    std::string message;
-    if (!args->GetNext(&message))
-        return false;
+    if (!jsonValue->IsString()) {
+        args->ThrowTypeError("Expected string");
+        return;
+    }
+    v8::Local<v8::String> jsonString = v8::Local<v8::String>::Cast(jsonValue);
+
+    QByteArray json(jsonString->Utf8Length(), 0);
+    jsonString->WriteUtf8(json.data(), json.size(),
+                         nullptr,
+                         v8::String::REPLACE_INVALID_UTF8);
 
-    QByteArray valueData(message.data(), message.size());
     QJsonParseError error;
-    QJsonDocument doc = QJsonDocument::fromJson(valueData, &error);
+    QJsonDocument doc = QJsonDocument::fromJson(json, &error);
     if (error.error != QJsonParseError::NoError) {
-        LOG(WARNING) << "Parsing error: " << qPrintable(error.errorString());
-        return false;
+        args->ThrowTypeError("Invalid JSON");
+        return;
     }
 
     int size = 0;
     const char *rawData = doc.rawData(&size);
-    if (size == 0)
-        return false;
-
     renderFrame->Send(new WebChannelIPCTransportHost_SendMessage(
                           renderFrame->GetRoutingID(),
                           std::vector<char>(rawData, rawData + size)));
-    return true;
 }
 
 gin::ObjectTemplateBuilder WebChannelTransport::GetObjectTemplateBuilder(v8::Isolate *isolate)
diff --git a/src/core/renderer_host/web_channel_ipc_transport_host.cpp b/src/core/renderer_host/web_channel_ipc_transport_host.cpp
index 6b32093a6..d99dfde97 100644
--- a/src/core/renderer_host/web_channel_ipc_transport_host.cpp
+++ b/src/core/renderer_host/web_channel_ipc_transport_host.cpp
@@ -49,6 +49,8 @@
 #include <QJsonObject>
 #include <QLoggingCategory>
 
+#include <QtCore/private/qjson_p.h>
+
 namespace QtWebEngineCore {
 
 Q_LOGGING_CATEGORY(log, "qt.webengine.webchanneltransport");
@@ -108,10 +110,19 @@ void WebChannelIPCTransportHost::setWorldId(content::RenderFrameHost *frame, bas
 
 void WebChannelIPCTransportHost::onWebChannelMessage(const std::vector<char> &message)
 {
-    Q_ASSERT(!message.empty());
-    QJsonDocument doc = QJsonDocument::fromRawData(message.data(), message.size(), QJsonDocument::BypassValidation);
-    Q_ASSERT(doc.isObject());
     content::RenderFrameHost *frame = web_contents()->GetMainFrame();
+
+    QJsonDocument doc;
+    // QJsonDocument::fromRawData does not check the length before it starts
+    // parsing the QJsonPrivate::Header and QJsonPrivate::Base structures.
+    if (message.size() >= sizeof(QJsonPrivate::Header) + sizeof(QJsonPrivate::Base))
+        doc = QJsonDocument::fromRawData(message.data(), message.size());
+
+    if (!doc.isObject()) {
+        qCCritical(log).nospace() << "received invalid webchannel message from " << frame;
+        return;
+    }
+
     qCDebug(log).nospace() << "received webchannel message from " << frame << ": " << doc;
     Q_EMIT messageReceived(doc.object(), this);
 }
diff --git a/tests/auto/widgets/qwebenginescript/resources/webChannelWithBadString.html b/tests/auto/widgets/qwebenginescript/resources/webChannelWithBadString.html
new file mode 100644
index 000000000..af40f6a2b
--- /dev/null
+++ b/tests/auto/widgets/qwebenginescript/resources/webChannelWithBadString.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <title>webChannelWithBadString</title>
+    </head>
+    <body>
+        <script src="/qwebchannel.js"></script>
+        <script type="text/javascript">
+         new QWebChannel(qt.webChannelTransport, (channel) => {
+             channel.objects.host.text = String.fromCharCode(0xD800);
+         });
+        </script>
+    </body>
+</html>
diff --git a/tests/auto/widgets/qwebenginescript/tst_qwebenginescript.cpp b/tests/auto/widgets/qwebenginescript/tst_qwebenginescript.cpp
index cb45e524e..a9efabf97 100644
--- a/tests/auto/widgets/qwebenginescript/tst_qwebenginescript.cpp
+++ b/tests/auto/widgets/qwebenginescript/tst_qwebenginescript.cpp
@@ -42,6 +42,7 @@ private Q_SLOTS:
     void webChannelResettingAndUnsetting();
     void webChannelWithExistingQtObject();
     void navigation();
+    void webChannelWithBadString();
 };
 
 void tst_QWebEngineScript::domEditing()
@@ -470,6 +471,22 @@ void tst_QWebEngineScript::navigation()
     QCOMPARE(testObject.text(), url3);
 }
 
+// Try to set TestObject::text to an invalid UTF-16 string.
+//
+// See QTBUG-61969.
+void tst_QWebEngineScript::webChannelWithBadString()
+{
+    QWebEnginePage page;
+    TestObject host;
+    QSignalSpy hostSpy(&host, &TestObject::textChanged);
+    QWebChannel channel;
+    channel.registerObject(QStringLiteral("host"), &host);
+    page.setWebChannel(&channel);
+    page.setUrl(QStringLiteral("qrc:/resources/webChannelWithBadString.html"));
+    QVERIFY(hostSpy.wait(20000));
+    QCOMPARE(host.text(), QString(QChar(QChar::ReplacementCharacter)));
+}
+
 QTEST_MAIN(tst_QWebEngineScript)
 
 #include "tst_qwebenginescript.moc"
diff --git a/tests/auto/widgets/qwebenginescript/tst_qwebenginescript.qrc b/tests/auto/widgets/qwebenginescript/tst_qwebenginescript.qrc
index 9960a37ba..ada06119a 100644
--- a/tests/auto/widgets/qwebenginescript/tst_qwebenginescript.qrc
+++ b/tests/auto/widgets/qwebenginescript/tst_qwebenginescript.qrc
@@ -4,5 +4,6 @@
     <file>resources/test_iframe_outer.html</file>
     <file>resources/test_iframe_inner.html</file>
     <file>resources/test_window_open.html</file>
+    <file>resources/webChannelWithBadString.html</file>
 </qresource>
 </RCC>