Merge #10408: Net: Improvements to Tor control port parser

49a199b torcontrol: Handle escapes in Tor QuotedStrings (Jack Grigg)
0182a11 torcontrol: Log invalid parameters in Tor reply strings where meaningful (Jack Grigg)
0b6f40d torcontrol: Check for reading errors in ReadBinaryFile (Jack Grigg)
d63677b torcontrol: Fix ParseTorReplyMapping (Jack Grigg)
29f3c20 torcontrol: Add unit tests for Tor reply parsers (Jack Grigg)
d8e03c0 torcontrol: Improve comments (Jack Grigg)

Tree-SHA512: aa3ce8072d20299b38c4ba9471af7fab1f5df096c237bf40a96ee9274a357f7366f95ced0cc80f8da1f22f6455a1a8e68bad9a5ff71817eef3397b6aefcbc7ae

3 files changed, 275 insertions, 7 deletions

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 10cb7e775a..ee1c11ff1f 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -78,6 +78,7 @@ BITCOIN_TESTS =\
   test/testutil.cpp \
   test/testutil.h \
   test/timedata_tests.cpp \
+  test/torcontrol_tests.cpp \
   test/transaction_tests.cpp \
   test/txvalidationcache_tests.cpp \
   test/versionbits_tests.cpp \
