Opened 9 years ago

Closed 8 years ago

Last modified 6 years ago

#2332 closed defect (fixed)

dnsserv_resolved has needless casts, unused strndup.

Reported by: rransom Owned by: nickm
Priority: High Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: easy tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

doors found some more buried treasure:

dnsserv_resolved casts its argument const char *answer to char * before passing it to evdns_server_request_add_a_reply or evdns_server_request_add_ptr_reply. In both cases, it is then passed as a const char * argument to evdns_server_request_add_reply, so the casts probably aren't dangerous, but they're ugly and should be removed.

dnsserv_resolved also allocates a copy ans of answer before the call to evdns_server_request_add_ptr_reply, and frees it immediately afterward without using it. This clearly needs to be removed too.

Child Tickets

Change History (16)

comment:1 Changed 9 years ago by nickm

Milestone: Tor: 0.2.2.x-final

comment:2 Changed 9 years ago by nickm

Milestone: Tor: 0.2.2.x-finalTor: 0.2.1.x-final
Priority: minormajor
Status: newneeds_review

Fix for the trivial parts of this in branch bug2332: that one should go into 0.2.2 and later.

There's a more serious problem fixed in brach bug2332_part2; that should go into 0.2.1 and later.

comment:3 Changed 9 years ago by nickm

Summary: dnsserv_resolved is ickydnsserv_resolved has needless casts, unused strndup.

comment:4 Changed 9 years ago by cypherpunks

eventdns.c then rejects any name of length over 255, so the
bogus data would be neither transmitted nor altered.

So there memleak, memleak. Any memleak missed? Either it's still there.
Most obvious, if name of length over 255:

  evdns_server_request_respond(req, err);

  conn->dns_server_request = NULL;

comment:5 Changed 9 years ago by cypherpunks

Heh

Most obvious

is non obvious. No memleak.

comment:6 Changed 9 years ago by cypherpunks

There are old known broken evdns_server_request_format_response(), so no matter.

overflow:
		j = 512;
		buf[2] |= 0x02; /* set the truncated bit. */
	}

	req->response_len = (size_t)j;

	if (!(req->response = mm_malloc(req->response_len))) {
		server_request_free_answers(req);
		dnslabel_clear(&table);
		return (-1);
	}
	memcpy(req->response, buf, req->response_len);

Ok it's a truncated bit. What has buf around first 512 bytes, just curious?

comment:7 Changed 9 years ago by cypherpunks

Why

answer_len < 256

missed for answer_type == RESOLVED_TYPE_HOSTNAME.
Is it reserved ext? Tor going to implement non exist rfc for DNS stuff?

comment:8 Changed 9 years ago by arma

bug2332_part2 looks fine.

It also looks harmless, since the evdns_* functions don't modify the string passed in? Or are there libevent versions that do and sometimes we call their functions instead?

comment:9 Changed 9 years ago by arma

Ah. It doesn't modify, but the problem is that the thing being passed in is not nul terminated.

In that case, bug2332_part2 looks fine.

comment:10 Changed 9 years ago by nickm

Owner: set to nickm
Status: needs_reviewassigned

ok, merged bug2332_part2 to 0.2.1 and later. will leave this open for the remaining 2 issues.

comment:11 Changed 9 years ago by nickm

Ok it's a truncated bit. What has buf around first 512 bytes, just curious?

I don't understand the question.

Why
answer_len < 256
missed for answer_type == RESOLVED_TYPE_HOSTNAME.

In dnsserv_resolv? I've just added a check there.

comment:12 Changed 8 years ago by nickm

Merging the bug2332 branch. Now to re-read all of cypherpunks's comments and see if I can figure them out.

comment:13 Changed 8 years ago by nickm

Resolution: fixed
Status: assignedclosed

Okay, on review I don't see any issues above that aren't fixed now. Please comment if I missed something, and explain what I missed? Closing this ticket.

comment:14 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:15 Changed 7 years ago by nickm

Component: Tor RelayTor

comment:16 Changed 6 years ago by nickm

Milestone: Tor: 0.2.1.x-final

Milestone Tor: 0.2.1.x-final deleted

Note: See TracTickets for help on using tickets.