Skip to content

Commit

Permalink
API Auth: forbid ttl < 0
Browse files Browse the repository at this point in the history
  • Loading branch information
@zeha
zeha committed Aug 11, 2023
1 parent 70f1db7 commit cf17e6b
Show file tree
Hide file tree
Showing 4 changed files with 125 additions and 80 deletions.
83 changes: 53 additions & 30 deletions pdns/json.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<int>(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()) {
Expand All @@ -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)
Expand All @@ -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);
}
2 changes: 2 additions & 0 deletions pdns/json.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
112 changes: 63 additions & 49 deletions pdns/ws-auth.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -1718,25 +1718,30 @@ static void apiServerZones(HttpRequest* req, HttpResponse* resp) {
vector<DNSResourceRecord> new_records;
vector<Comment> 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) {
Expand Down Expand Up @@ -1916,22 +1921,27 @@ static void apiServerZoneDetail(HttpRequest* req, HttpResponse* resp) {
vector<DNSResourceRecord> new_records;
vector<Comment> 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();
Expand Down Expand Up @@ -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;
Expand Down
8 changes: 7 additions & 1 deletion regression-tests.api/test_Zones.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down

0 comments on commit cf17e6b

Please sign in to comment.