Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#5647 closed defect (fixed)

rend_parse_client_keys() prints stack in logs if base64_decode fails

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

Description

int
rend_parse_client_keys(strmap_t *parsed_clients, const char *ckstr)
{
...
    char descriptor_cookie_base64[REND_DESC_COOKIE_LEN_BASE64+2+1];
...
    /* The size of descriptor_cookie_tmp needs to be REND_DESC_COOKIE_LEN+2,
     * because a base64 encoding of length 24 does not fit into 16 bytes in all
     * cases. */
    if ((base64_decode(descriptor_cookie_tmp, REND_DESC_COOKIE_LEN+2,
                       tok->args[0], REND_DESC_COOKIE_LEN_BASE64+2+1)
           != REND_DESC_COOKIE_LEN)) {
      log_warn(LD_REND, "Descriptor cookie contains illegal characters: "
                        "%s", descriptor_cookie_base64);
      goto err;
    }
...

descriptor_cookie_base64 was never initialized, so it upon base64_decode() failure, it prints stack garbage to the logs.

Not an important bug, but a bug alright, so I'm putting it here to not forget it.

Child Tickets

Change History (11)

comment:1 Changed 8 years ago by arma

If I'm reading this right, descriptor_cookie_base64's only role in life is to be an uninitialized string that we print in that log message. Fun.

(I'm guessing we wanted to print tok->args[0] instead?)

comment:2 Changed 8 years ago by arma

Milestone: Tor: unspecifiedTor: 0.2.3.x-final

comment:3 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.2.x-final
Status: newneeds_review

(I'm guessing we wanted to print tok->args[0] instead?)

escaped(tok->args[0]).

See branch bug5647 against maint-0.2.2; please review.

comment:4 Changed 8 years ago by asn

Your fix looks good.

Kinda lame that gcc and coverity didn't detect this one as use of uninitialized variable. Maybe we should send them an email?

Also, should srclen in base64_decode be
REND_DESC_COOKIE_LEN_BASE64+2+1 or REND_DESC_COOKIE_LEN_BASE64+2 to match with:

 if (strlen(tok->args[0]) != REND_DESC_COOKIE_LEN_BASE64 + 2) {

?

It seems to me that if the base64 blob doesn't contain padding characters, base64_decode will also read the NUL (because of the +1) and fail. In other cases of base64_decode() we are feeding the output of strlen() as the srclen, which does not consider the NUL.

comment:5 in reply to:  4 Changed 8 years ago by arma

Replying to asn:

Kinda lame that gcc and coverity didn't detect this one as use of uninitialized variable. Maybe we should send them an email?

We're passing an array to a function, which then passes it to all sorts of other functions. I'm not too surprised that gcc/coverity couldn't convince themselves that we don't write to it.

Also, should srclen in base64_decode be
REND_DESC_COOKIE_LEN_BASE64+2+1 or REND_DESC_COOKIE_LEN_BASE64+2 to match with:

 if (strlen(tok->args[0]) != REND_DESC_COOKIE_LEN_BASE64 + 2) {

?

It seems to me that if the base64 blob doesn't contain padding characters, base64_decode will also read the NUL (because of the +1) and fail. In other cases of base64_decode() we are feeding the output of strlen() as the srclen, which does not consider the NUL.

No idea. I would hope our base64_decode is robust to a nul at the end of the source string.

comment:6 Changed 8 years ago by nickm

Passing that NUL is I believe harmless but needless. Merging the main patch; leaving this open to fix that issue in 0.2.3.

comment:7 Changed 8 years ago by nickm

Merged the main patch. Now see branch "bug5647_cleanup" for a fix to the issue asn noted above.

comment:8 in reply to:  6 Changed 8 years ago by asn

It seems to me that base64_decode() simply trusts srclen and doesn't assume that the input is a NUL-terminated string. I think that NUL will simply reach

    unsigned char c = (unsigned char) *src;
    uint8_t v = base64_decode_table[c];

return X and base64_decode() will fail. I could be wrong though.

In any case, bug5647_cleanup looks good to me.

comment:9 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Ok, merged!

comment:10 Changed 8 years ago by nickm

Keywords: tor-hs added

comment:11 Changed 8 years ago by nickm

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