Opened 8 weeks ago

Last modified 24 hours ago

#33788 needs_review defect

Check the return value of tor_inet_ntop() and tor_inet_ntoa()

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: extra-review, 044-should, security-low, 035-backport, 041-backport, 042-backport, 043-backport, outreachy-ipv6, ipv6, 044-must
Cc: MrSquanchee Actual Points:
Parent ID: #33768 Points: 1
Reviewer: nickm Sponsor: Sponsor55-must

Description (last modified by teor)

The following functions don't check the return value of tor_inet_ntop() or tor_inet_ntoa():

IPv6, could be serious:

  • evdns_callback(), multiple times

IPv4 only, unlikely to be a serious bug:

  • tor_dup_ip()
  • fmt_addr32()
  • evdns_wildcard_check_callback()

These functions should log a bug log using BUG() or tor_assert_nonfatal(), and return an error. (Or for the formatting functions, a sensible placeholder string.)

We will also need to make their callers check for the error.

Child Tickets

Change History (7)

comment:1 Changed 8 weeks ago by teor

Description: modified (diff)
Summary: Check the return value of tor_inet_ntop()Check the return value of tor_inet_ntop() and tor_inet_ntoa()

Also fix tor_inet_ntoa().

comment:2 Changed 7 weeks ago by rl1987

Status: newneeds_review

comment:3 Changed 6 weeks ago by asn

Reviewer: teor

comment:4 Changed 12 days ago by teor

Keywords: extra-review added
Reviewer: teor
Status: needs_reviewneeds_revision

Sorry it's taken a while for me to do this review, things have been a bit chaotic recently.

I think this code looks good, but it's also a bit complicated. And I'm not sure if we have test coverage for all these changes.

There are also some conflicts with master.

So I'd like someone else to review, once you've resolved those conflicts.

comment:5 Changed 6 days ago by rl1987

Status: needs_revisionneeds_review

comment:6 Changed 6 days ago by nickm

Keywords: 044-must added

Add 044-must to all security tickets in 0.4.4

comment:7 Changed 24 hours ago by asn

Reviewer: nickm
Note: See TracTickets for help on using tickets.