Opened 11 months ago

Last modified 9 months ago

#32673 merge_ready defect

'buf_read_from_tls()' can return the wrong error code

Reported by: opara Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.3.0.4-rc
Severity: Normal Keywords: tor-tls, tor-security, consider-backport-if-needed, consider-backport-after-0433, 035-backport, 040-backport, 041-backport, 042-backport fast-fix
Cc: Actual Points: .1
Parent ID: Points: 1
Reviewer: ahf Sponsor:

Description

The function buf_read_from_tls(...) returns an integer. This integer can either be <=0 (in which case it corresponds to a TOR_TLS_ status) or a positive number (in which case it corresponds to the number of bytes read). This return value is used in connection_buf_read_from_socket() in a large switch(result) statement.

At the beginning of buf_read_from_tls(...), it returns -1 on the lines:

IF_BUG_ONCE(buf->datalen >= INT_MAX)
  return -1;
IF_BUG_ONCE(buf->datalen >= INT_MAX - at_most)
  return -1;

This value of -1 is the same as TOR_TLS_WANTWRITE. This causes the switch statement in connection_buf_read_from_socket() to interpret the return value as TOR_TLS_WANTWRITE, which is not correct for the buf->datalen >= INT_MAX bug. I suggest returning TOR_TLS_ERROR_MISC instead of -1. Note that this would close the connection.

I don't think you'll see incorrect behavior due to this, but it might be a good idea to fix.

Child Tickets

Change History (12)

comment:1 Changed 11 months ago by teor

Keywords: tor-tls tor-security added
Milestone: Tor: 0.4.3.x-final
Owner: set to nickm
Points: 1
Status: newassigned

I think Nick is the best person to triage this bug, and decide if it should block our planned releases in the next 2 weeks.

comment:2 Changed 11 months ago by opara

For reference, this code was added in commit ee5471f9 in response to #21369.

comment:3 Changed 11 months ago by teor

Keywords: 035-backport 040-backport 041-backport 042-backport added
Version: Tor: 0.3.0.4-rc

comment:4 Changed 10 months ago by nickm

Keywords: fast-fix 043-should added

comment:5 Changed 9 months ago by nickm

Actual Points: .1
Status: assignedneeds_review

Branch is bug32673_035 with PR at https://github.com/torproject/tor/pull/1695 . Should merge cleanly to master.

comment:6 Changed 9 months ago by teor

Keywords: consider-backport-after-0433 added

comment:7 Changed 9 months ago by ahf

Reviewer: ahf
Status: needs_reviewmerge_ready

Looks good.

comment:8 Changed 9 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.2.x-final

Merged to master; marking for possible backport. Note that I'm not so sure about a backport here: any behavioral changes that this patch causes will be subtle indeed, and perhaps best not backported unless we are certain they are beneficial.

comment:9 Changed 9 months ago by teor

Keywords: consider-backport-if-needed added

comment:10 in reply to:  8 ; Changed 9 months ago by arma

Replying to nickm:

marking for possible backport. Note that I'm not so sure about a backport here: any behavioral changes that this patch causes will be subtle indeed, and perhaps best not backported unless we are certain they are beneficial.

"We don't know of any actual behavior this will fix" plus "we're not sure if the patch introduces surprises" sound together like a great rationale for not backporting. Or for leaving the ticket in limbo (open but not backported) until it becomes clearer that we should backport it or close it.

comment:11 in reply to:  10 Changed 9 months ago by teor

Replying to arma:

Replying to nickm:

marking for possible backport. Note that I'm not so sure about a backport here: any behavioral changes that this patch causes will be subtle indeed, and perhaps best not backported unless we are certain they are beneficial.

"We don't know of any actual behavior this will fix" plus "we're not sure if the patch introduces surprises" sound together like a great rationale for not backporting. Or for leaving the ticket in limbo (open but not backported) until it becomes clearer that we should backport it or close it.

This ticket is tagged with consider-backport-if-needed.

I use the tag consider-backport-if-needed for tickets which we're not sure if we want to backport:
https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/Backports#WaitingtoseeifaBackportisNeeded

I won't backport those tickets, and no-one else should, either:
https://trac.torproject.org/projects/tor/wiki/user/teor#Backports

(If someone thinks there are good reasons for backporting a consider-backport-if-needed, they should double-check with me and Nick.)

comment:12 Changed 9 months ago by teor

Keywords: 043-should removed
Note: See TracTickets for help on using tickets.