From 14f437f2efe72b95a947a45df41d57390291286b Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Fri, 17 May 2024 11:57:55 +0200 Subject: [PATCH] dnsdist: Get rid of assert() PowerDNS Security Advisory 2024-03 has made it clear that some of them that have been designed to break during testing might break in production, as we compile with `NDEBUG` unset. --- pdns/dnsdistdist/dnsdist-ecs.cc | 37 +++++++++++++++++++++++---------- pdns/ednsoptions.cc | 14 +++++++------ pdns/ipcipher.cc | 16 ++++++++++---- 3 files changed, 46 insertions(+), 21 deletions(-) diff --git a/pdns/dnsdistdist/dnsdist-ecs.cc b/pdns/dnsdistdist/dnsdist-ecs.cc index 78ab5e14b184..988345640745 100644 --- a/pdns/dnsdistdist/dnsdist-ecs.cc +++ b/pdns/dnsdistdist/dnsdist-ecs.cc @@ -44,7 +44,10 @@ bool g_addEDNSToSelfGeneratedResponses{true}; int rewriteResponseWithoutEDNS(const PacketBuffer& initialPacket, PacketBuffer& newContent) { - assert(initialPacket.size() >= sizeof(dnsheader)); + if (initialPacket.size() < sizeof(dnsheader)) { + return ENOENT; + } + const dnsheader_aligned dnsHeader(initialPacket.data()); if (ntohs(dnsHeader->arcount) == 0) { @@ -153,7 +156,10 @@ static bool addOrReplaceEDNSOption(std::vector> bool slowRewriteEDNSOptionInQueryWithRecords(const PacketBuffer& initialPacket, PacketBuffer& newContent, bool& ednsAdded, uint16_t optionToReplace, bool& optionAdded, bool overrideExisting, const string& newOptionContent) { - assert(initialPacket.size() >= sizeof(dnsheader)); + if (initialPacket.size() < sizeof(dnsheader)) { + return false; + } + const dnsheader_aligned dnsHeader(initialPacket.data()); if (ntohs(dnsHeader->qdcount) == 0) { @@ -321,9 +327,10 @@ static bool slowParseEDNSOptions(const PacketBuffer& packet, EDNSOptionViewMap& int locateEDNSOptRR(const PacketBuffer& packet, uint16_t* optStart, size_t* optLen, bool* last) { - assert(optStart != nullptr); - assert(optLen != nullptr); - assert(last != nullptr); + if (optStart == nullptr || optLen == nullptr || last == nullptr) { + throw std::runtime_error("Invalid values passed to locateEDNSOptRR"); + } + const dnsheader_aligned dnsHeader(packet.data()); if (ntohs(dnsHeader->arcount) == 0) { @@ -390,8 +397,10 @@ int locateEDNSOptRR(const PacketBuffer& packet, uint16_t* optStart, size_t* optL /* extract the start of the OPT RR in a QUERY packet if any */ int getEDNSOptionsStart(const PacketBuffer& packet, const size_t offset, uint16_t* optRDPosition, size_t* remaining) { - assert(optRDPosition != nullptr); - assert(remaining != nullptr); + if (optRDPosition == nullptr || remaining == nullptr) { + throw std::runtime_error("Invalid values passed to getEDNSOptionsStart"); + } + const dnsheader_aligned dnsHeader(packet.data()); if (offset >= packet.size()) { @@ -474,8 +483,9 @@ bool generateOptRR(const std::string& optRData, PacketBuffer& res, size_t maximu static bool replaceEDNSClientSubnetOption(PacketBuffer& packet, size_t maximumSize, size_t const oldEcsOptionStartPosition, size_t const oldEcsOptionSize, size_t const optRDLenPosition, const string& newECSOption) { - assert(oldEcsOptionStartPosition < packet.size()); - assert(optRDLenPosition < packet.size()); + if (oldEcsOptionStartPosition >= packet.size() || optRDLenPosition >= packet.size()) { + throw std::runtime_error("Invalid values passed to replaceEDNSClientSubnetOption"); + } if (newECSOption.size() == oldEcsOptionSize) { /* same size as the existing option */ @@ -593,7 +603,9 @@ static bool addEDNSWithECS(PacketBuffer& packet, size_t maximumSize, const strin bool handleEDNSClientSubnet(PacketBuffer& packet, const size_t maximumSize, const size_t qnameWireLength, bool& ednsAdded, bool& ecsAdded, bool overrideExisting, const string& newECSOption) { - assert(qnameWireLength <= packet.size()); + if (qnameWireLength > packet.size()) { + throw std::runtime_error("Invalid value passed to handleEDNSClientSubnet"); + } const dnsheader_aligned dnsHeader(packet.data()); @@ -763,7 +775,10 @@ bool isEDNSOptionInOpt(const PacketBuffer& packet, const size_t optStart, const int rewriteResponseWithoutEDNSOption(const PacketBuffer& initialPacket, const uint16_t optionCodeToSkip, PacketBuffer& newContent) { - assert(initialPacket.size() >= sizeof(dnsheader)); + if (initialPacket.size() < sizeof(dnsheader)) { + return ENOENT; + } + const dnsheader_aligned dnsHeader(initialPacket.data()); if (ntohs(dnsHeader->arcount) == 0) { diff --git a/pdns/ednsoptions.cc b/pdns/ednsoptions.cc index 8221f7201907..93c127e554c5 100644 --- a/pdns/ednsoptions.cc +++ b/pdns/ednsoptions.cc @@ -45,12 +45,14 @@ bool getNextEDNSOption(const char* data, size_t dataLen, uint16_t& optionCode, u /* extract the position (relative to the optRR pointer!) and size of a specific EDNS0 option from a pointer on the beginning rdLen of the OPT RR */ int getEDNSOption(const char* optRR, const size_t len, uint16_t wantedOption, size_t* optionValuePosition, size_t * optionValueSize) { - assert(optRR != nullptr); - assert(optionValuePosition != nullptr); - assert(optionValueSize != nullptr); + if (optRR == nullptr || optionValuePosition == nullptr || optionValueSize == nullptr) { + return EINVAL; + } + size_t pos = 0; - if (len < DNS_RDLENGTH_SIZE) + if (len < DNS_RDLENGTH_SIZE) { return EINVAL; + } const uint16_t rdLen = (((unsigned char) optRR[pos]) * 256) + ((unsigned char) optRR[pos+1]); size_t rdPos = 0; @@ -94,10 +96,10 @@ int getEDNSOption(const char* optRR, const size_t len, uint16_t wantedOption, si /* extract all EDNS0 options from a pointer on the beginning rdLen of the OPT RR */ int getEDNSOptions(const char* optRR, const size_t len, EDNSOptionViewMap& options) { - assert(optRR != nullptr); size_t pos = 0; - if (len < DNS_RDLENGTH_SIZE) + if (optRR == nullptr || len < DNS_RDLENGTH_SIZE) { return EINVAL; + } const uint16_t rdLen = (((unsigned char) optRR[pos]) * 256) + ((unsigned char) optRR[pos+1]); size_t rdPos = 0; diff --git a/pdns/ipcipher.cc b/pdns/ipcipher.cc index 1d64cc902dd7..d4782aafa659 100644 --- a/pdns/ipcipher.cc +++ b/pdns/ipcipher.cc @@ -77,7 +77,9 @@ static ComboAddress encryptCA6(const ComboAddress& address, const std::string& k const auto inSize = sizeof(address.sin6.sin6_addr.s6_addr); static_assert(inSize == 16, "We disable padding and so we must assume a data size of 16 bytes"); const auto blockSize = EVP_CIPHER_get_block_size(aes128cbc.get()); - assert(blockSize == 16); + if (blockSize != 16) { + throw pdns::OpenSSL::error("encryptCA6: unexpected block size"); + } EVP_CIPHER_CTX_set_padding(ctx.get(), 0); int updateLen = 0; @@ -94,7 +96,9 @@ static ComboAddress encryptCA6(const ComboAddress& address, const std::string& k throw pdns::OpenSSL::error("encryptCA6: Could not finalize address encryption"); } - assert(updateLen + finalLen == inSize); + if ((updateLen + finalLen) != inSize) { + throw pdns::OpenSSL::error("encryptCA6: unexpected final size"); + } #else AES_KEY wctx; AES_set_encrypt_key((const unsigned char*)key.c_str(), 128, &wctx); @@ -130,7 +134,9 @@ static ComboAddress decryptCA6(const ComboAddress& address, const std::string& k const auto inSize = sizeof(address.sin6.sin6_addr.s6_addr); static_assert(inSize == 16, "We disable padding and so we must assume a data size of 16 bytes"); const auto blockSize = EVP_CIPHER_get_block_size(aes128cbc.get()); - assert(blockSize == 16); + if (blockSize != 16) { + throw pdns::OpenSSL::error("decryptCA6: unexpected block size"); + } EVP_CIPHER_CTX_set_padding(ctx.get(), 0); int updateLen = 0; @@ -147,7 +153,9 @@ static ComboAddress decryptCA6(const ComboAddress& address, const std::string& k throw pdns::OpenSSL::error("decryptCA6: Could not finalize address decryption"); } - assert(updateLen + finalLen == inSize); + if ((updateLen + finalLen) != inSize) { + throw pdns::OpenSSL::error("decryptCA6: unexpected final size"); + } #else AES_KEY wctx; AES_set_decrypt_key((const unsigned char*)key.c_str(), 128, &wctx);