diff --git a/src/test/torcontrol_tests.cpp b/src/test/torcontrol_tests.cpp
new file mode 100644
index 0000000000..b7affaacde
--- /dev/null
+++ b/src/test/torcontrol_tests.cpp
@@ -0,0 +1,199 @@
+// Copyright (c) 2017 The Zcash developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+//
+#include "test/test_bitcoin.h"
+#include "torcontrol.cpp"
+
+#include <boost/test/unit_test.hpp>
+
+
+BOOST_FIXTURE_TEST_SUITE(torcontrol_tests, BasicTestingSetup)
+
+void CheckSplitTorReplyLine(std::string input, std::string command, std::string args)
+{
+    BOOST_TEST_MESSAGE(std::string("CheckSplitTorReplyLine(") + input + ")");
+    auto ret = SplitTorReplyLine(input);
+    BOOST_CHECK_EQUAL(ret.first, command);
+    BOOST_CHECK_EQUAL(ret.second, args);
+}
+
+BOOST_AUTO_TEST_CASE(util_SplitTorReplyLine)
+{
+    // Data we should receive during normal usage
+    CheckSplitTorReplyLine(
+        "PROTOCOLINFO PIVERSION",
+        "PROTOCOLINFO", "PIVERSION");
+    CheckSplitTorReplyLine(
+        "AUTH METHODS=COOKIE,SAFECOOKIE COOKIEFILE=\"/home/x/.tor/control_auth_cookie\"",
+        "AUTH", "METHODS=COOKIE,SAFECOOKIE COOKIEFILE=\"/home/x/.tor/control_auth_cookie\"");
+    CheckSplitTorReplyLine(
+        "AUTH METHODS=NULL",
+        "AUTH", "METHODS=NULL");
+    CheckSplitTorReplyLine(
+        "AUTH METHODS=HASHEDPASSWORD",
+        "AUTH", "METHODS=HASHEDPASSWORD");
+    CheckSplitTorReplyLine(
+        "VERSION Tor=\"0.2.9.8 (git-a0df013ea241b026)\"",
+        "VERSION", "Tor=\"0.2.9.8 (git-a0df013ea241b026)\"");
+    CheckSplitTorReplyLine(
+        "AUTHCHALLENGE SERVERHASH=aaaa SERVERNONCE=bbbb",
+        "AUTHCHALLENGE", "SERVERHASH=aaaa SERVERNONCE=bbbb");
+
+    // Other valid inputs
+    CheckSplitTorReplyLine("COMMAND", "COMMAND", "");
+    CheckSplitTorReplyLine("COMMAND SOME  ARGS", "COMMAND", "SOME  ARGS");
+
+    // These inputs are valid because PROTOCOLINFO accepts an OtherLine that is
+    // just an OptArguments, which enables multiple spaces to be present
+    // between the command and arguments.
+    CheckSplitTorReplyLine("COMMAND  ARGS", "COMMAND", " ARGS");
+    CheckSplitTorReplyLine("COMMAND   EVEN+more  ARGS", "COMMAND", "  EVEN+more  ARGS");
+}
+
+void CheckParseTorReplyMapping(std::string input, std::map<std::string,std::string> expected)
+{
+    BOOST_TEST_MESSAGE(std::string("CheckParseTorReplyMapping(") + input + ")");
+    auto ret = ParseTorReplyMapping(input);
+    BOOST_CHECK_EQUAL(ret.size(), expected.size());
+    auto r_it = ret.begin();
+    auto e_it = expected.begin();
+    while (r_it != ret.end() && e_it != expected.end()) {
+        BOOST_CHECK_EQUAL(r_it->first, e_it->first);
+        BOOST_CHECK_EQUAL(r_it->second, e_it->second);
+        r_it++;
+        e_it++;
+    }
+}
+
+BOOST_AUTO_TEST_CASE(util_ParseTorReplyMapping)
+{
+    // Data we should receive during normal usage
+    CheckParseTorReplyMapping(
+        "METHODS=COOKIE,SAFECOOKIE COOKIEFILE=\"/home/x/.tor/control_auth_cookie\"", {
+            {"METHODS", "COOKIE,SAFECOOKIE"},
+            {"COOKIEFILE", "/home/x/.tor/control_auth_cookie"},
+        });
+    CheckParseTorReplyMapping(
+        "METHODS=NULL", {
+            {"METHODS", "NULL"},
+        });
+    CheckParseTorReplyMapping(
+        "METHODS=HASHEDPASSWORD", {
+            {"METHODS", "HASHEDPASSWORD"},
+        });
+    CheckParseTorReplyMapping(
+        "Tor=\"0.2.9.8 (git-a0df013ea241b026)\"", {
+            {"Tor", "0.2.9.8 (git-a0df013ea241b026)"},
+        });
+    CheckParseTorReplyMapping(
+        "SERVERHASH=aaaa SERVERNONCE=bbbb", {
+            {"SERVERHASH", "aaaa"},
+            {"SERVERNONCE", "bbbb"},
+        });
+    CheckParseTorReplyMapping(
+        "ServiceID=exampleonion1234", {
+            {"ServiceID", "exampleonion1234"},
+        });
+    CheckParseTorReplyMapping(
+        "PrivateKey=RSA1024:BLOB", {
+            {"PrivateKey", "RSA1024:BLOB"},
+        });
+    CheckParseTorReplyMapping(
+        "ClientAuth=bob:BLOB", {
+            {"ClientAuth", "bob:BLOB"},
+        });
+
+    // Other valid inputs
+    CheckParseTorReplyMapping(
+        "Foo=Bar=Baz Spam=Eggs", {
+            {"Foo", "Bar=Baz"},
+            {"Spam", "Eggs"},
+        });
+    CheckParseTorReplyMapping(
+        "Foo=\"Bar=Baz\"", {
+            {"Foo", "Bar=Baz"},
+        });
+    CheckParseTorReplyMapping(
+        "Foo=\"Bar Baz\"", {
+            {"Foo", "Bar Baz"},
+        });
+
+    // Escapes
+    CheckParseTorReplyMapping(
+        "Foo=\"Bar\\ Baz\"", {
+            {"Foo", "Bar Baz"},
+        });
+    CheckParseTorReplyMapping(
+        "Foo=\"Bar\\Baz\"", {
+            {"Foo", "BarBaz"},
+        });
+    CheckParseTorReplyMapping(
+        "Foo=\"Bar\\@Baz\"", {
+            {"Foo", "Bar@Baz"},
+        });
+    CheckParseTorReplyMapping(
+        "Foo=\"Bar\\\"Baz\" Spam=\"\\\"Eggs\\\"\"", {
+            {"Foo", "Bar\"Baz"},
+            {"Spam", "\"Eggs\""},
+        });
+    CheckParseTorReplyMapping(
+        "Foo=\"Bar\\\\Baz\"", {
+            {"Foo", "Bar\\Baz"},
+        });
+
+    // C escapes
+    CheckParseTorReplyMapping(
+        "Foo=\"Bar\\nBaz\\t\" Spam=\"\\rEggs\" Octals=\"\\1a\\11\\17\\18\\81\\377\\378\\400\\2222\" Final=Check", {
+            {"Foo", "Bar\nBaz\t"},
+            {"Spam", "\rEggs"},
+            {"Octals", "\1a\11\17\1" "881\377\37" "8\40" "0\222" "2"},
+            {"Final", "Check"},
+        });
+    CheckParseTorReplyMapping(
+        "Valid=Mapping Escaped=\"Escape\\\\\"", {
+            {"Valid", "Mapping"},
+            {"Escaped", "Escape\\"},
+        });
+    CheckParseTorReplyMapping(
+        "Valid=Mapping Bare=\"Escape\\\"", {});
+    CheckParseTorReplyMapping(
+        "OneOctal=\"OneEnd\\1\" TwoOctal=\"TwoEnd\\11\"", {
+            {"OneOctal", "OneEnd\1"},
+            {"TwoOctal", "TwoEnd\11"},
+        });
+
+    // Special handling for null case
+    // (needed because string comparison reads the null as end-of-string)
+    BOOST_TEST_MESSAGE(std::string("CheckParseTorReplyMapping(Null=\"\\0\")"));
+    auto ret = ParseTorReplyMapping("Null=\"\\0\"");
+    BOOST_CHECK_EQUAL(ret.size(), 1);
+    auto r_it = ret.begin();
+    BOOST_CHECK_EQUAL(r_it->first, "Null");
+    BOOST_CHECK_EQUAL(r_it->second.size(), 1);
+    BOOST_CHECK_EQUAL(r_it->second[0], '\0');
+
+    // A more complex valid grammar. PROTOCOLINFO accepts a VersionLine that
+    // takes a key=value pair followed by an OptArguments, making this valid.
+    // Because an OptArguments contains no semantic data, there is no point in
+    // parsing it.
+    CheckParseTorReplyMapping(
+        "SOME=args,here MORE optional=arguments  here", {
+            {"SOME", "args,here"},
+        });
+
+    // Inputs that are effectively invalid under the target grammar.
+    // PROTOCOLINFO accepts an OtherLine that is just an OptArguments, which
+    // would make these inputs valid. However,
+    // - This parser is never used in that situation, because the
+    //   SplitTorReplyLine parser enables OtherLine to be skipped.
+    // - Even if these were valid, an OptArguments contains no semantic data,
+    //   so there is no point in parsing it.
+    CheckParseTorReplyMapping("ARGS", {});
+    CheckParseTorReplyMapping("MORE ARGS", {});
+    CheckParseTorReplyMapping("MORE  ARGS", {});
+    CheckParseTorReplyMapping("EVEN more=ARGS", {});
+    CheckParseTorReplyMapping("EVEN+more ARGS", {});
+}
+
+BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
index 9e615142c6..8a37139f1d 100644
--- a/src/torcontrol.cpp
+++ b/src/torcontrol.cpp
@@ -1,4 +1,5 @@
 // Copyright (c) 2015-2016 The Bitcoin Core developers
