Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#2352 closed defect (fixed)

more size_t_ceiling fun

Reported by: arma Owned by:
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

<doorss> need to replace "if (tok->object_size >= INT_MAX) {"
<doorss> with "if (tok->object_size >= SIZE_T_CEILING || tok->object_size >= INT_MAX) {"
<doorss> for two places.
<doorss> if it can be >= INT_MAX it can be INT_MAX - 1 too.
<doorss> "tok->object_body = ALLOC(next-*s); /* really, this is too much RAM. */" is fragile too. no checks, utill assert.

> doorss: which branch are you looking at?
<doorss> it was master.

<doorss> "if (tok->object_size >= INT_MAX)" mostly about cast to int. but if size can be such big (no assert here) so need to check for SIZE_T_CEILING.
<doorss> it prevents overflow underflow and another truncates during "sig->signature_len = (int) tok->object_size"
<doorss> SIZE_T_CEILING < INT_MAX for 32bit size_t
<doorss> if tok->object_size == INT_MAX - 1 then "sig->signature = tor_memdup(tok->object_body, tok->object_size)" trigger assert for 32 bit.
<doorss> we can't just replace INT_MAX with SIZE_T_CEILING. for 64 bit.

Child Tickets

Change History (19)

comment:1 Changed 9 years ago by nickm

Milestone: Tor: 0.2.2.x-final

comment:2 Changed 9 years ago by nickm

Status: newneeds_review

Check branch bug2352_obsize in my public repository. This fix not only adds more SIZE_T_CEILING checks but also clamps down on how large an object can get much earlier in the parsing process.

comment:3 Changed 9 years ago by cypherpunks

While changes for crypto_pk_read_private_key_from_string(): seems like BIO_new_mem_buf() returns NULL if memory out or something broken, and PEM_read_bio_RSAPrivateKey() does not like it.

comment:4 Changed 9 years ago by cypherpunks

Seems like Tor do not checks a pointers returned by openssl, like BIO_new(). And calling with it openssl's funcs. It's another bug of course.

comment:5 Changed 9 years ago by cypherpunks

Memory out is not a reson to segfault, btw. So better to check, and assert correctly inside Tor's malloc.

comment:6 Changed 9 years ago by cypherpunks

tor_assert(len < INT_MAX && len < SIZE_T_CEILING);

len is ssize_t (signed).
Hopes no one platform defines those constants as unsigned.
Except it does Tor, for missed defines:

#ifndef INT32_MAX
#define INT32_MAX 0x7fffffffu
#ifndef SSIZE_T_MAX
#if (SIZEOF_SIZE_T == 4)
#define SSIZE_T_MAX INT32_MAX

Fix:

tor_assert(len < (ssize_t)INT_MAX && len < (ssize_t)SIZE_T_CEILING);

comment:7 Changed 9 years ago by cypherpunks

Another thing:

(int)len

sszite_t truncates to small int for 64bit vs 32 bit.
Is it safe for non 2's complement 64bit platform (lets assume it exist)?

comment:8 in reply to:  3 Changed 9 years ago by cypherpunks

Replying to cypherpunks:

While changes for crypto_pk_read_private_key_from_string(): seems like BIO_new_mem_buf() returns NULL if memory out or something broken, and PEM_read_bio_RSAPrivateKey() does not like it.

Seems like Tor do not checks a pointers returned by openssl, like BIO_new(). And calling with it openssl's funcs. It's another bug of course.

Memory out is not a reson to segfault, btw. So better to check, and assert correctly inside Tor's malloc.

That was wrong statement, at least for PEM_read_bio_RSAPrivateKey(). Not so obvious but it can check and recover if it was NULL.

comment:9 in reply to:  4 Changed 9 years ago by nickm

Replying to cypherpunks:

Seems like Tor do not checks a pointers returned by openssl, like BIO_new(). And calling with it openssl's funcs. It's another bug of course.

Added as bug #2378.

comment:10 in reply to:  6 Changed 9 years ago by nickm

Replying to cypherpunks:

tor_assert(len < INT_MAX && len < SIZE_T_CEILING);

len is ssize_t (signed).
Hopes no one platform defines those constants as unsigned.
Except it does Tor, for missed defines:

#ifndef INT32_MAX
#define INT32_MAX 0x7fffffffu
#ifndef SSIZE_T_MAX
#if (SIZEOF_SIZE_T == 4)
#define SSIZE_T_MAX INT32_MAX

Fix:

tor_assert(len < (ssize_t)INT_MAX && len < (ssize_t)SIZE_T_CEILING);

Here I'm going to fix the definition of INT32_MAX: It should be signed. (fixed in commit 9fcc14224b689dff1be8336feeeb563199694c27.)

comment:11 in reply to:  7 ; Changed 9 years ago by nickm

Replying to cypherpunks:

Another thing:

(int)len

sszite_t truncates to small int for 64bit vs 32 bit.
Is it safe for non 2's complement 64bit platform (lets assume it exist)?

I'm not sure, but Tor requires two's complement arithmetic. We check for it in autoconf, and fail to build with an error in torint.h if arithmetic is not two's complement.

comment:12 in reply to:  11 Changed 9 years ago by cypherpunks

Replying to nickm:

Replying to cypherpunks:

Another thing:

(int)len

sszite_t truncates to small int for 64bit vs 32 bit.
Is it safe for non 2's complement 64bit platform (lets assume it exist)?

I'm not sure, but Tor requires two's complement arithmetic. We check for it in autoconf, and fail to build with an error in torint.h if arithmetic is not two's complement.

So while limit of positive numbers (object size) guranteed less than INT_MAX and negative the only -1, I guess it's a safe truncation.

comment:13 Changed 9 years ago by cypherpunks

another thing, ulimited length for object name:

tok->object_type = STRNDUP(*s+11, eol-*s-16);

comment:14 Changed 9 years ago by nickm

Milestone: Tor: 0.2.2.x-finalTor: 0.2.1.x-final

comment:15 Changed 9 years ago by nickm

uploaded a fix to the STRNDUP() issue.

comment:16 Changed 9 years ago by arma

"partsing"

other than that, nickm/bug2352_obsize (1f3b4420233) looks plausible to me.

comment:17 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

merged. forgot to fix the partsing though. :/

comment:18 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:19 Changed 7 years ago by nickm

Milestone: Tor: 0.2.1.x-final

Milestone Tor: 0.2.1.x-final deleted

Note: See TracTickets for help on using tickets.