Remove support for DNS-over-TLS certificate fingerprints. (0e73c2e5) · Commits · e / os / android_packages_modules_DnsResolver

Android.bp

+3 −2

Original line number	Diff line number	Diff line
		@@ -70,7 +70,7 @@ cc_library {
		// on system ABIs
		stl: "libc++_static",
		static_libs: [
		"dnsresolver_aidl_interface-V2-ndk_platform",
		"dnsresolver_aidl_interface-ndk_platform",
		"libbase",
		"libcrypto",
		"libcutils",
		@@ -100,6 +100,7 @@ cc_library {
		debuggable: {
		cppflags: [
		"-DRESOLV_ALLOW_VERBOSE_LOGGING=1",
		"-DRESOLV_INJECT_CA_CERTIFICATE=1",
		],
		},
		},
		@@ -171,6 +172,7 @@ cc_test {
		"libutils",
		],
		static_libs: [
		"dnsresolver_aidl_interface-cpp",
		"libgmock",
		"libnetd_test_dnsresponder",
		"libnetd_test_metrics_listener",
		@@ -180,7 +182,6 @@ cc_test {
		"libnetdutils",
		"netd_aidl_interface-V2-cpp",
		"netd_event_listener_interface-V1-cpp",
		"dnsresolver_aidl_interface-V2-cpp",
		],
		compile_multilib: "both",
		sanitize: {

DnsResolverService.cpp

+2 −43

Original line number	Diff line number	Diff line
		@@ -27,8 +27,6 @@
		#include <android/binder_manager.h>
		#include <android/binder_process.h>
		#include <netdutils/DumpWriter.h>
		#include <netdutils/NetworkConstants.h> // SHA256_SIZE
		#include <openssl/base64.h>
		#include <private/android_filesystem_config.h> // AID_SYSTEM

		#include "DnsResolver.h"
		@@ -164,33 +162,6 @@ binder_status_t DnsResolverService::dump(int fd, const char**, uint32_t) {
		return ::ndk::ScopedAStatus(AStatus_fromExceptionCodeWithMessage(EX_SECURITY, err.c_str()));
		}

		namespace {

		// Parse a base64 encoded string into a vector of bytes.
		// On failure, return an empty vector.
		static std::vector<uint8_t> parseBase64(const std::string& input) {
		std::vector<uint8_t> decoded;
		size_t out_len;
		if (EVP_DecodedLength(&out_len, input.size()) != 1) {
		return decoded;
		}
		// out_len is now an upper bound on the output length.
		decoded.resize(out_len);
		if (EVP_DecodeBase64(decoded.data(), &out_len, decoded.size(),
		reinterpret_cast<const uint8_t*>(input.data()), input.size()) == 1) {
		// Possibly shrink the vector if the actual output was smaller than the bound.
		decoded.resize(out_len);
		} else {
		decoded.clear();
		}
		if (out_len != android::netdutils::SHA256_SIZE) {
		decoded.clear();
		}
		return decoded;
		}

		} // namespace

		::ndk::ScopedAStatus DnsResolverService::setResolverConfiguration(
		const ResolverParamsParcel& resolverParams) {
		// Locking happens in PrivateDnsConfiguration and res_* functions.
		@@ -203,21 +174,9 @@ static std::vector<uint8_t> parseBase64(const std::string& input) {
		resolverParams.sampleValiditySeconds, resolverParams.successThreshold,
		resolverParams.minSamples, resolverParams.maxSamples,
		resolverParams.baseTimeoutMsec, resolverParams.retryCount,
		resolverParams.tlsServers, resolverParams.tlsFingerprints);

		std::set<std::vector<uint8_t>> decoded_fingerprints;
		for (const std::string& fingerprint : resolverParams.tlsFingerprints) {
		std::vector<uint8_t> decoded = parseBase64(fingerprint);
		if (decoded.empty()) {
		return ::ndk::ScopedAStatus(AStatus_fromServiceSpecificErrorWithMessage(
		EINVAL, "ResolverController error: bad fingerprint"));
		}
		decoded_fingerprints.emplace(decoded);
		}

		int res =
		gDnsResolv->resolverCtrl.setResolverConfiguration(resolverParams, decoded_fingerprints);
		resolverParams.tlsName, resolverParams.tlsServers);

		int res = gDnsResolv->resolverCtrl.setResolverConfiguration(resolverParams);
		gResNetdCallbacks.log(entry.returns(res).withAutomaticDuration().toString().c_str());

		return statusFromErrcode(res);

DnsTlsServer.cpp

+2 −3

Original line number	Diff line number	Diff line
		@@ -88,7 +88,7 @@ static bool operator ==(const sockaddr_storage& x, const sockaddr_storage& y) {
		namespace android {
		namespace net {

		// This comparison ignores ports and fingerprints.
		// This comparison ignores ports and certificates.
		bool AddressComparator::operator() (const DnsTlsServer& x, const DnsTlsServer& y) const {
		if (x.ss.ss_family != y.ss.ss_family) {
		return x.ss.ss_family < y.ss.ss_family;
		@@ -112,7 +112,6 @@ auto make_tie(const DnsTlsServer& s) {
		return std::tie(
		s.ss,
		s.name,
		s.fingerprints,
		s.protocol
		);
		}
		@@ -126,7 +125,7 @@ bool DnsTlsServer::operator ==(const DnsTlsServer& other) const {
		}

		bool DnsTlsServer::wasExplicitlyConfigured() const {
		return !name.empty() \|\| !fingerprints.empty();
		return !name.empty();
		}

		} // namespace net

DnsTlsServer.h

+4 −6

Original line number	Diff line number	Diff line
		@@ -47,16 +47,14 @@ struct DnsTlsServer {
		// The server location, including IP and port.
		sockaddr_storage ss = {};

		// A set of SHA256 public key fingerprints. If this set is nonempty, the server
		// must present a self-consistent certificate chain that contains a certificate
		// whose public key matches one of these fingerprints. Otherwise, the client will
		// terminate the connection.
		std::set<std::vector<uint8_t>> fingerprints;

		// The server's hostname. If this string is nonempty, the server must present a
		// certificate that indicates this name and has a valid chain to a trusted root CA.
		std::string name;

		// The certificate of the CA that signed the server's certificate.
		// It is used to store temporary test CA certificate for internal tests.
		std::string certificate;

		// Placeholder. More protocols might be defined in the future.
		int protocol = IPPROTO_TCP;

DnsTlsSocket.cpp

+33 −73

Original line number	Diff line number	Diff line
		@@ -39,6 +39,11 @@
		#include "netdutils/SocketOption.h"
		#include "private/android_filesystem_config.h" // AID_DNS

		// NOTE: Inject CA certificate for internal testing -- do NOT enable in production builds
		#ifndef RESOLV_INJECT_CA_CERTIFICATE
		#define RESOLV_INJECT_CA_CERTIFICATE 0
		#endif

		namespace android {

		using netdutils::enableSockopt;
		@@ -51,7 +56,6 @@ namespace net {
		namespace {

		constexpr const char kCaCertDir[] = "/system/etc/security/cacerts";
		constexpr size_t SHA256_SIZE = SHA256_DIGEST_LENGTH;

		int waitForReading(int fd) {
		struct pollfd fds = { .fd = fd, .events = POLLIN };
		@@ -121,31 +125,27 @@ Status DnsTlsSocket::tcpConnect() {
		return netdutils::status::ok;
		}

		bool getSPKIDigest(const X509* cert, std::vector<uint8_t>* out) {
		int spki_len = i2d_X509_PUBKEY(X509_get_X509_PUBKEY(cert), nullptr);
		unsigned char spki[spki_len];
		unsigned char* temp = spki;
		if (spki_len != i2d_X509_PUBKEY(X509_get_X509_PUBKEY(cert), &temp)) {
		LOG(WARNING) << "SPKI length mismatch";
		return false;
		}
		out->resize(SHA256_SIZE);
		unsigned int digest_len = 0;
		int ret = EVP_Digest(spki, spki_len, out->data(), &digest_len, EVP_sha256(), nullptr);
		if (ret != 1) {
		LOG(WARNING) << "Server cert digest extraction failed";
		bool DnsTlsSocket::setTestCaCertificate() {
		bssl::UniquePtr<BIO> bio(
		BIO_new_mem_buf(mServer.certificate.data(), mServer.certificate.size()));
		bssl::UniquePtr<X509> cert(PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr));
		if (!cert) {
		LOG(ERROR) << "Failed to read cert";
		return false;
		}
		if (digest_len != out->size()) {
		LOG(WARNING) << "Wrong digest length: " << digest_len;

		X509_STORE* cert_store = SSL_CTX_get_cert_store(mSslCtx.get());
		if (!X509_STORE_add_cert(cert_store, cert.get())) {
		LOG(ERROR) << "Failed to add cert";
		return false;
		}
		return true;
		}

		// TODO: Try to use static sSslCtx instead of mSslCtx
		bool DnsTlsSocket::initialize() {
		// This method should only be called once, at the beginning, so locking should be
		// unnecessary. This lock only serves to help catch bugs in code that calls this method.
		// This method is called every time when a new SSL connection is created.
		// This lock only serves to help catch bugs in code that calls this method.
		std::lock_guard guard(mLock);
		if (mSslCtx) {
		// This is a bug in the caller.
		@@ -156,13 +156,22 @@ bool DnsTlsSocket::initialize() {
		return false;
		}

		// Load system CA certs for hostname verification.
		// Load system CA certs from CAPath for hostname verification.
		//
		// For discussion of alternative, sustainable approaches see b/71909242.
		if (RESOLV_INJECT_CA_CERTIFICATE && !mServer.certificate.empty()) {
		// Inject test CA certs from ResolverParamsParcel.caCertificate for internal testing.
		LOG(WARNING) << "test CA certificate is valid";
		if (!setTestCaCertificate()) {
		LOG(ERROR) << "Failed to set test CA certificate";
		return false;
		}
		} else {
		if (SSL_CTX_load_verify_locations(mSslCtx.get(), nullptr, kCaCertDir) != 1) {
		LOG(ERROR) << "Failed to load CA cert dir: " << kCaCertDir;
		return false;
		}
		}

		// Enable TLS false start
		SSL_CTX_set_false_start_allowed_without_alpn(mSslCtx.get(), 1);
		@@ -210,8 +219,9 @@ bssl::UniquePtr<SSL> DnsTlsSocket::sslConnect(int fd) {
		}

		if (!mServer.name.empty()) {
		LOG(VERBOSE) << "Checking DNS over TLS hostname = " << mServer.name.c_str();
		if (SSL_set_tlsext_host_name(ssl.get(), mServer.name.c_str()) != 1) {
		LOG(ERROR) << "ailed to set SNI to " << mServer.name;
		LOG(ERROR) << "Failed to set SNI to " << mServer.name;
		return nullptr;
		}
		X509_VERIFY_PARAM* param = SSL_get0_param(ssl.get());
		@@ -258,56 +268,6 @@ bssl::UniquePtr<SSL> DnsTlsSocket::sslConnect(int fd) {
		}
		}

		// TODO: Call SSL_shutdown before discarding the session if validation fails.
		if (!mServer.fingerprints.empty()) {
		LOG(DEBUG) << "Checking DNS over TLS fingerprint";

		// We only care that the chain is internally self-consistent, not that
		// it chains to a trusted root, so we can ignore some kinds of errors.
		// TODO: Add a CA root verification mode that respects these errors.
		int verify_result = SSL_get_verify_result(ssl.get());
		switch (verify_result) {
		case X509_V_OK:
		case X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT:
		case X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN:
		case X509_V_ERR_CERT_UNTRUSTED:
		break;
		default:
		LOG(WARNING) << "Invalid certificate chain, error " << verify_result;
		return nullptr;
		}

		STACK_OF(X509) *chain = SSL_get_peer_cert_chain(ssl.get());
		if (!chain) {
		LOG(WARNING) << "Server has null certificate";
		return nullptr;
		}
		// Chain and its contents are owned by ssl, so we don't need to free explicitly.
		bool matched = false;
		for (size_t i = 0; i < sk_X509_num(chain); ++i) {
		// This appears to be O(N^2), but there doesn't seem to be a straightforward
		// way to walk a STACK_OF nondestructively in linear time.
		X509* cert = sk_X509_value(chain, i);
		std::vector<uint8_t> digest;
		if (!getSPKIDigest(cert, &digest)) {
		LOG(ERROR) << "Digest computation failed";
		return nullptr;
		}

		if (mServer.fingerprints.count(digest) > 0) {
		matched = true;
		break;
		}
		}

		if (!matched) {
		LOG(WARNING) << "No matching fingerprint";
		return nullptr;
		}

		LOG(DEBUG) << "DNS over TLS fingerprint is correct";
		}

		LOG(DEBUG) << mMark << " handshake complete";

		return ssl;