Opened 5 months ago

Closed 5 months ago

#26522 closed defect (fixed)

strncat() without bounds

Reported by: Dhiraj Owned by: rl1987
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Major Keywords: security-low, 035-proposed
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

Hi Team,

https://github.com/torproject/tor/blob/master/src/lib/err/backtrace.c#L267-L268

i.e

strncat(version, " ", sizeof(version)-1);
strncat(version, tor_version, sizeof(version)-1);

Easily used incorrectly (e.g., incorrectly computing the correct maximum size to add) such as (CWE-120).

Consider strcat_s, strlcat, snprintf, or automatically resizing strings. I feel the risk is high because the length parameter appears to be a constant, instead of computing the number of characters left.

Request team to please have a look and validate.

Regards
Dhiraj

Child Tickets

Change History (9)

comment:1 Changed 5 months ago by teor

Keywords: security-low 035-proposed added
Milestone: Tor: unspecified
Version: Tor: unspecified

Marking as "security-low" in 0.3.5, because our use of strncat() is a defence in depth mechanism that doesn't provide as much security as it should:
https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/SecurityPolicy

We should review all uses of strncat() to make sure we always pass the *remaining*string length.
(And all uses of strlcat() to make sure we check the return value.)

comment:2 Changed 5 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:3 Changed 5 months ago by rl1987

Status: acceptedneeds_review

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

Not sure if it's a good thing to introduce dependency between lib/err and lib/string. Perhaps the reason strncat() was used was to avoid including stuff from lib/string?

comment:4 in reply to:  3 Changed 5 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final

Replying to rl1987:

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

Not sure if it's a good thing to introduce dependency between lib/err and lib/string. Perhaps the reason strncat() was used was to avoid including stuff from lib/string?

That's unlikely, because strncat() was used long before we refactored code into lib/string.

comment:5 Changed 5 months ago by nickm

I don't think strncat() was used in backtrace.c before we moved stuff around though. And yeah -- I'd rather not have a dependency from lib/err to lib/string, since that makes the library graph circular again.

comment:6 Changed 5 months ago by rl1987

strncat() was introduced pretty recently in a969ce464dc23db39725a891d60537f3d3e51b50.

I updated my patch to use snprintf() from libc.

comment:7 Changed 5 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

lgtm;

Note: The changes file is not needed if we fix something that was never released except for new features.

comment:8 Changed 5 months ago by nickm

Thanks, that's better! Squashed and merged to master.

comment:9 Changed 5 months ago by rl1987

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