Skip to content

Commit

Permalink
LookupServer: Verify that DNS response questions match the request
Browse files Browse the repository at this point in the history
To protect against DNS spoof attacks, we now check that the questions
in incoming responses match the questions in the request we sent out.

Suggested by @zecke in SerenityOS#10.
  • Loading branch information
awesomekling committed Jan 26, 2020
1 parent b4d55b1 commit 00be9b3
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 3 deletions.
10 changes: 10 additions & 0 deletions Servers/LookupServer/DNSQuestion.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ class DNSQuestion {
u16 class_code() const { return m_class_code; }
const String& name() const { return m_name; }

bool operator==(const DNSQuestion& other) const
{
return m_name == other.m_name && m_record_type == other.m_record_type && m_class_code == other.m_class_code;
}

bool operator!=(const DNSQuestion& other) const
{
return !(*this == other);
}

private:
String m_name;
u16 m_record_type { 0 };
Expand Down
2 changes: 2 additions & 0 deletions Servers/LookupServer/DNSRequest.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ class DNSRequest {

void add_question(const String& name, u16 record_type);

const Vector<DNSQuestion>& questions() const { return m_questions; }

u16 question_count() const
{
ASSERT(m_questions.size() < UINT16_MAX);
Expand Down
3 changes: 1 addition & 2 deletions Servers/LookupServer/DNSResponse.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ class [[gnu::packed]] DNSRecordWithoutName

static_assert(sizeof(DNSRecordWithoutName) == 10);


Optional<DNSResponse> DNSResponse::from_raw_response(const u8* raw_data, size_t raw_size)
{
if (raw_size < sizeof(DNSPacket)) {
Expand Down Expand Up @@ -79,7 +78,7 @@ Optional<DNSResponse> DNSResponse::from_raw_response(const u8* raw_data, size_t
// FIXME: Parse some other record types perhaps?
dbg() << " data=(unimplemented record type " << record.type() << ")";
}
dbg() << "Answer #" << i << ": type=" << record.type() << ", ttl=" << record.ttl() << ", length=" << record.data_length() << ", data=_" << data << "_";
dbg() << "Answer #" << i << ": name=_" << name << "_, type=" << record.type() << ", ttl=" << record.ttl() << ", length=" << record.data_length() << ", data=_" << data << "_";
response.m_answers.empend(name, record.type(), record.record_class(), record.ttl(), data);
offset += record.data_length();
}
Expand Down
14 changes: 13 additions & 1 deletion Servers/LookupServer/LookupServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -216,10 +216,22 @@ Vector<String> LookupServer::lookup(const String& hostname, bool& did_timeout, u
dbgprintf("LookupServer: ID mismatch (%u vs %u) :(\n", response.id(), request.id());
return {};
}
if (response.question_count() != 1) {
if (response.question_count() != request.question_count()) {
dbgprintf("LookupServer: Question count (%u vs %u) :(\n", response.question_count(), request.question_count());
return {};
}

for (size_t i = 0; i < request.question_count(); ++i) {
auto& request_question = request.questions()[i];
auto& response_question = response.questions()[i];
if (request_question != response_question) {
dbg() << "Request and response questions do not match";
dbg() << " Request: {_" << request_question.name() << "_, " << request_question.record_type() << ", " << request_question.class_code() << "}";
dbg() << " Response: {_" << response_question.name() << "_, " << response_question.record_type() << ", " << response_question.class_code() << "}";
return {};
}
}

if (response.answer_count() < 1) {
dbgprintf("LookupServer: Not enough answers (%u) :(\n", response.answer_count());
return {};
Expand Down

0 comments on commit 00be9b3

Please sign in to comment.