Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6827 closed defect (fixed)

UB in rend_parse_v2_service_descriptor()

Reported by: asn Owned by: rransom
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

    version = (int) tor_parse_long(smartlist_get(versions, i),
                                   10, 0, INT_MAX, &num_ok, NULL);
    if (!num_ok) /* It's a string; let's ignore it. */
      continue;
    result->protocols |= 1 << version;

This is UB if 'version' is bigger than the size of integer of the platform.

Child Tickets

Change History (7)

comment:1 Changed 7 years ago by nickm

Seems harmless at first glance; if version is greater than the width of protocols, then we'll just ignore the value. But AFAIK, we should also never actually use the value if it describes a protocol we can't speak, so our failure to record it shouldn't matter.

So the desired behavior (ignore protocols we can't speak) would seem identical to the current behavior.

This deserves a comment, though. And somebody should doublecheck my logic.

comment:2 in reply to:  1 Changed 7 years ago by rransom

Replying to nickm:

Seems harmless at first glance; if version is greater than the width of protocols, then we'll just ignore the value. But AFAIK, we should also never actually use the value if it describes a protocol we can't speak, so our failure to record it shouldn't matter.

So the desired behavior (ignore protocols we can't speak) would seem identical to the current behavior.

The current behaviour on at least one computer (with 8-bit bytes) that I have had is result->protocols |= (1 << (version % (sizeof(result->protocols) * 8)));.

Shifting left by more bits at once than an integer has really does produce undefined behaviour (‘UB’).

comment:3 Changed 7 years ago by rransom

Owner: set to rransom
Status: newassigned

comment:4 Changed 7 years ago by rransom

Status: assignedneeds_review

See my bug6827-v2 branch for a fix.

comment:5 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged; thank you!

comment:6 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:7 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.