Skip to content

Commit

Permalink
Merge #10408, #13291, and partial #13163
Browse files Browse the repository at this point in the history
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
97c112d 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
bitcoin/bitcoin#13291

(partial) Merge #13163: Make it clear which functions that are intended to be translation unit local
c3f34d0 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
bitcoin/bitcoin#13163

Completes T612

Introduces a memory leak fixed by PR10587
bitcoin/bitcoin#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:<tor port>
  ./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
  • Loading branch information
laanwj authored and Nico Guiton committed Jun 27, 2019
1 parent 5f48bb4 commit be756d0
Show file tree
Hide file tree
Showing 5 changed files with 294 additions and 12 deletions.
5 changes: 4 additions & 1 deletion arcanist/linter/LocaleDependenceLinter.php
Expand Up @@ -29,7 +29,10 @@ final class LocaleDependenceLinter extends ArcanistLinter {
"split",
"is_space",
],
"src/torcontrol.cpp" => ["atoi"],
"src/torcontrol.cpp" => [
"atoi",
"strtol",
],
"src/uint256.cpp" => ["tolower"],
"src/util.cpp" => ["atoi", "tolower"],
"src/utilmoneystr.cpp" => ["isdigit"],
Expand Down
1 change: 1 addition & 0 deletions src/Makefile.test.include
Expand Up @@ -116,6 +116,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 \
Expand Down
1 change: 1 addition & 0 deletions src/test/CMakeLists.txt
Expand Up @@ -129,6 +129,7 @@ add_test_to_suite(bitcoin test_bitcoin
test_bitcoin.cpp
test_bitcoin_main.cpp
timedata_tests.cpp
torcontrol_tests.cpp
transaction_tests.cpp
txindex_tests.cpp
txvalidationcache_tests.cpp
Expand Down
201 changes: 201 additions & 0 deletions 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 <torcontrol.h>

#include <test/test_bitcoin.h>

#include <boost/test/unit_test.hpp>

#include <map>
#include <string>
#include <utility>

std::pair<std::string, std::string> SplitTorReplyLine(const std::string &s);
std::map<std::string, std::string> 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<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()
98 changes: 87 additions & 11 deletions 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.

Expand Down Expand Up @@ -270,9 +271,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<std::string, std::string>
SplitTorReplyLine(const std::string &s) {
std::pair<std::string, std::string> SplitTorReplyLine(const std::string &s) {
size_t ptr = 0;
std::string type;
while (ptr < s.size() && s[ptr] != ' ') {
Expand All @@ -289,30 +291,38 @@ 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) {
std::map<std::string, std::string> ParseTorReplyMapping(const std::string &s) {
std::map<std::string, std::string> 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;
}
// unexpected end of line
if (ptr == s.size()) {
return std::map<std::string, std::string>();
}
// 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;
}
Expand All @@ -322,10 +332,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
Expand Down Expand Up @@ -364,6 +423,11 @@ ReadBinaryFile(const fs::path &filename,
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;
Expand Down Expand Up @@ -490,6 +554,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",
Expand Down Expand Up @@ -582,6 +653,11 @@ void TorController::authchallenge_cb(TorControlConnection &_conn,
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,
Expand Down

0 comments on commit be756d0

Please sign in to comment.