Opened 3 years ago

Closed 3 years ago

Last modified 20 months ago

#15202 closed defect (invalid)

Second argument to strlcpy must always be NUL-terminated.

Reported by: nickm Owned by:
Priority: Very High Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: 023-backport, 024-backport, 025-backport, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Even though strlcpy and strlcat stop copying their inputs when further bytes would fill up the output buffer, they keep reading the input string until they find a terminating NUL. This means that if you pass strlcpy or strlcat a non-NUL-terminated argument, they will keep reading off into the heap, and potentially crash.

We do this in at least one place.

Found while investigating #15083. This can be remotely triggerable on some systems, depending on the behavior of malloc(), and on whether buffer freelists are turned on, and on the phase of the moon.

Child Tickets

Change History (7)

comment:1 Changed 3 years ago by nickm

Status: newneeds_review

Branch "bug15202_023"; needs review!

comment:2 Changed 3 years ago by nickm

(to be clear, this is at worst a remote crash, not a remote overflow.)

comment:3 Changed 3 years ago by ioerror

I looked at https://gitweb.torproject.org/nickm/tor.git/diff/?h=bug15202_023 and it looks reasonable. As I said on irc, I'm surprised the defensive stuff at the top didn't consider this issue.

Is that the only problematic use of strlcpy/strlcat?

comment:4 Changed 3 years ago by nickm

I may have spoken too soon; looking at the code, it appears that this input _is_ nul-terminated. hmmmm. must investigate more.

comment:5 Changed 3 years ago by nickm

Resolution: invalid
Status: needs_reviewclosed

Arg, looks like this _was_ NUL-terminated. I think I found a different bug instead. Closing this; opening another.

comment:6 Changed 3 years ago by arma

(is this worth doing a workaround in Tor to better handle the osx bug, at least for a while?)

comment:7 Changed 20 months ago by nickm

Keywords: 2016-bug-retrospective added

Marking for bug retrospective based on Priority.

Note: See TracTickets for help on using tickets.