From 85c483cec964fba788a9f30c196666d2eabe558b Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 18 May 2017 19:58:34 +0200 Subject: [PATCH] Merge #10408, #13291, and partial #13163 Summary: 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 Backport of Core PR10408 https://github.com/bitcoin/bitcoin/pull/10408/files Merge #13291: test: Don't include torcontrol.cpp into the test file 97c112d4ca Declare TorReply parsing functions in torcontrol_tests (Ben Woosley) Pull request description: These methods are standalone string parsing methods which were included into test via an include of torcontrol.cpp, which is bad practice. ~~Splitting them out reveals that they were the only torcontrol.cpp methods under test, so the test file is renamed tor_reply_tests.cpp.~~ Introduced in #10408 Tree-SHA512: 8ff11a9c900a88f910a73dfe16f43581a567e9d60e9298a8a963fc9dd7cffb4d97a644da677610aafb7d89f1dd1cede9afeae2c6344305e021a9a322dbcea0ac Backport of Core PR13291 https://github.com/bitcoin/bitcoin/pull/13291/ (partial) Merge #13163: Make it clear which functions that are intended to be translation unit local c3f34d06be Make it clear which functions that are intended to be translation unit local (practicalswift) Pull request description: Make it clear which functions that are intended to be translation unit local. Do not share functions that are meant to be translation unit local with other translation units. Use internal linkage for those consistently. Tree-SHA512: 05eebd233d5cfbf6116724eec3a99b465bf534ca220f2b6f5e56341a7da41387454d3cb6ceadd8ab6714a5df94069e4ad0dcab8801ccc7e8949be7199a19fb53 Backport of Core PR13163 https://github.com/bitcoin/bitcoin/pull/13163/ Completes T612 Introduces a memory leak fixed by PR10587 https://github.com/bitcoin/bitcoin/pull/10587 Memory leak fix: https://reviews.bitcoinabc.org/D3285 Includes a fix to the locale linter discussed with fabien D3285 should be landed immediately after this one Test Plan: make check test_runner.py sudo apt install tor # if not already installed sudo etc/init.d/tor status # to check if tor is running sudo etc/init.d/tor start # if it is not already running ./src/bitcoind -proxy=127.0.0.1: ./bitcoin-cli getpeerinfo tor defaults to port 9050. `getpeerinfo` should be populated. arc lint should pass with no errors. Reviewers: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3283 --- src/Makefile.test.include | 1 + src/catch_tests/CMakeLists.txt | 1 + src/catch_tests/catch_unit.h | 10 ++ src/catch_tests/torcontrol_tests.cpp | 185 ++++++++++++++++++++++++ src/test/CMakeLists.txt | 1 + src/test/torcontrol_tests.cpp | 201 +++++++++++++++++++++++++++ src/torcontrol.cpp | 93 +++++++++++-- 7 files changed, 481 insertions(+), 11 deletions(-) create mode 100644 src/catch_tests/torcontrol_tests.cpp create mode 100644 src/test/torcontrol_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 463e06f48..f44b974dd 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -111,6 +111,7 @@ BITCOIN_TESTS =\ test/test_bitcoin.h \ test/test_bitcoin_main.cpp \ test/timedata_tests.cpp \ + test/torcontrol_tests.cpp \ test/transaction_tests.cpp \ test/txindex_tests.cpp \ test/txvalidationcache_tests.cpp \ diff --git a/src/catch_tests/CMakeLists.txt b/src/catch_tests/CMakeLists.txt index f808624e1..30e51bd64 100644 --- a/src/catch_tests/CMakeLists.txt +++ b/src/catch_tests/CMakeLists.txt @@ -137,6 +137,7 @@ set(UNIT_CTESTS streams sync timedata + torcontrol transaction txindex txvalidationcache diff --git a/src/catch_tests/catch_unit.h b/src/catch_tests/catch_unit.h index 7fda2166f..7fcf9a7ad 100644 --- a/src/catch_tests/catch_unit.h +++ b/src/catch_tests/catch_unit.h @@ -13,5 +13,15 @@ #define BOOST_ERROR(A) std::cout << (A) << "\n"; +#define BOOST_FIXTURE_TEST_SUITE(a,b) +#define BOOST_AUTO_TEST_SUITE_END() + +#define STR_VALUE(arg) #arg +// this extra level may not be needed +#define XXX_CASE TEST_CASE +#define BOOST_AUTO_TEST_CASE(name) XXX_CASE(STR_VALUE(name)) + +#define BOOST_TEST_MESSAGE(a) std::cout << a + #define CATCH_CONFIG_MAIN #include diff --git a/src/catch_tests/torcontrol_tests.cpp b/src/catch_tests/torcontrol_tests.cpp new file mode 100644 index 000000000..c1a232e72 --- /dev/null +++ b/src/catch_tests/torcontrol_tests.cpp @@ -0,0 +1,185 @@ +// 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 + +#include + +#include "catch_unit.h" + +#include +#include +#include + +std::pair SplitTorReplyLine(const std::string &s); +std::map ParseTorReplyMapping(const std::string &s); + +BOOST_FIXTURE_TEST_SUITE(torcontrol_tests, BasicTestingSetup) + +static 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) { + BasicTestingSetup setup; + // 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"); +} + +static void CheckParseTorReplyMapping(std::string input, std::map 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) { + BasicTestingSetup setup; + // 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/test/CMakeLists.txt b/src/test/CMakeLists.txt index 2ce00680a..f16a16dd2 100644 --- a/src/test/CMakeLists.txt +++ b/src/test/CMakeLists.txt @@ -130,6 +130,7 @@ add_test_to_suite(devault test_devault test_bitcoin.cpp test_bitcoin_main.cpp timedata_tests.cpp + torcontrol_tests.cpp transaction_tests.cpp txindex_tests.cpp txvalidationcache_tests.cpp diff --git a/src/test/torcontrol_tests.cpp b/src/test/torcontrol_tests.cpp new file mode 100644 index 000000000..445adce1e --- /dev/null +++ b/src/test/torcontrol_tests.cpp @@ -0,0 +1,201 @@ +// 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 + +#include + +#include + +#include +#include +#include + +std::pair SplitTorReplyLine(const std::string &s); +std::map ParseTorReplyMapping(const std::string &s); + +BOOST_FIXTURE_TEST_SUITE(torcontrol_tests, BasicTestingSetup) + +static 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"); +} + +static void +CheckParseTorReplyMapping(std::string input, + std::map 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 09372b96a..f15597142 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. @@ -265,9 +266,10 @@ bool TorControlConnection::Command(const std::string &cmd, /* 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 -SplitTorReplyLine(const std::string &s) { +std::pair SplitTorReplyLine(const std::string &s) { size_t ptr = 0; std::string type; while (ptr < s.size() && s[ptr] != ' ') { @@ -284,14 +286,17 @@ 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 -ParseTorReplyMapping(const std::string &s) { +std::map ParseTorReplyMapping(const std::string &s) { std::map mapping; 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; } @@ -299,15 +304,20 @@ ParseTorReplyMapping(const std::string &s) { if (ptr == s.size()) { return std::map(); } + // The remaining string is an OptArguments + if (s[ptr] == ' ') { + break; + } // skip '=' ++ptr; // Quoted string if (ptr < s.size() && s[ptr] == '"') { - // skip '=' + // skip opening '"' ++ptr; 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; } @@ -317,10 +327,59 @@ ParseTorReplyMapping(const std::string &s) { } // skip closing '"' ++ptr; - /* 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 @@ -491,6 +550,13 @@ void TorController::add_onion_cb(TorControlConnection &_conn, 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", @@ -583,6 +649,11 @@ void TorController::authchallenge_cb(TorControlConnection &_conn, if (l.first == "AUTHCHALLENGE") { std::map m = ParseTorReplyMapping(l.second); + if (m.empty()) { + LogPrintf("tor: Error parsing AUTHCHALLENGE parameters: %s\n", + SanitizeString(l.second)); + return; + } std::vector serverHash = ParseHex(m["SERVERHASH"]); std::vector serverNonce = ParseHex(m["SERVERNONCE"]); LogPrint(BCLog::TOR,