Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6861 closed defect (fixed)

Undefined behavior in rend_parse_service_authorization()

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

Description

  char descriptor_cookie_tmp[REND_DESC_COOKIE_LEN+2];
...
    if (base64_decode(descriptor_cookie_tmp, sizeof(descriptor_cookie_tmp),
                      descriptor_cookie_base64ext,
                      strlen(descriptor_cookie_base64ext)) < 0) {
      log_warn(LD_CONFIG, "Decoding authorization cookie failed: '%s'",
               descriptor_cookie);
      goto err;
    }
    auth_type_val = (descriptor_cookie_tmp[16] >> 4) + 1;

descriptor_cookie_tmp is a char array and chars are signed. The right shift there can cause undefined behavior if descriptor_cookie_tmp[16] is a negative value.

Reported on IRC.

Child Tickets

Change History (8)

comment:1 Changed 7 years ago by nickm_mobile

Hm. I could have sworn that rightshifting signed integers in C was implementation-defined, not undefined. I also thought we maybe had a check for it, in di_ops.c. I'll check both when online on a real computer again.

Still, it's still wrong in this case. Implementations are allowed to signextend, and that's wrong here.

comment:2 Changed 7 years ago by asn

Inded, it seems to be implementation-defined behavior.

C89/99 Standard Section 6.5.7 Paragraph 5 says:

The result of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type
or if E1 has a signed type and a nonnegative value, the value of the result is the integral 
part of the quotient of E1 / 2^{E2} . If E1 has a signed type and a negative value, the 
resulting value is implementation-defined.

comment:3 Changed 7 years ago by nickm

Status: newneeds_review

Please review bug6861 in my public repository. It's a one-line fix with a long commit message.

comment:4 in reply to:  3 Changed 7 years ago by rransom

Replying to nickm:

Please review bug6861 in my public repository. It's a one-line fix with a long commit message.

Looks good.

comment:5 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Fixed a typo in the commit message (spotted by asn) and merged; thanks!

comment:6 Changed 7 years ago by arma

Draw a card: your changes file said bug 6841.

comment:7 Changed 7 years ago by nickm

Keywords: tor-client added

comment:8 Changed 7 years ago by nickm

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