Opened 3 months ago

Last modified 7 weeks ago

#33889 new defect

Functions with char* arguments are dangerous when used with casting

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: security, memory-safety, 044-can
Cc: Actual Points:
Parent ID: Points: 3
Reviewer: Sponsor:

Description (last modified by asn)

There are various functions in our codebase which receive char* arguments when they actually receive a byte array. That's I guess because of the old C standard when uint8_t didn't really exist, so we had to use char* for everything.

Still, these days we use uint8_t* and this means that we need to cast it to char* when we use those functions. This can cause security issues because this casting is silencing type-safety warnings (as an example see https://github.com/torproject/tor/pull/1843#discussion_r406191281 from #33545).

For example, see how fast_mem_is_zero() is used 75% of the time with casting its first argument. Instead of doing this, we could make a new fast_mem_is_zero_uint8t() function that takes uint8_t as argument. Or even make a fast_mem_is_zero_char() function that takes char*.

In other functions, it might be possible to replace the char* with void* but in the case of fast_mem_is_zero() that's not possible because of the implementation of the function.

Other potential problem functions: base64_encode() hex_str() crypto_rand() crypto_digest().

Child Tickets

Change History (4)

comment:1 Changed 3 months ago by asn

Description: modified (diff)

comment:2 Changed 3 months ago by nickm

I'm +1 on this general idea. I'm against using void* here though; we should be using uint8_t or unsigned char.

If we're going to have two functions, let's have e.g. fast_mem_is_zero() take the unsigned pointer, and define a fast_mem_is_zero_char() version for legacy usage.

comment:3 Changed 7 weeks ago by nickm

Keywords: 044-must added

Add 044-must to all security tickets in 0.4.4

comment:4 Changed 7 weeks ago by nickm

Keywords: 044-can added; 044-must removed
Note: See TracTickets for help on using tickets.