+// Copyright (c) 2017 The Zcash developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -249,6 +250,8 @@ bool TorControlConnection::Command(const std::string &cmd, const ReplyHandlerCB&
 
 /* Split reply line in the form 'AUTH METHODS=...' into a type
  * 'AUTH' and arguments 'METHODS=...'.
+ * Grammar is implicitly defined in https://spec.torproject.org/control-spec by
+ * the server reply formats for PROTOCOLINFO (S3.21) and AUTHCHALLENGE (S3.24).
  */
 static std::pair<std::string,std::string> SplitTorReplyLine(const std::string &s)
 {
@@ -264,6 +267,10 @@ static std::pair<std::string,std::string> SplitTorReplyLine(const std::string &s
 }
 
 /** Parse reply arguments in the form 'METHODS=COOKIE,SAFECOOKIE COOKIEFILE=".../control_auth_cookie"'.
+ * Returns a map of keys to values, or an empty map if there was an error.
+ * Grammar is implicitly defined in https://spec.torproject.org/control-spec by
+ * the server reply formats for PROTOCOLINFO (S3.21), AUTHCHALLENGE (S3.24),
+ * and ADD_ONION (S3.27). See also sections 2.1 and 2.3.
  */
 static std::map<std::string,std::string> ParseTorReplyMapping(const std::string &s)
 {
@@ -271,28 +278,74 @@ static std::map<std::string,std::string> ParseTorReplyMapping(const std::string
     size_t ptr=0;
     while (ptr < s.size()) {
         std::string key, value;
-        while (ptr < s.size() && s[ptr] != '=') {
+        while (ptr < s.size() && s[ptr] != '=' && s[ptr] != ' ') {
             key.push_back(s[ptr]);
             ++ptr;
         }
         if (ptr == s.size()) // unexpected end of line
             return std::map<std::string,std::string>();
+        if (s[ptr] == ' ') // The remaining string is an OptArguments
+            break;
         ++ptr; // skip '='
         if (ptr < s.size() && s[ptr] == '"') { // Quoted string
-            ++ptr; // skip '='
+            ++ptr; // skip opening '"'
             bool escape_next = false;
-            while (ptr < s.size() && (!escape_next && s[ptr] != '"')) {
-                escape_next = (s[ptr] == '\\');
+            while (ptr < s.size() && (escape_next || s[ptr] != '"')) {
+                // Repeated backslashes must be interpreted as pairs
+                escape_next = (s[ptr] == '\\' && !escape_next);
                 value.push_back(s[ptr]);
                 ++ptr;
             }
             if (ptr == s.size()) // unexpected end of line
                 return std::map<std::string,std::string>();
             ++ptr; // skip closing '"'
-            /* TODO: unescape value - according to the spec this depends on the
-             * context, some strings use C-LogPrintf style escape codes, some
-             * don't. So may be better handled at the call site.
+            /**
+             * Unescape value. Per https://spec.torproject.org/control-spec section 2.1.1:
+             *
+             *   For future-proofing, controller implementors MAY use the following
+             *   rules to be compatible with buggy Tor implementations and with
+             *   future ones that implement the spec as intended:
+             *
+             *     Read \n \t \r and \0 ... \377 as C escapes.
+             *     Treat a backslash followed by any other character as that character.
              */
+            std::string escaped_value;
+            for (size_t i = 0; i < value.size(); ++i) {
+                if (value[i] == '\\') {
+                    // This will always be valid, because if the QuotedString
+                    // ended in an odd number of backslashes, then the parser
+                    // would already have returned above, due to a missing
+                    // terminating double-quote.
+                    ++i;
+                    if (value[i] == 'n') {
+                        escaped_value.push_back('\n');
+                    } else if (value[i] == 't') {
+                        escaped_value.push_back('\t');
+                    } else if (value[i] == 'r') {
+                        escaped_value.push_back('\r');
+                    } else if ('0' <= value[i] && value[i] <= '7') {
+                        size_t j;
+                        // Octal escape sequences have a limit of three octal digits,
+                        // but terminate at the first character that is not a valid
+                        // octal digit if encountered sooner.
+                        for (j = 1; j < 3 && (i+j) < value.size() && '0' <= value[i+j] && value[i+j] <= '7'; ++j) {}
+                        // Tor restricts first digit to 0-3 for three-digit octals.
+                        // A leading digit of 4-7 would therefore be interpreted as
+                        // a two-digit octal.
+                        if (j == 3 && value[i] > '3') {
+                            j--;
+                        }
+                        escaped_value.push_back(strtol(value.substr(i, j).c_str(), NULL, 8));
+                        // Account for automatic incrementing at loop end
+                        i += j - 1;
+                    } else {
+                        escaped_value.push_back(value[i]);
+                    }
+                } else {
+                    escaped_value.push_back(value[i]);
+                }
+            }
+            value = escaped_value;
         } else { // Unquoted value. Note that values can contain '=' at will, just no spaces
             while (ptr < s.size() && s[ptr] != ' ') {
                 value.push_back(s[ptr]);
@@ -322,6 +375,10 @@ static std::pair<bool,std::string> ReadBinaryFile(const fs::path &filename, size
     char buffer[128];
     size_t n;
     while ((n=fread(buffer, 1, sizeof(buffer), f)) > 0) {
+        // Check for reading errors so we don't return any data if we couldn't
+        // read the entire file (or up to maxsize)
+        if (ferror(f))
+            return std::make_pair(false,"");
         retval.append(buffer, buffer+n);
         if (retval.size() > maxsize)
             break;
@@ -438,6 +495,13 @@ void TorController::add_onion_cb(TorControlConnection& _conn, const TorControlRe
             if ((i = m.find("PrivateKey")) != m.end())
                 private_key = i->second;
         }
+        if (service_id.empty()) {
+            LogPrintf("tor: Error parsing ADD_ONION parameters:\n");
+            for (const std::string &s : reply.lines) {
+                LogPrintf("    %s\n", SanitizeString(s));
+            }
+            return;
+        }
         service = LookupNumeric(std::string(service_id+".onion").c_str(), GetListenPort());
         LogPrintf("tor: Got service ID %s, advertising service %s\n", service_id, service.ToString());
         if (WriteBinaryFile(GetPrivateKeyFile(), private_key)) {
@@ -515,6 +579,10 @@ void TorController::authchallenge_cb(TorControlConnection& _conn, const TorContr
         std::pair<std::string,std::string> l = SplitTorReplyLine(reply.lines[0]);
         if (l.first == "AUTHCHALLENGE") {
             std::map<std::string,std::string> m = ParseTorReplyMapping(l.second);
+            if (m.empty()) {
+                LogPrintf("tor: Error parsing AUTHCHALLENGE parameters: %s\n", SanitizeString(l.second));
+                return;
+            }
             std::vector<uint8_t> serverHash = ParseHex(m["SERVERHASH"]);
             std::vector<uint8_t> serverNonce = ParseHex(m["SERVERNONCE"]);
             LogPrint(BCLog::TOR, "tor: AUTHCHALLENGE ServerHash %s ServerNonce %s\n", HexStr(serverHash), HexStr(serverNonce));