From cf17e6b8f8b6127abe38c458d15fb275724a7f01 Mon Sep 17 00:00:00 2001 From: Chris Hofstaedtler Date: Fri, 3 Feb 2023 11:13:59 +0100 Subject: [PATCH] API Auth: forbid ttl < 0 --- pdns/json.cc | 83 +++++++++++++-------- pdns/json.hh | 2 + pdns/ws-auth.cc | 112 ++++++++++++++++------------- regression-tests.api/test_Zones.py | 8 ++- 4 files changed, 125 insertions(+), 80 deletions(-) diff --git a/pdns/json.cc b/pdns/json.cc index 929fc81cc1cc..285a953aefcb 100644 --- a/pdns/json.cc +++ b/pdns/json.cc @@ -28,36 +28,55 @@ using json11::Json; -int intFromJson(const Json& container, const std::string& key) +static inline int intFromJsonInternal(const Json& container, const std::string& key, const bool have_default, const int default_value) { auto val = container[key]; if (val.is_number()) { return val.int_value(); } else if (val.is_string()) { - return std::stoi(val.string_value()); + try { + return std::stoi(val.string_value()); + } catch (std::out_of_range&) { + throw JsonException("Key '" + string(key) + "' is out of range"); + } } else { + if (have_default) { + return default_value; + } throw JsonException("Key '" + string(key) + "' not an Integer or not present"); } } +int intFromJson(const Json& container, const std::string& key) +{ + return intFromJsonInternal(container, key, false, 0); +} + int intFromJson(const Json& container, const std::string& key, const int default_value) { - auto val = container[key]; - if (val.is_number()) { - return val.int_value(); - } else if (val.is_string()) { - try { - return std::stoi(val.string_value()); - } catch (std::out_of_range&) { - throw JsonException("Value for key '" + string(key) + "' is out of range"); - } - } else { - // TODO: check if value really isn't present - return default_value; + return intFromJsonInternal(container, key, true, default_value); +} + +static inline unsigned int uintFromJsonInternal(const Json& container, const std::string& key, const bool have_default, const unsigned int default_value) +{ + int intval = intFromJsonInternal(container, key, have_default, static_cast(default_value)); + if (intval >= 0) { + return intval; } + throw JsonException("Key '" + string(key) + "' is not a positive Integer"); } -double doubleFromJson(const Json& container, const std::string& key) +unsigned int uintFromJson(const Json& container, const std::string& key) +{ + return uintFromJsonInternal(container, key, false, 0); +} + +unsigned int uintFromJson(const Json& container, const std::string& key, const unsigned int default_value) +{ + return uintFromJsonInternal(container, key, true, default_value); +} + +static inline double doubleFromJsonInternal(const Json& container, const std::string& key, const bool have_default, const double default_value) { auto val = container[key]; if (val.is_number()) { @@ -69,21 +88,21 @@ double doubleFromJson(const Json& container, const std::string& key) throw JsonException("Value for key '" + string(key) + "' is out of range"); } } else { + if (have_default) { + return default_value; + } throw JsonException("Key '" + string(key) + "' not an Integer or not present"); } } +double doubleFromJson(const Json& container, const std::string& key) +{ + return doubleFromJsonInternal(container, key, false, 0); +} + double doubleFromJson(const Json& container, const std::string& key, const double default_value) { - auto val = container[key]; - if (val.is_number()) { - return val.number_value(); - } else if (val.is_string()) { - return std::stod(val.string_value()); - } else { - // TODO: check if value really isn't present - return default_value; - } + return doubleFromJsonInternal(container, key, true, default_value); } string stringFromJson(const Json& container, const std::string &key) @@ -96,20 +115,24 @@ string stringFromJson(const Json& container, const std::string &key) } } -bool boolFromJson(const Json& container, const std::string& key) +static inline bool boolFromJsonInternal(const Json& container, const std::string& key, const bool have_default, const bool default_value) { auto val = container[key]; if (val.is_bool()) { return val.bool_value(); } + if (have_default) { + return default_value; + } throw JsonException("Key '" + string(key) + "' not present or not a Bool"); } +bool boolFromJson(const Json& container, const std::string& key) +{ + return boolFromJsonInternal(container, key, false, false); +} + bool boolFromJson(const Json& container, const std::string& key, const bool default_value) { - auto val = container[key]; - if (val.is_bool()) { - return val.bool_value(); - } - return default_value; + return boolFromJsonInternal(container, key, true, default_value); } diff --git a/pdns/json.hh b/pdns/json.hh index db2703641bd3..ef5de1fbb1ea 100644 --- a/pdns/json.hh +++ b/pdns/json.hh @@ -27,6 +27,8 @@ int intFromJson(const json11::Json& container, const std::string& key); int intFromJson(const json11::Json& container, const std::string& key, const int default_value); +unsigned int uintFromJson(const json11::Json& container, const std::string& key); +unsigned int uintFromJson(const json11::Json& container, const std::string& key, const unsigned int default_value); double doubleFromJson(const json11::Json& container, const std::string& key); double doubleFromJson(const json11::Json& container, const std::string& key, const double default_value); std::string stringFromJson(const json11::Json& container, const std::string &key); diff --git a/pdns/ws-auth.cc b/pdns/ws-auth.cc index fc8eaca2c849..bb854c91be48 100644 --- a/pdns/ws-auth.cc +++ b/pdns/ws-auth.cc @@ -583,8 +583,8 @@ static void gatherComments(const Json& container, const DNSName& qname, const QT time_t now = time(nullptr); for (const auto& comment : container["comments"].array_items()) { - // FIXME this is converting to a *signed* int, 2036 issue - c.modified_at = intFromJson(comment, "modified_at", now); + // FIXME 2036 issue internally in uintFromJson + c.modified_at = uintFromJson(comment, "modified_at", now); c.content = stringFromJson(comment, "content"); c.account = stringFromJson(comment, "account"); new_comments.push_back(c); @@ -1718,25 +1718,30 @@ static void apiServerZones(HttpRequest* req, HttpResponse* resp) { vector new_records; vector new_comments; - if (rrsets.is_array()) { - for (const auto& rrset : rrsets.array_items()) { - DNSName qname = apiNameToDNSName(stringFromJson(rrset, "name")); - apiCheckQNameAllowedCharacters(qname.toString()); - QType qtype; - qtype = stringFromJson(rrset, "type"); - if (qtype.getCode() == 0) { - throw ApiException("RRset "+qname.toString()+" IN "+stringFromJson(rrset, "type")+": unknown type given"); - } - if (rrset["records"].is_array()) { - int ttl = intFromJson(rrset, "ttl"); - gatherRecords(rrset, qname, qtype, ttl, new_records); - } - if (rrset["comments"].is_array()) { - gatherComments(rrset, qname, qtype, new_comments); + try { + if (rrsets.is_array()) { + for (const auto& rrset : rrsets.array_items()) { + DNSName qname = apiNameToDNSName(stringFromJson(rrset, "name")); + apiCheckQNameAllowedCharacters(qname.toString()); + QType qtype; + qtype = stringFromJson(rrset, "type"); + if (qtype.getCode() == 0) { + throw ApiException("RRset "+qname.toString()+" IN "+stringFromJson(rrset, "type")+": unknown type given"); + } + if (rrset["records"].is_array()) { + int ttl = uintFromJson(rrset, "ttl"); + gatherRecords(rrset, qname, qtype, ttl, new_records); + } + if (rrset["comments"].is_array()) { + gatherComments(rrset, qname, qtype, new_comments); + } } + } else if (zonestring != "") { + gatherRecordsFromZone(zonestring, new_records, zonename); } - } else if (zonestring != "") { - gatherRecordsFromZone(zonestring, new_records, zonename); + } + catch (const JsonException& e) { + throw ApiException("New RRsets are invalid: " + string(e.what())); } for(auto& rr : new_records) { @@ -1916,22 +1921,27 @@ static void apiServerZoneDetail(HttpRequest* req, HttpResponse* resp) { vector new_records; vector new_comments; - for (const auto& rrset : rrsets.array_items()) { - DNSName qname = apiNameToDNSName(stringFromJson(rrset, "name")); - apiCheckQNameAllowedCharacters(qname.toString()); - QType qtype; - qtype = stringFromJson(rrset, "type"); - if (qtype.getCode() == 0) { - throw ApiException("RRset "+qname.toString()+" IN "+stringFromJson(rrset, "type")+": unknown type given"); - } - if (rrset["records"].is_array()) { - int ttl = intFromJson(rrset, "ttl"); - gatherRecords(rrset, qname, qtype, ttl, new_records); - } - if (rrset["comments"].is_array()) { - gatherComments(rrset, qname, qtype, new_comments); + try { + for (const auto& rrset : rrsets.array_items()) { + DNSName qname = apiNameToDNSName(stringFromJson(rrset, "name")); + apiCheckQNameAllowedCharacters(qname.toString()); + QType qtype; + qtype = stringFromJson(rrset, "type"); + if (qtype.getCode() == 0) { + throw ApiException("RRset "+qname.toString()+" IN "+stringFromJson(rrset, "type")+": unknown type given"); + } + if (rrset["records"].is_array()) { + int ttl = uintFromJson(rrset, "ttl"); + gatherRecords(rrset, qname, qtype, ttl, new_records); + } + if (rrset["comments"].is_array()) { + gatherComments(rrset, qname, qtype, new_comments); + } } } + catch (const JsonException& e) { + throw ApiException("New RRsets are invalid: " + string(e.what())); + } for(auto& rr : new_records) { rr.qname.makeUsLowerCase(); @@ -2177,28 +2187,32 @@ static void patchZone(UeberBackend& B, HttpRequest* req, HttpResponse* resp) { new_records.clear(); new_comments.clear(); - if (replace_records) { - // ttl shouldn't be part of DELETE, and it shouldn't be required if we don't get new records. - int ttl = intFromJson(rrset, "ttl"); - - gatherRecords(rrset, qname, qtype, ttl, new_records); - - for(DNSResourceRecord& rr : new_records) { - rr.domain_id = di.id; - if (rr.qtype.getCode() == QType::SOA && rr.qname==zonename) { - soa_edit_done = increaseSOARecord(rr, soa_edit_api_kind, soa_edit_kind); + try { + if (replace_records) { + // ttl shouldn't be part of DELETE, and it shouldn't be required if we don't get new records. + int ttl = uintFromJson(rrset, "ttl"); + gatherRecords(rrset, qname, qtype, ttl, new_records); + + for(DNSResourceRecord& rr : new_records) { + rr.domain_id = di.id; + if (rr.qtype.getCode() == QType::SOA && rr.qname==zonename) { + soa_edit_done = increaseSOARecord(rr, soa_edit_api_kind, soa_edit_kind); + } } + checkNewRecords(new_records, zonename); } - checkNewRecords(new_records, zonename); - } - if (replace_comments) { - gatherComments(rrset, qname, qtype, new_comments); + if (replace_comments) { + gatherComments(rrset, qname, qtype, new_comments); - for(Comment& c : new_comments) { - c.domain_id = di.id; + for(Comment& c : new_comments) { + c.domain_id = di.id; + } } } + catch (const JsonException& e) { + throw ApiException("New RRsets are invalid: " + string(e.what())); + } if (replace_records) { bool ent_present = false; diff --git a/regression-tests.api/test_Zones.py b/regression-tests.api/test_Zones.py index 050eef9ab94f..18d39643e782 100644 --- a/regression-tests.api/test_Zones.py +++ b/regression-tests.api/test_Zones.py @@ -1945,7 +1945,7 @@ def test_zone_comment_out_of_range_modified_at(self): data=json.dumps(payload), headers={'content-type': 'application/json'}) self.assertEqual(r.status_code, 422) - self.assertIn("Value for key 'modified_at' is out of range", r.json()['error']) + self.assertIn("Key 'modified_at' is out of range", r.json()['error']) @unittest.skipIf(is_auth_lmdb(), "No comments in LMDB") def test_zone_comment_stay_intact(self): @@ -2372,6 +2372,12 @@ def test_zone_replace_rrsets_no_soa_secondary(self): name, _, _ = self.create_zone(kind='Secondary', nameservers=None, masters=['127.0.0.2']) self.put_zone(name, {'rrsets': []}, expect_error='Must give SOA record for zone when replacing all RR sets') + def test_zone_replace_rrsets_negative_ttl(self): + name, _, _ = self.create_zone(dnssec=False, soa_edit='', soa_edit_api='') + rrsets = [ + {'name': name, 'type': 'SOA', 'ttl': -1, 'records': [{'content': 'invalid. hostmaster.invalid. 1 10800 3600 604800 3600'}]}, + ] + self.put_zone(name, {'rrsets': rrsets}, expect_error="Key 'ttl' is not a positive Integer") @unittest.skipIf(not is_auth(), "Not applicable") class AuthRootZone(ApiTestCase, AuthZonesHelperMixin):