From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Fri, 2 Dec 2022 08:16:55 +0000 (+0100)
Subject: Properly encode json string containing binary data
X-Git-Tag: rec-4.8.2~9^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F12344%2Fhead;p=thirdparty%2Fpdns.git

Properly encode json string containing binary data

The existing code assumes the strings are alreayd valid UTF8 and contain potential out-of-bound accesses.

Also urlEncode path in log lines, as it trips pytest.xml:

Running tests...

$ 'pytest' '--junitxml=pytest.xml' '-v'
==STDOUT===

==STDERRR===
  File "/home/otto/pdns/regression-tests.api/runtests.py", line 304, in <module>
    print(serverproc.stderr.read())
  File "/usr/lib/python3.9/codecs.py", line 322, in decode
    (result, consumed) = self._buffer_decode(data, self.errors, final)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xeb in position 4304: invalid continuation byte

There might be more places where this is needed.

(cherry picked from commit 1478a2c8713535e4cbd1943e2526e3527d58a19b)
---

diff --git a/ext/json11/json11.cpp b/ext/json11/json11.cpp
index f48833b1b8..b1da1fca2a 100644
--- a/ext/json11/json11.cpp
+++ b/ext/json11/json11.cpp
@@ -93,18 +93,10 @@ static void dump(const string &value, string &out) {
             out += "\\r";
         } else if (ch == '\t') {
             out += "\\t";
-        } else if (static_cast<uint8_t>(ch) <= 0x1f) {
+        } else if (static_cast<uint8_t>(ch) <= 0x1f || static_cast<uint8_t>(ch) >= 0x7f) {
             char buf[8];
             snprintf(buf, sizeof buf, "\\u%04x", ch);
             out += buf;
-        } else if (static_cast<uint8_t>(ch) == 0xe2 && static_cast<uint8_t>(value[i+1]) == 0x80
-                   && static_cast<uint8_t>(value[i+2]) == 0xa8) {
-            out += "\\u2028";
-            i += 2;
-        } else if (static_cast<uint8_t>(ch) == 0xe2 && static_cast<uint8_t>(value[i+1]) == 0x80
-                   && static_cast<uint8_t>(value[i+2]) == 0xa9) {
-            out += "\\u2029";
-            i += 2;
         } else {
             out += ch;
         }
diff --git a/pdns/webserver.cc b/pdns/webserver.cc
index 78f83b0241..874e7cfac6 100644
--- a/pdns/webserver.cc
+++ b/pdns/webserver.cc
@@ -535,7 +535,7 @@ void WebServer::serveConnection(const std::shared_ptr<Socket>& client) const {
   }
 
   if (d_loglevel >= WebServer::LogLevel::Normal) {
-    SLOG(g_log<<Logger::Notice<<logprefix<<remote<<" \""<<req.method<<" "<<req.url.path<<" HTTP/"<<req.versionStr(req.version)<<"\" "<<resp.status<<" "<<reply.size()<<endl,
+    SLOG(g_log<<Logger::Notice<<logprefix<<remote<<" \""<<req.method<<" "<<YaHTTP::Utility::encodeURL(req.url.path)<<" HTTP/"<<req.versionStr(req.version)<<"\" "<<resp.status<<" "<<reply.size()<<endl,
          d_slog->info(Logr::Info, "Request", "remote", Logging::Loggable(remote), "method", Logging::Loggable(req.method),
                       "urlpath", Logging::Loggable(req.url.path), "HTTPVersion", Logging::Loggable(req.versionStr(req.version)),
                       "status", Logging::Loggable(resp.status), "respsize",  Logging::Loggable(reply.size())));
diff --git a/regression-tests.api/test_Servers.py b/regression-tests.api/test_Servers.py
index 47122ebb15..c9f59d19c1 100644
--- a/regression-tests.api/test_Servers.py
+++ b/regression-tests.api/test_Servers.py
@@ -2,6 +2,7 @@ import json
 import operator
 import requests
 import unittest
+import socket
 from test_helper import ApiTestCase, is_auth, is_recursor, is_auth_lmdb
 
 
@@ -41,13 +42,18 @@ class Servers(ApiTestCase):
         self.assertIn('daemon', data)
 
     def test_read_statistics(self):
+        # Use low-level API as we want to create an invalid request to test log line encoding
+        sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM);
+        sock.connect((self.server_address, self.server_port))
+        sock.send(b'GET /binary\x00\x01\xeb HTTP/1.0\r\n')
+        sock.close()
         r = self.session.get(self.url("/api/v1/servers/localhost/statistics"))
         self.assert_success_json(r)
         data = r.json()
         self.assertIn('uptime', [e['name'] for e in data])
         print(data)
         if is_auth():
-            qtype_stats, respsize_stats, queries_stats, rcode_stats = None, None, None, None
+            qtype_stats, respsize_stats, queries_stats, rcode_stats, logmessages = None, None, None, None, None
             for elem in data:
                 if elem['type'] == 'MapStatisticItem' and elem['name'] == 'response-by-qtype':
                     qtype_stats = elem['value']
@@ -57,10 +63,13 @@ class Servers(ApiTestCase):
                     queries_stats = elem['value']
                 elif elem['type'] == 'MapStatisticItem' and elem['name'] == 'response-by-rcode':
                     rcode_stats = elem['value']
+                elif elem['type'] == 'RingStatisticItem' and elem['name'] == 'logmessages':
+                    logmessages = elem['value']
             self.assertIn('A', [e['name'] for e in qtype_stats])
             self.assertIn('80', [e['name'] for e in respsize_stats])
             self.assertIn('example.com/A', [e['name'] for e in queries_stats])
             self.assertIn('No Error', [e['name'] for e in rcode_stats])
+            self.assertTrue(logmessages[0]['name'].startswith('[webserver]'))
         else:
             qtype_stats, respsize_stats, rcode_stats = None, None, None
             for elem in data: