Skip to content

Commit

Permalink
Don't return a valid hash in questions_hash() to signify an error
Browse files Browse the repository at this point in the history
Use a proper return code and store the hash in a given pointer instead.
In addition, if the packet is not a valid DNS query, I don't think
ignoring the hash is the right thing to do.
Either something is broken or we're under attack, or someone is fuzzing
the proxy. Either way, normal applications have no reason to try
sending non-DNS queries to the proxy, so we'd better print an error and
drop the packet.
  • Loading branch information
jedisct1 committed Jan 5, 2015
1 parent 13bb35f commit 915e06c
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 11 deletions.
11 changes: 5 additions & 6 deletions rfc1035.c
Original file line number Diff line number Diff line change
Expand Up @@ -176,24 +176,23 @@ extract_name(struct dns_header *header, size_t plen, unsigned char **pp, char
than hash the raw bytes, since replies might be compressed differently.
We ignore case in the names for the same reason. Return all-ones
if there is not question section. */
uint64_t
questions_hash(struct dns_header *header, size_t plen, char *name, const unsigned char key[crypto_shorthash_KEYBYTES])
int
questions_hash(uint64_t *hash, struct dns_header *header, size_t plen, char *name, const unsigned char key[crypto_shorthash_KEYBYTES])
{
unsigned char qb[MAXDNAME + 4];
uint64_t hash = 0xffffffffffffffffULL;
unsigned char *p = (unsigned char *)(header+1);
size_t name_len;

if (ntohs(header->qdcount) != 1 ||
!extract_name(header, plen, &p, name, 1, 4) ||
(name_len = strlen(name)) > (sizeof qb - 4)) {
return hash;
return -1;
}
memcpy(qb, name, name_len);
memcpy(qb + name_len, p, 4);
crypto_shorthash((unsigned char *) &hash, qb, name_len + 4ULL, key);
crypto_shorthash((unsigned char *) hash, qb, name_len + 4ULL, key);

return hash;
return 0;
}

static unsigned char *skip_name(unsigned char *ansp, struct dns_header *header, size_t plen, int extrabytes)
Expand Down
2 changes: 1 addition & 1 deletion rfc1035.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#include "dns-protocol.h"
#include <sodium.h>

uint64_t questions_hash(struct dns_header *header, size_t plen, char *buff, const unsigned char key[crypto_shorthash_KEYBYTES]);
int questions_hash(uint64_t *hash, struct dns_header *header, size_t plen, char *buff, const unsigned char key[crypto_shorthash_KEYBYTES]);
int extract_name(struct dns_header *header, size_t plen, unsigned char **pp, char
*name, int isExtract, int extrabytes);
int add_resource_record(struct dns_header *header, unsigned int nameoffset, unsigned char **pp,
Expand Down
14 changes: 10 additions & 4 deletions udp_request.c
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,10 @@ client_to_proxy_cb(evutil_socket_t client_proxy_handle, short ev_flags,
}

udp_request->id = ntohs(header->id);
udp_request->hash = questions_hash(header, dns_query_len, c->namebuff, c->hash_key);
if (questions_hash(&udp_request->hash, header, dns_query_len, c->namebuff, c->hash_key) != 0) {
logger(LOG_WARNING, "Received a suspicious query from the client");
udp_request_kill(udp_request);
}

udp_request->timeout_timer =
evtimer_new(udp_request->context->event_loop, timeout_timer_cb,
Expand All @@ -374,15 +377,14 @@ client_to_proxy_cb(evutil_socket_t client_proxy_handle, short ev_flags,

/*
* Find corresponding request by DNS id and hash of questions.
* Don't check hash if not know (0xffffffffffffffff).
*/
static UDPRequest *
lookup_request(struct context *c, uint16_t id, uint64_t hash)
{
UDPRequest *scanned_udp_request;
TAILQ_FOREACH(scanned_udp_request, &c->udp_request_queue, queue) {
if (id == scanned_udp_request->id
&& (scanned_udp_request->hash == hash || hash == 0xffffffffffffffffULL)) {
&& scanned_udp_request->hash == hash) {
return scanned_udp_request;
}
}
Expand Down Expand Up @@ -425,7 +427,11 @@ resolver_to_proxy_cb(evutil_socket_t proxy_resolver_handle, short ev_flags,

struct dns_header *header = (struct dns_header *)dns_reply;
uint16_t id = ntohs(header->id);
uint64_t hash = questions_hash(header, dns_reply_len, c->namebuff, c->hash_key);
uint64_t hash;
if (questions_hash(&hash, header, dns_reply_len, c->namebuff, c->hash_key) != 0) {
logger(LOG_ERR, "Received an invalid response from the server");
return;
}
udp_request = lookup_request(c, id, hash);
if (udp_request == NULL) {
logger(LOG_ERR, "Received a reply that doesn't match any active query");
Expand Down

0 comments on commit 915e06c

Please sign in to comment.