Skip to content

Commit

Permalink
LogDnsClient now rejects responses unless they contain exactly one TX…
Browse files Browse the repository at this point in the history 
…T RDATA string

Previously, multiple TXT RDATA strings would be concatenated if present,
allowing a larger number of audit proof nodes to be delivered. However,
the CT-over-DNS draft RFC
(https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft-ct-over-dns.md)
explicitly states that the response should contain only one string, and trying
to handle other cases only causes confusion.

BUG=624894

Review-Url: https://codereview.chromium.org/2375693002
Cr-Commit-Position: refs/heads/master@{#423140}
  • Loading branch information
robpercival authored and Commit bot committed Oct 5, 2016
1 parent 08ee122 commit 9dc75a8
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 36 deletions.
18 changes: 17 additions & 1 deletion components/certificate_transparency/log_dns_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ namespace certificate_transparency {

namespace {

// Parses the DNS response and extracts a single string from the TXT RDATA.
// If the response is malformed, not a TXT record, or contains any number of
// strings other than 1, this returns false and extracts nothing.
// Otherwise, it returns true and the extracted string is assigned to |*txt|.
bool ParseTxtResponse(const net::DnsResponse& response, std::string* txt) {
DCHECK(txt);

Expand All @@ -44,10 +48,17 @@ bool ParseTxtResponse(const net::DnsResponse& response, std::string* txt) {
if (txt_record == nullptr)
return false;

*txt = base::JoinString(txt_record->texts(), "");
// The draft CT-over-DNS RFC says that there MUST be exactly one string in the
// TXT record.
if (txt_record->texts().size() != 1)
return false;

*txt = txt_record->texts().front();
return true;
}

// Extracts a leaf index value from a DNS response's TXT RDATA.
// Returns true on success, false otherwise.
bool ParseLeafIndex(const net::DnsResponse& response, uint64_t* index) {
DCHECK(index);

Expand All @@ -58,6 +69,11 @@ bool ParseLeafIndex(const net::DnsResponse& response, uint64_t* index) {
return base::StringToUint64(index_str, index);
}

// Extracts audit proof nodes from a DNS response's TXT RDATA.
// Returns true on success, false otherwise.
// It will fail if there is not a whole number of nodes present > 0.
// There must only be one string in the TXT RDATA.
// The nodes will be appended to |proof->nodes|
bool ParseAuditPath(const net::DnsResponse& response,
net::ct::MerkleAuditProof* proof) {
DCHECK(proof);
Expand Down
84 changes: 73 additions & 11 deletions components/certificate_transparency/log_dns_client_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "components/certificate_transparency/log_dns_client.h"

#include <memory>
#include <numeric>
#include <string>
#include <utility>
#include <vector>
Expand Down Expand Up @@ -152,7 +153,7 @@ class LogDnsClientTest : public ::testing::TestWithParam<net::IoMode> {
TEST_P(LogDnsClientTest, QueryLeafIndex) {
mock_dns_.ExpectLeafIndexRequestAndResponse(
"D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
"123456");
123456);

MockLeafIndexCallback callback;
QueryLeafIndex("ct.test", kLeafHash, &callback);
Expand Down Expand Up @@ -197,11 +198,37 @@ TEST_P(LogDnsClientTest, QueryLeafIndexReportsServerRefusal) {
EXPECT_THAT(callback.leaf_index(), 0);
}

TEST_P(LogDnsClientTest,
QueryLeafIndexReportsMalformedResponseIfContainsNoStrings) {
mock_dns_.ExpectRequestAndResponse(
"D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
std::vector<base::StringPiece>());

MockLeafIndexCallback callback;
QueryLeafIndex("ct.test", kLeafHash, &callback);
ASSERT_TRUE(callback.called());
EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_MALFORMED_RESPONSE));
EXPECT_THAT(callback.leaf_index(), 0);
}

TEST_P(LogDnsClientTest,
QueryLeafIndexReportsMalformedResponseIfContainsMoreThanOneString) {
mock_dns_.ExpectRequestAndResponse(
"D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
{"123456", "7"});

MockLeafIndexCallback callback;
QueryLeafIndex("ct.test", kLeafHash, &callback);
ASSERT_TRUE(callback.called());
EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_MALFORMED_RESPONSE));
EXPECT_THAT(callback.leaf_index(), 0);
}

TEST_P(LogDnsClientTest,
QueryLeafIndexReportsMalformedResponseIfLeafIndexIsNotNumeric) {
mock_dns_.ExpectLeafIndexRequestAndResponse(
mock_dns_.ExpectRequestAndResponse(
"D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
"foo");
{"foo"});

MockLeafIndexCallback callback;
QueryLeafIndex("ct.test", kLeafHash, &callback);
Expand All @@ -212,9 +239,9 @@ TEST_P(LogDnsClientTest,

TEST_P(LogDnsClientTest,
QueryLeafIndexReportsMalformedResponseIfLeafIndexIsFloatingPoint) {
mock_dns_.ExpectLeafIndexRequestAndResponse(
mock_dns_.ExpectRequestAndResponse(
"D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
"123456.0");
{"123456.0"});

MockLeafIndexCallback callback;
QueryLeafIndex("ct.test", kLeafHash, &callback);
Expand All @@ -225,8 +252,9 @@ TEST_P(LogDnsClientTest,

TEST_P(LogDnsClientTest,
QueryLeafIndexReportsMalformedResponseIfLeafIndexIsEmpty) {
mock_dns_.ExpectLeafIndexRequestAndResponse(
"D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.", "");
mock_dns_.ExpectRequestAndResponse(
"D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
{""});

MockLeafIndexCallback callback;
QueryLeafIndex("ct.test", kLeafHash, &callback);
Expand All @@ -237,9 +265,9 @@ TEST_P(LogDnsClientTest,

TEST_P(LogDnsClientTest,
QueryLeafIndexReportsMalformedResponseIfLeafIndexHasNonNumericPrefix) {
mock_dns_.ExpectLeafIndexRequestAndResponse(
mock_dns_.ExpectRequestAndResponse(
"D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
"foo123456");
{"foo123456"});

MockLeafIndexCallback callback;
QueryLeafIndex("ct.test", kLeafHash, &callback);
Expand All @@ -250,9 +278,9 @@ TEST_P(LogDnsClientTest,

TEST_P(LogDnsClientTest,
QueryLeafIndexReportsMalformedResponseIfLeafIndexHasNonNumericSuffix) {
mock_dns_.ExpectLeafIndexRequestAndResponse(
mock_dns_.ExpectRequestAndResponse(
"D4S6DSV2J743QJZEQMH4UYHEYK7KRQ5JIQOCPMFUHZVJNFGHXACA.hash.ct.test.",
"123456foo");
{"123456foo"});

MockLeafIndexCallback callback;
QueryLeafIndex("ct.test", kLeafHash, &callback);
Expand Down Expand Up @@ -415,6 +443,40 @@ TEST_P(LogDnsClientTest, QueryAuditProofReportsServerRefusal) {
EXPECT_THAT(callback.proof(), IsNull());
}

TEST_P(LogDnsClientTest,
QueryAuditProofReportsResponseMalformedIfContainsNoStrings) {
mock_dns_.ExpectRequestAndResponse("0.123456.999999.tree.ct.test.",
std::vector<base::StringPiece>());

MockAuditProofCallback callback;
QueryAuditProof("ct.test", 123456, 999999, &callback);
ASSERT_TRUE(callback.called());
EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_MALFORMED_RESPONSE));
EXPECT_THAT(callback.proof(), IsNull());
}

TEST_P(LogDnsClientTest,
QueryAuditProofReportsResponseMalformedIfContainsMoreThanOneString) {
// The CT-over-DNS draft RFC states that the response will contain "exactly
// one character-string."
const std::vector<std::string> audit_proof = GetSampleAuditProof(10);

std::string first_chunk_of_proof = std::accumulate(
audit_proof.begin(), audit_proof.begin() + 7, std::string());
std::string second_chunk_of_proof = std::accumulate(
audit_proof.begin() + 7, audit_proof.end(), std::string());

mock_dns_.ExpectRequestAndResponse(
"0.123456.999999.tree.ct.test.",
{first_chunk_of_proof, second_chunk_of_proof});

MockAuditProofCallback callback;
QueryAuditProof("ct.test", 123456, 999999, &callback);
ASSERT_TRUE(callback.called());
EXPECT_THAT(callback.net_error(), IsError(net::ERR_DNS_MALFORMED_RESPONSE));
EXPECT_THAT(callback.proof(), IsNull());
}

TEST_P(LogDnsClientTest,
QueryAuditProofReportsResponseMalformedIfNodeTooShort) {
// node is shorter than a SHA-256 hash (31 vs 32 bytes)
Expand Down
37 changes: 19 additions & 18 deletions components/certificate_transparency/mock_log_dns_traffic.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include <vector>

#include "base/big_endian.h"
#include "base/numerics/safe_conversions.h"
#include "base/strings/string_number_conversions.h"
#include "base/sys_byteorder.h"
#include "base/test/test_timeouts.h"
#include "net/dns/dns_client.h"
Expand Down Expand Up @@ -193,15 +195,24 @@ void MockLogDnsTraffic::ExpectRequestAndTimeout(base::StringPiece qname) {
SetDnsTimeout(TestTimeouts::tiny_timeout());
}

void MockLogDnsTraffic::ExpectLeafIndexRequestAndResponse(
void MockLogDnsTraffic::ExpectRequestAndResponse(
base::StringPiece qname,
base::StringPiece leaf_index) {
// Prepend size to leaf_index to create the query answer (rdata)
ASSERT_LE(leaf_index.size(), 0xFFul); // size must fit into a single byte
std::string answer = leaf_index.as_string();
answer.insert(answer.begin(), static_cast<char>(leaf_index.size()));
const std::vector<base::StringPiece>& txt_strings) {
std::string answer;
for (base::StringPiece str : txt_strings) {
// The size of the string must precede it. The size must fit into 1 byte.
answer.insert(answer.end(), base::checked_cast<uint8_t>(str.size()));
str.AppendToString(&answer);
}

ExpectRequestAndResponse(qname, answer);
std::vector<char> request = CreateDnsTxtRequest(qname);
EmplaceMockSocketData(request, CreateDnsTxtResponse(request, answer));
}

void MockLogDnsTraffic::ExpectLeafIndexRequestAndResponse(
base::StringPiece qname,
uint64_t leaf_index) {
ExpectRequestAndResponse(qname, { base::Uint64ToString(leaf_index) });
}

void MockLogDnsTraffic::ExpectAuditProofRequestAndResponse(
Expand All @@ -212,11 +223,7 @@ void MockLogDnsTraffic::ExpectAuditProofRequestAndResponse(
std::string proof =
std::accumulate(audit_path_start, audit_path_end, std::string());

// Prepend size to proof to create the query answer (rdata)
ASSERT_LE(proof.size(), 0xFFul); // size must fit into a single byte
proof.insert(proof.begin(), static_cast<char>(proof.size()));

ExpectRequestAndResponse(qname, proof);
ExpectRequestAndResponse(qname, { proof });
}

void MockLogDnsTraffic::InitializeDnsConfig() {
Expand Down Expand Up @@ -246,12 +253,6 @@ std::unique_ptr<net::DnsClient> MockLogDnsTraffic::CreateDnsClient() {
base::Bind(&FakeRandInt));
}

void MockLogDnsTraffic::ExpectRequestAndResponse(base::StringPiece qname,
base::StringPiece answer) {
std::vector<char> request = CreateDnsTxtRequest(qname);
EmplaceMockSocketData(request, CreateDnsTxtResponse(request, answer));
}

template <typename... Args>
void MockLogDnsTraffic::EmplaceMockSocketData(Args&&... args) {
mock_socket_data_.emplace_back(
Expand Down
12 changes: 6 additions & 6 deletions components/certificate_transparency/mock_log_dns_traffic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,16 @@ class MockLogDnsTraffic {
// This will reduce the DNS timeout to minimize test duration.
void ExpectRequestAndTimeout(base::StringPiece qname);
// Expect a CT DNS request for the domain |qname|.
// Such a request will receive a DNS TXT response containing |txt_strings|.
void ExpectRequestAndResponse(
base::StringPiece qname,
const std::vector<base::StringPiece>& txt_strings);
// Expect a CT DNS request for the domain |qname|.
// Such a request will receive a DNS response containing |leaf_index|.
// A description of such a request and response can be seen here:
// https://github.com/google/certificate-transparency-rfcs/blob/c8844de6bd0b5d3d16bac79865e6edef533d760b/dns/draft-ct-over-dns.md#hash-query-hashquery
void ExpectLeafIndexRequestAndResponse(base::StringPiece qname,
base::StringPiece leaf_index);
uint64_t leaf_index);
// Expect a CT DNS request for the domain |qname|.
// Such a request will receive a DNS response containing the inclusion proof
// nodes between |audit_path_start| and |audit_path_end|.
Expand Down Expand Up @@ -131,11 +136,6 @@ class MockLogDnsTraffic {
}

private:
// Expect A CT DNS request for the domain |qname|.
// Such a request will receive a DNS response containing |answer|.
void ExpectRequestAndResponse(base::StringPiece qname,
base::StringPiece answer);

// Constructs MockSocketData from |args| and adds it to |socket_factory_|.
template <typename... Args>
void EmplaceMockSocketData(Args&&... args);
Expand Down

0 comments on commit 9dc75a8

Please sign in to comment.