Opened 7 weeks ago

Closed 6 weeks ago

#31545 closed enhancement (implemented)

CID 1452819: nul-terminated string handling, possibly spurious

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: 042-must, memory-safety?, easy, intro, ipv6, logging, fast-fix, coverity
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: asn Sponsor: Sponsor27-must

Description

Bug introduced by #21003, copying sponsors and tags.

/src/feature/nodelist/describe.c: 77 in format_node_description()
71       }
72       if (addr32h && has_addr) {
73         memcpy(cp, " and ", 5);
74         cp += 5;
75       }
76       if (has_addr) {
   CID 1452819:    (STRING_NULL)
   Passing unterminated string "cp" to "tor_addr_to_str", which expects a null-terminated string.
77         tor_addr_to_str(cp, addr, TOR_ADDR_BUF_LEN, 1);
78       }
79     
80       return buf;
81     }
82     
/src/feature/nodelist/describe.c: 70 in format_node_description()
64         cp += 4;
65       }
66       if (addr32h) {
67         struct in_addr in;
68         in.s_addr = htonl(addr32h);
69         tor_inet_ntoa(&in, cp, INET_NTOA_BUF_LEN);
   CID 1452819:    (STRING_NULL)
   Passing unterminated string "cp" to "strlen", which expects a null-terminated string.
70         cp += strlen(cp);
71       }
72       if (addr32h && has_addr) {
73         memcpy(cp, " and ", 5);
74         cp += 5;
75       }

I think the best fix for this issue is using strncpy() rather than memcpy().

Child Tickets

Change History (7)

comment:1 Changed 7 weeks ago by teor

Status: assignedneeds_review
Type: defectenhancement

I rewrote describe.c to use strlcpy() and strlcat(), and check their return values.

https://github.com/torproject/tor/pull/1270

comment:2 Changed 7 weeks ago by catalyst

Keywords: coverity added

comment:3 Changed 6 weeks ago by asn

Reviewer: dgoulet

comment:4 Changed 6 weeks ago by asn

Reviewer: dgouletasn

comment:5 Changed 6 weeks ago by asn

Status: needs_reviewmerge_ready

Looks well made to me!!!!

comment:6 Changed 6 weeks ago by nickm

Merging to master.

comment:7 Changed 6 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.