Skip to content

Commit

Permalink
Validate hostnames in DNS responses and discard from malicious servers (
Browse files Browse the repository at this point in the history
#406)

To prevent possible users having XSS issues due to intentionally malformed DNS replies, validate hostnames returned in responses and return EBADRESP if they are not valid.

It is not clear what legitimate issues this may cause at this point.

Bug Reported By: philipp.jeitner@sit.fraunhofer.de
Fix By: Brad House (@bradh352)
  • Loading branch information
bradh352 committed Jun 20, 2021
1 parent 44c009b commit c9b6c60
Show file tree
Hide file tree
Showing 8 changed files with 78 additions and 28 deletions.
6 changes: 3 additions & 3 deletions src/lib/ares__parse_into_addrinfo.c
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ int ares__parse_into_addrinfo2(const unsigned char *abuf,

/* Expand the name from the question, and skip past the question. */
aptr = abuf + HFIXEDSZ;
status = ares__expand_name_for_response(aptr, abuf, alen, question_hostname, &len);
status = ares__expand_name_for_response(aptr, abuf, alen, question_hostname, &len, 0);
if (status != ARES_SUCCESS)
return status;
if (aptr + len + QFIXEDSZ > abuf + alen)
Expand All @@ -86,7 +86,7 @@ int ares__parse_into_addrinfo2(const unsigned char *abuf,
for (i = 0; i < (int)ancount; i++)
{
/* Decode the RR up to the data field. */
status = ares__expand_name_for_response(aptr, abuf, alen, &rr_name, &len);
status = ares__expand_name_for_response(aptr, abuf, alen, &rr_name, &len, 0);
if (status != ARES_SUCCESS)
{
rr_name = NULL;
Expand Down Expand Up @@ -188,7 +188,7 @@ int ares__parse_into_addrinfo2(const unsigned char *abuf,
{
got_cname = 1;
status = ares__expand_name_for_response(aptr, abuf, alen, &rr_data,
&len);
&len, 1);
if (status != ARES_SUCCESS)
{
goto failed_stat;
Expand Down
66 changes: 55 additions & 11 deletions src/lib/ares_expand_name.c
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
#define MAX_INDIRS 50

static int name_length(const unsigned char *encoded, const unsigned char *abuf,
int alen);
int alen, int is_hostname);

/* Reserved characters for names that need to be escaped */
static int is_reservedch(int ch)
Expand All @@ -52,6 +52,31 @@ static int is_reservedch(int ch)
return 0;
}

static int ares__isprint(int ch)
{
if (ch >= 0x20 && ch <= 0x7E)
return 1;
return 0;
}

/* Character set allowed by hostnames */
static int is_hostnamech(int ch)
{
/* [A-Za-z0-9-.]
* Don't use isalnum() as it is locale-specific
*/
if (ch >= 'A' && ch <= 'Z')
return 1;
if (ch >= 'a' && ch <= 'z')
return 1;
if (ch >= '0' && ch <= '9')
return 1;
if (ch == '-' || ch == '.')
return 1;

return 0;
}

/* Expand an RFC1035-encoded domain name given by encoded. The
* containing message is given by abuf and alen. The result given by
* *s, which is set to a NUL-terminated allocated buffer. *enclen is
Expand All @@ -74,10 +99,15 @@ static int is_reservedch(int ch)
*
* Since the expanded name uses '.' as a label separator, we use
* backslashes to escape periods or backslashes in the expanded name.
*
* If the result is expected to be a hostname, then no escaped data is allowed
* and will return error.
*/

int ares_expand_name(const unsigned char *encoded, const unsigned char *abuf,
int alen, char **s, long *enclen)
int ares__expand_name_validated(const unsigned char *encoded,
const unsigned char *abuf,
int alen, char **s, long *enclen,
int is_hostname)
{
int len, indir = 0;
char *q;
Expand All @@ -87,7 +117,7 @@ int ares_expand_name(const unsigned char *encoded, const unsigned char *abuf,
size_t uns;
} nlen;

nlen.sig = name_length(encoded, abuf, alen);
nlen.sig = name_length(encoded, abuf, alen, is_hostname);
if (nlen.sig < 0)
return ARES_EBADNAME;

Expand Down Expand Up @@ -135,9 +165,8 @@ int ares_expand_name(const unsigned char *encoded, const unsigned char *abuf,
{
/* Output as \DDD for consistency with RFC1035 5.1, except
* for the special case of a root name response */
if (!isprint(*p) && !(name_len == 1 && *p == 0))
if (!ares__isprint(*p) && !(name_len == 1 && *p == 0))
{

*q++ = '\\';
*q++ = '0' + *p / 100;
*q++ = '0' + (*p % 100) / 10;
Expand Down Expand Up @@ -170,11 +199,18 @@ int ares_expand_name(const unsigned char *encoded, const unsigned char *abuf,
return ARES_SUCCESS;
}


int ares_expand_name(const unsigned char *encoded, const unsigned char *abuf,
int alen, char **s, long *enclen)
{
return ares__expand_name_validated(encoded, abuf, alen, s, enclen, 0);
}

/* Return the length of the expansion of an encoded domain name, or
* -1 if the encoding is invalid.
*/
static int name_length(const unsigned char *encoded, const unsigned char *abuf,
int alen)
int alen, int is_hostname)
{
int n = 0, offset, indir = 0, top;

Expand Down Expand Up @@ -212,16 +248,22 @@ static int name_length(const unsigned char *encoded, const unsigned char *abuf,

while (offset--)
{
if (!isprint(*encoded) && !(name_len == 1 && *encoded == 0))
if (!ares__isprint(*encoded) && !(name_len == 1 && *encoded == 0))
{
if (is_hostname)
return -1;
n += 4;
}
else if (is_reservedch(*encoded))
{
if (is_hostname)
return -1;
n += 2;
}
else
{
if (is_hostname && !is_hostnamech(*encoded))
return -1;
n += 1;
}
encoded++;
Expand All @@ -244,12 +286,14 @@ static int name_length(const unsigned char *encoded, const unsigned char *abuf,
return (n) ? n - 1 : n;
}

/* Like ares_expand_name but returns EBADRESP in case of invalid input. */
/* Like ares_expand_name_validated but returns EBADRESP in case of invalid
* input. */
int ares__expand_name_for_response(const unsigned char *encoded,
const unsigned char *abuf, int alen,
char **s, long *enclen)
char **s, long *enclen, int is_hostname)
{
int status = ares_expand_name(encoded, abuf, alen, s, enclen);
int status = ares__expand_name_validated(encoded, abuf, alen, s, enclen,
is_hostname);
if (status == ARES_EBADNAME)
status = ARES_EBADRESP;
return status;
Expand Down
6 changes: 3 additions & 3 deletions src/lib/ares_parse_ns_reply.c
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ int ares_parse_ns_reply( const unsigned char* abuf, int alen,

/* Expand the name from the question, and skip past the question. */
aptr = abuf + HFIXEDSZ;
status = ares__expand_name_for_response( aptr, abuf, alen, &hostname, &len);
status = ares__expand_name_for_response( aptr, abuf, alen, &hostname, &len, 0);
if ( status != ARES_SUCCESS )
return status;
if ( aptr + len + QFIXEDSZ > abuf + alen )
Expand All @@ -85,7 +85,7 @@ int ares_parse_ns_reply( const unsigned char* abuf, int alen,
for ( i = 0; i < ( int ) ancount; i++ )
{
/* Decode the RR up to the data field. */
status = ares__expand_name_for_response( aptr, abuf, alen, &rr_name, &len );
status = ares__expand_name_for_response( aptr, abuf, alen, &rr_name, &len, 0);
if ( status != ARES_SUCCESS )
break;
aptr += len;
Expand All @@ -110,7 +110,7 @@ int ares_parse_ns_reply( const unsigned char* abuf, int alen,
{
/* Decode the RR data and add it to the nameservers list */
status = ares__expand_name_for_response( aptr, abuf, alen, &rr_data,
&len);
&len, 1);
if ( status != ARES_SUCCESS )
{
ares_free(rr_name);
Expand Down
8 changes: 4 additions & 4 deletions src/lib/ares_parse_ptr_reply.c
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ int ares_parse_ptr_reply(const unsigned char *abuf, int alen, const void *addr,

/* Expand the name from the question, and skip past the question. */
aptr = abuf + HFIXEDSZ;
status = ares__expand_name_for_response(aptr, abuf, alen, &ptrname, &len);
status = ares__expand_name_for_response(aptr, abuf, alen, &ptrname, &len, 0);
if (status != ARES_SUCCESS)
return status;
if (aptr + len + QFIXEDSZ > abuf + alen)
Expand All @@ -84,7 +84,7 @@ int ares_parse_ptr_reply(const unsigned char *abuf, int alen, const void *addr,
for (i = 0; i < (int)ancount; i++)
{
/* Decode the RR up to the data field. */
status = ares__expand_name_for_response(aptr, abuf, alen, &rr_name, &len);
status = ares__expand_name_for_response(aptr, abuf, alen, &rr_name, &len, 0);
if (status != ARES_SUCCESS)
break;
aptr += len;
Expand All @@ -110,7 +110,7 @@ int ares_parse_ptr_reply(const unsigned char *abuf, int alen, const void *addr,
{
/* Decode the RR data and set hostname to it. */
status = ares__expand_name_for_response(aptr, abuf, alen, &rr_data,
&len);
&len, 1);
if (status != ARES_SUCCESS)
{
ares_free(rr_name);
Expand Down Expand Up @@ -146,7 +146,7 @@ int ares_parse_ptr_reply(const unsigned char *abuf, int alen, const void *addr,
{
/* Decode the RR data and replace ptrname with it. */
status = ares__expand_name_for_response(aptr, abuf, alen, &rr_data,
&len);
&len, 1);
if (status != ARES_SUCCESS)
{
ares_free(rr_name);
Expand Down
8 changes: 4 additions & 4 deletions src/lib/ares_parse_soa_reply.c
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ ares_parse_soa_reply(const unsigned char *abuf, int alen,
aptr = abuf + HFIXEDSZ;

/* query name */
status = ares__expand_name_for_response(aptr, abuf, alen, &qname, &len);
status = ares__expand_name_for_response(aptr, abuf, alen, &qname, &len, 0);
if (status != ARES_SUCCESS)
goto failed_stat;

Expand All @@ -83,7 +83,7 @@ ares_parse_soa_reply(const unsigned char *abuf, int alen,
for (i = 0; i < ancount; i++)
{
rr_name = NULL;
status = ares__expand_name_for_response (aptr, abuf, alen, &rr_name, &len);
status = ares__expand_name_for_response (aptr, abuf, alen, &rr_name, &len, 0);
if (status != ARES_SUCCESS)
{
ares_free(rr_name);
Expand Down Expand Up @@ -120,7 +120,7 @@ ares_parse_soa_reply(const unsigned char *abuf, int alen,

/* nsname */
status = ares__expand_name_for_response(aptr, abuf, alen, &soa->nsname,
&len);
&len, 0);
if (status != ARES_SUCCESS)
{
ares_free(rr_name);
Expand All @@ -130,7 +130,7 @@ ares_parse_soa_reply(const unsigned char *abuf, int alen,

/* hostmaster */
status = ares__expand_name_for_response(aptr, abuf, alen,
&soa->hostmaster, &len);
&soa->hostmaster, &len, 0);
if (status != ARES_SUCCESS)
{
ares_free(rr_name);
Expand Down
6 changes: 5 additions & 1 deletion src/lib/ares_private.h
Original file line number Diff line number Diff line change
Expand Up @@ -356,9 +356,13 @@ int ares__read_line(FILE *fp, char **buf, size_t *bufsize);
void ares__free_query(struct query *query);
unsigned short ares__generate_new_id(rc4_key* key);
struct timeval ares__tvnow(void);
int ares__expand_name_validated(const unsigned char *encoded,
const unsigned char *abuf,
int alen, char **s, long *enclen,
int is_hostname);
int ares__expand_name_for_response(const unsigned char *encoded,
const unsigned char *abuf, int alen,
char **s, long *enclen);
char **s, long *enclen, int is_hostname);
void ares__init_servers_state(ares_channel channel);
void ares__destroy_servers_state(ares_channel channel);
int ares__parse_qtype_reply(const unsigned char* abuf, int alen, int* qtype);
Expand Down
3 changes: 1 addition & 2 deletions src/lib/ares_process.c
Original file line number Diff line number Diff line change
Expand Up @@ -605,8 +605,7 @@ static void process_answer(ares_channel channel, unsigned char *abuf,
packetsz = PACKETSZ;
/* If we use EDNS and server answers with FORMERR without an OPT RR, the protocol
* extension is not understood by the responder. We must retry the query
* without EDNS enabled.
*/
* without EDNS enabled. */
if (channel->flags & ARES_FLAG_EDNS)
{
packetsz = channel->ednspsz;
Expand Down
3 changes: 3 additions & 0 deletions test/ares-test-parse.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ TEST_F(LibraryTest, ParseIndirectRootName) {
ares_free_hostent(host);
}


#if 0 /* We are validating hostnames now, its not clear how this would ever be valid */
TEST_F(LibraryTest, ParseEscapedName) {
std::vector<byte> data = {
0x12, 0x34, // qid
Expand Down Expand Up @@ -105,6 +107,7 @@ TEST_F(LibraryTest, ParseEscapedName) {
EXPECT_EQ('c', hent.name_[6]);
ares_free_hostent(host);
}
#endif

TEST_F(LibraryTest, ParsePartialCompressedName) {
std::vector<byte> data = {
Expand Down

0 comments on commit c9b6c60

Please sign in to comment.