From 8ca3ea3384927e434e9b0c69ce25b9a927aa3640 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Tue, 3 May 2016 10:39:53 +0200 Subject: [PATCH] auth: Move key validity check out of `fromISCMap()` It doesn't make a lot of sense to check the key validity at every call of `fromISCMap()`, and it hurts performance a lot when keys are not cached. * Add separate `DNSSECKeeper::checkKeys()` and `DNSCryptoKeyEngine::checkKeys()` methods * Key validity is checked on import-zone-key, check-zone and test-algorithm(s) --- pdns/dbdnsseckeeper.cc | 16 ++++++++++++++++ pdns/dnssecinfra.cc | 10 +++++++++- pdns/dnssecinfra.hh | 5 ++++- pdns/dnsseckeeper.hh | 1 + pdns/opensslsigners.cc | 31 ++++++++++++------------------- pdns/pdnsutil.cc | 6 ++++++ 6 files changed, 48 insertions(+), 21 deletions(-) diff --git a/pdns/dbdnsseckeeper.cc b/pdns/dbdnsseckeeper.cc index 5f430d400..e62180c89 100644 --- a/pdns/dbdnsseckeeper.cc +++ b/pdns/dbdnsseckeeper.cc @@ -468,6 +468,22 @@ DNSSECKeeper::keyset_t DNSSECKeeper::getKeys(const DNSName& zone, bool useCache) return retkeyset; } +bool DNSSECKeeper::checkKeys(const DNSName& zone) +{ + vector dbkeyset; + d_keymetadb->getDomainKeys(zone, 0, dbkeyset); + + for(const DNSBackend::KeyData &keydata : dbkeyset) { + DNSKEYRecordContent dkrc; + shared_ptr dke(DNSCryptoKeyEngine::makeFromISCString(dkrc, keydata.content)); + if (!dke->checkKeys()) { + return false; + } + } + + return true; +} + bool DNSSECKeeper::getPreRRSIGs(UeberBackend& db, const DNSName& signer, const DNSName& qname, const DNSName& wildcardname, const QType& qtype, DNSResourceRecord::Place signPlace, vector& rrsigs, uint32_t signTTL) diff --git a/pdns/dnssecinfra.cc b/pdns/dnssecinfra.cc index 859dc824d..30a350cc0 100644 --- a/pdns/dnssecinfra.cc +++ b/pdns/dnssecinfra.cc @@ -38,7 +38,12 @@ DNSCryptoKeyEngine* DNSCryptoKeyEngine::makeFromISCFile(DNSKEYRecordContent& drc isc += sline; } fclose(fp); - return makeFromISCString(drc, isc); + DNSCryptoKeyEngine* dke = makeFromISCString(drc, isc); + if(!dke->checkKeys()) { + delete dke; + throw runtime_error("Invalid DNS Private Key in file '"+string(fname)); + } + return dke; } DNSCryptoKeyEngine* DNSCryptoKeyEngine::makeFromISCString(DNSKEYRecordContent& drc, const std::string& content) @@ -252,6 +257,9 @@ pair DNSCryptoKeyEngine::testMakers(unsigned int alg stormap[toLower(key)]=raw; } dckeSign->fromISCMap(dkrc, stormap); + if(!dckeSign->checkKeys()) { + throw runtime_error("Verification of keys with creator "+dckeCreate->getName()+" with signer "+dckeSign->getName()+" and verifier "+dckeVerify->getName()+" failed"); + } } string message("Hi! How is life?"); diff --git a/pdns/dnssecinfra.hh b/pdns/dnssecinfra.hh index 9d5cdd82b..b9620f63e 100644 --- a/pdns/dnssecinfra.hh +++ b/pdns/dnssecinfra.hh @@ -38,7 +38,10 @@ class DNSCryptoKeyEngine throw std::runtime_error("Can't import from PEM string"); } virtual void fromPublicKeyString(const std::string& content) = 0; - + virtual bool checkKeys() const + { + return true; + } static DNSCryptoKeyEngine* makeFromISCFile(DNSKEYRecordContent& drc, const char* fname); static DNSCryptoKeyEngine* makeFromISCString(DNSKEYRecordContent& drc, const std::string& content); static DNSCryptoKeyEngine* makeFromPEMString(DNSKEYRecordContent& drc, const std::string& raw); diff --git a/pdns/dnsseckeeper.hh b/pdns/dnsseckeeper.hh index 8eab6177d..e71beee6d 100644 --- a/pdns/dnsseckeeper.hh +++ b/pdns/dnsseckeeper.hh @@ -167,6 +167,7 @@ public: bool removeKey(const DNSName& zname, unsigned int id); bool activateKey(const DNSName& zname, unsigned int id); bool deactivateKey(const DNSName& zname, unsigned int id); + bool checkKeys(const DNSName& zname); bool getNSEC3PARAM(const DNSName& zname, NSEC3PARAMRecordContent* n3p=0, bool* narrow=0); bool setNSEC3PARAM(const DNSName& zname, const NSEC3PARAMRecordContent& n3p, const bool& narrow=false); diff --git a/pdns/opensslsigners.cc b/pdns/opensslsigners.cc index 49679f8c5..02c4028dd 100644 --- a/pdns/opensslsigners.cc +++ b/pdns/opensslsigners.cc @@ -99,6 +99,7 @@ public: std::string getPublicKeyString() const; void fromISCMap(DNSKEYRecordContent& drc, std::map& stormap); void fromPublicKeyString(const std::string& content); + bool checkKeys() const override; static DNSCryptoKeyEngine* maker(unsigned int algorithm) { @@ -353,18 +354,17 @@ void OpenSSLRSADNSCryptoKeyEngine::fromISCMap(DNSKEYRecordContent& drc, std::map throw runtime_error(getName()+" tried to feed an algorithm "+std::to_string(drc.d_algorithm)+" to a "+std::to_string(d_algorithm)+" key"); } - int ret = RSA_check_key(key); - if (ret != 1) { - RSA_free(key); - throw runtime_error(getName()+" invalid public key"); - } - if (d_key) RSA_free(d_key); d_key = key; } +bool OpenSSLRSADNSCryptoKeyEngine::checkKeys() const +{ + return (RSA_check_key(d_key) == 1); +} + void OpenSSLRSADNSCryptoKeyEngine::fromPublicKeyString(const std::string& input) { string exponent, modulus; @@ -409,8 +409,7 @@ void OpenSSLRSADNSCryptoKeyEngine::fromPublicKeyString(const std::string& input) RSA_free(key); throw runtime_error(getName()+" error loading n value of public key"); } - /* we cannot use RSA_check_key(), because it requires the private key information - to be present. */ + if (d_key) RSA_free(d_key); @@ -472,6 +471,7 @@ public: std::string getPublicKeyString() const; void fromISCMap(DNSKEYRecordContent& drc, std::map& stormap); void fromPublicKeyString(const std::string& content); + bool checkKeys() const override; static DNSCryptoKeyEngine* maker(unsigned int algorithm) { @@ -678,14 +678,12 @@ void OpenSSLECDSADNSCryptoKeyEngine::fromISCMap(DNSKEYRecordContent& drc, std::m } EC_POINT_free(pub_key); - - ret = EC_KEY_check_key(d_eckey); - if (ret != 1) { - throw runtime_error(getName()+" invalid public key"); - } - } +bool OpenSSLECDSADNSCryptoKeyEngine::checkKeys() const +{ + return (EC_KEY_check_key(d_eckey) == 1); +} void OpenSSLECDSADNSCryptoKeyEngine::fromPublicKeyString(const std::string& input) { @@ -717,11 +715,6 @@ void OpenSSLECDSADNSCryptoKeyEngine::fromPublicKeyString(const std::string& inpu } EC_POINT_free(pub_key); - - ret = EC_KEY_check_key(d_eckey); - if (ret != 1) { - throw runtime_error(getName()+" invalid public key"); - } } diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc index fbe6e955b..96c32eef1 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -401,6 +401,7 @@ int checkZone(DNSSECKeeper &dk, UeberBackend &B, const DNSName& zone, const vect bool isSecure=dk.isSecuredZone(zone); bool presigned=dk.isPresigned(zone); + bool validKeys=dk.checkKeys(zone); DNSResourceRecord rr; uint64_t numrecords=0, numerrors=0, numwarnings=0; @@ -410,6 +411,11 @@ int checkZone(DNSSECKeeper &dk, UeberBackend &B, const DNSName& zone, const vect cout<<"[Error] zone '" << zone.toStringNoDot() << "' has NSEC3 semantics but is too long to have the hash prepended. Zone name is " << zone.wirelength() << " bytes long, whereas the maximum is 222 bytes." << endl; } + if (!validKeys) { + numerrors++; + cout<<"[Error] zone '" << zone.toStringNoDot() << "' has at least one invalid DNS Private Key." << endl; + } + // Check for delegation in parent zone DNSName parent(zone); while(parent.chopOff()) { -- 2.49.0