Opened 4 weeks ago

Last modified 6 days ago

#30561 merge_ready defect

Fixed tor_vasprintf on systems without vasprintf.

Reported by: paldium Owned by:
Priority: Low Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version: Tor: 0.4.0.5
Severity: Minor Keywords: 029-backport?, 034-backport?, 035-backport?, 040-backport?, dgoulet-merge
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

If tor is compiled on a system with neither vasprintf nor _vscprintf,
the fallback implementation exposes a logic flaw which prevents
proper usage of strings longer than 127 characters:

  • tor_vsnprintf returns -1 if supplied buffer is not large enough, but tor_vasprintf uses this function to retrieve required length
  • the result of tor_vsnprintf is not properly checked for negative return values

Both aspects together could in theory lead to exposure of uninitialized
stack memory in the resulting string. This requires an invalid format
string or data that exceeds integer limitations.

Fortunately tor is not even able to run with this implementation because
it runs into asserts early on during startup. Also the unit tests fail
during a "make check" run.

At this point it would make sense to check if support for these systems is still a desired option. It seems that nobody noticed lack of support for at least a year.

Child Tickets

Attachments (1)

Fixed-tor_vasprintf-on-systems-without-vasprintf.patch (2.0 KB) - added by paldium 4 weeks ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 4 weeks ago by catalyst

Milestone: Tor: unspecified

comment:2 Changed 3 weeks ago by nickm

Status: newneeds_review

comment:3 Changed 3 weeks ago by nickm

Reviewer: nickm

comment:4 Changed 3 weeks ago by nickm

Keywords: 029-backport? 034-backport? 035-backport? 040-backport? added
Milestone: Tor: unspecifiedTor: 0.4.1.x-final

Paldium -- can you give an example of a system like this that I might be able to find and use for testing?

comment:5 Changed 3 weeks ago by nickm

Keywords: dgoulet-merge added
Status: needs_reviewmerge_ready

The patch looks okay to me. I've made some branches to fix the formatting, add a changes file, backport to 0.2.9, and add a note about removing this case from the code. I've confirmed that if I manually disable the first two implementations of tor_vasprintf(), the code now works and passes its test.

For 0.2.9 through 0.3.4: branch ticket30561_029 with PR at https://github.com/torproject/tor/pull/1052

For 0.3.5 and onward: branch ticket30561_035 with PR at https://github.com/torproject/tor/pull/1054

comment:6 Changed 3 weeks ago by dgoulet

Milestone: Tor: 0.4.1.x-finalTor: 0.4.0.x-final

The 035 branch (PR 1054) merged into master. Going to 040 milestone for backport.

comment:7 Changed 13 days ago by paldium

Hi nickm,

sorry for the late reply. I have encountered this code through manual review and I had no system around which lacks vasprintf. Therefore I have disabled the other two implementations manually as well.

The only system that is POSIX and lacks vasprintf is a closed-source Solaris version. Due to this lack of available test platforms I came up with the recommendation to drop this code completely.

Thanks for merging. :)

comment:8 Changed 6 days ago by nickm

Keywords: 029-backport? 034-backport? 035-backport? 040-backport? dgoulet-merge029-backport?, 034-backport?, 035-backport?, 040-backport?, dgoulet-merge

Removing 034-backport from all open tickets: 034 has reached EOL.

Note: See TracTickets for help on using tickets.