From d8e03c0340a65601a92ca3a69cfd3049cbaf49f1 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 25 Mar 2017 20:13:18 +1300 Subject: [PATCH 1/6] torcontrol: Improve comments --- src/torcontrol.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index c1bd95b00..2e15c9e73 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -250,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 SplitTorReplyLine(const std::string &s) { @@ -265,6 +267,9 @@ static std::pair SplitTorReplyLine(const std::string &s } /** Parse reply arguments in the form 'METHODS=COOKIE,SAFECOOKIE COOKIEFILE=".../control_auth_cookie"'. + * 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) { @@ -280,7 +285,7 @@ static std::map ParseTorReplyMapping(const std::string return std::map(); ++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] == '\\'); From 29f3c200780bcb669f31932ce484aee2e5a51a02 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 25 Mar 2017 20:17:37 +1300 Subject: [PATCH 2/6] torcontrol: Add unit tests for Tor reply parsers --- src/Makefile.test.include | 1 + src/test/torcontrol_tests.cpp | 168 ++++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+) create mode 100644 src/test/torcontrol_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 10cb7e775..ee1c11ff1 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 000000000..68516599d --- /dev/null +++ b/src/test/torcontrol_tests.cpp @@ -0,0 +1,168 @@ +// 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_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 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 (which are left escaped by the parser) + CheckParseTorReplyMapping( + "Foo=\"Bar\\ Baz\"", { + {"Foo", "Bar\\ Baz"}, + }); + CheckParseTorReplyMapping( + "Foo=\"Bar\\Baz\"", { + {"Foo", "Bar\\Baz"}, + }); + 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"}, + }); + + // 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() From d63677bbb23a05feca7935c70673439705b06dca Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 26 Mar 2017 00:35:13 +1300 Subject: [PATCH 3/6] torcontrol: Fix ParseTorReplyMapping - Ignore remaining input if it is an OptArguments - Correctly handle escapes --- src/torcontrol.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index 2e15c9e73..0d787d16e 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -277,17 +277,19 @@ static std::map 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(); + if (s[ptr] == ' ') // The remaining string is an OptArguments + break; ++ptr; // skip '=' if (ptr < s.size() && s[ptr] == '"') { // Quoted string ++ptr; // skip opening '"' bool escape_next = false; - while (ptr < s.size() && (!escape_next && s[ptr] != '"')) { + while (ptr < s.size() && (escape_next || s[ptr] != '"')) { escape_next = (s[ptr] == '\\'); value.push_back(s[ptr]); ++ptr; From 0b6f40d4cabb3bebf551a49a69ce36d4b0375b6a Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 26 Mar 2017 13:53:13 +1300 Subject: [PATCH 4/6] torcontrol: Check for reading errors in ReadBinaryFile This ensures that ReadBinaryFile never returns exactly TOR_COOKIE_SIZE bytes if the file was larger than that. --- src/torcontrol.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index 0d787d16e..bc48665f7 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -330,6 +330,10 @@ static std::pair 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; From 0182a11737e02b2ce0ab162ecfade904feb5f640 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 26 Mar 2017 14:35:13 +1300 Subject: [PATCH 5/6] torcontrol: Log invalid parameters in Tor reply strings where meaningful --- src/torcontrol.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index bc48665f7..879a63bfe 100644 --- a/src/torcontrol.cpp +++ b/src/torcontrol.cpp @@ -267,6 +267,7 @@ static std::pair 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. @@ -450,6 +451,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)) { @@ -527,6 +535,10 @@ void TorController::authchallenge_cb(TorControlConnection& _conn, const TorContr std::pair l = SplitTorReplyLine(reply.lines[0]); 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, "tor: AUTHCHALLENGE ServerHash %s ServerNonce %s\n", HexStr(serverHash), HexStr(serverNonce)); From 49a199bb51fc00659f8134e5b16f5d36364b0554 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 11 Apr 2017 18:30:42 +1200 Subject: [PATCH 6/6] torcontrol: Handle escapes in Tor QuotedStrings https://trac.torproject.org/projects/tor/ticket/14999 is tracking an encoding bug with the Tor control protocol, where many of the QuotedString instances that Tor outputs are in fact CStrings, but it is not documented which ones are which. https://spec.torproject.org/control-spec section 2.1.1 provides a future-proofed rule for handing QuotedStrings, which this commit implements. This commit merges all six commits from https://github.com/zcash/zcash/pull/2251 --- src/test/torcontrol_tests.cpp | 45 ++++++++++++++++++++++++----- src/torcontrol.cpp | 53 ++++++++++++++++++++++++++++++++--- 2 files changed, 87 insertions(+), 11 deletions(-) diff --git a/src/test/torcontrol_tests.cpp b/src/test/torcontrol_tests.cpp index 68516599d..b7affaacd 100644 --- a/src/test/torcontrol_tests.cpp +++ b/src/test/torcontrol_tests.cpp @@ -119,29 +119,60 @@ BOOST_AUTO_TEST_CASE(util_ParseTorReplyMapping) {"Foo", "Bar Baz"}, }); - // Escapes (which are left escaped by the parser) + // Escapes CheckParseTorReplyMapping( "Foo=\"Bar\\ Baz\"", { - {"Foo", "Bar\\ Baz"}, + {"Foo", "Bar Baz"}, }); CheckParseTorReplyMapping( "Foo=\"Bar\\Baz\"", { - {"Foo", "Bar\\Baz"}, + {"Foo", "BarBaz"}, }); CheckParseTorReplyMapping( "Foo=\"Bar\\@Baz\"", { - {"Foo", "Bar\\@Baz"}, + {"Foo", "Bar@Baz"}, }); CheckParseTorReplyMapping( "Foo=\"Bar\\\"Baz\" Spam=\"\\\"Eggs\\\"\"", { - {"Foo", "Bar\\\"Baz"}, - {"Spam", "\\\"Eggs\\\""}, + {"Foo", "Bar\"Baz"}, + {"Spam", "\"Eggs\""}, }); CheckParseTorReplyMapping( "Foo=\"Bar\\\\Baz\"", { - {"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 diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp index 879a63bfe..46a926b3e 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. @@ -291,17 +292,61 @@ static std::map ParseTorReplyMapping(const std::string ++ptr; // skip opening '"' bool escape_next = false; while (ptr < s.size() && (escape_next || s[ptr] != '"')) { - 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(); ++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]);