coverity warnings (Y2K38_SAFETY and OOB access)

From: Sergey Bronnikov via curl-library <curl-library_at_lists.haxx.se>
Date: Mon, 25 Sep 2023 14:28:58 +0300

Hello, everyone!


we are using libcurl as a part of application server in Tarantool
in-memory database [1].

After bumping Git submodule with curl repository to the latest version
(8.3.0)

Coverity reported a couple of warnings found in libcurl source code.

As I got it right according to curl issues and pull requests Curl
development team regularly fixes

problems found by Coverity. So flaws found in 8.3.0 could be unprocessed
for unknown for me reasons or

marked by Curl development team as false positive.

Below is my own analysis of warnings found by Coverity.


1. 1568154 Use of 32-bit time_t
The time value stored in this integer will represent a different, but
possibly valid, time.
In Curl_hostcache_prune: A 64-bit time_t value is stored in a smaller
width integer. (CWE-197)


Relevant part of source code in ./lib/hostip.c:257:

int timeout = data->set.dns_cache_timeout;

<snipped>

do {
   <snipped>

   if(oldest < INT_MAX)

       timeout = (int)oldest; /* we know it fits */

^^^^^^^^^^

CID 1568154 (#1 of 1): Use of 32-bit time_t (Y2K38_SAFETY)
1. store_truncates_time_t: A time_t value is stored in an integer with
too few bits to accommodate it.The expression oldest is cast to int.

   else
       timeout = INT_MAX - 1;

Last time condition above was touched by Daniel and comment was added in
commit [2].

I suspect this warning is false positive and could be ignored, right?


2. 1568144 Out-of-bounds access
Access of memory not owned by this buffer may cause crashes or incorrect
computations.
In Curl_sock_assign_addr: Out-of-bounds access to a buffer (CWE-119)


Relevant part of source code, ./lib/cf-socket.c:250:

<snipped>

   dest->addrlen = ai->ai_addrlen;

   if(dest->addrlen > sizeof(struct Curl_sockaddr_storage))
     dest->addrlen = sizeof(struct Curl_sockaddr_storage);
   memcpy(&dest->sa_addr, ai->ai_addr, dest->addrlen);

^^^^^^^^

CID 1568144 (#1 of 1): Out-of-bounds access (OVERRUN)
5. overrun-buffer-arg: Overrunning struct type sockaddr of 16 bytes by
passing
it to a function which accesses it at byte offset 127 using argument
dest->addrlen (which evaluates to 128).

<snipped>

First argument of memcpy is a pointer to a memory region  with struct
sockaddr, see Curl_sockaddr_ex.

Second argument of memcpy is a pointer to a memory region with struct
sockaddr too, see Curl_addrinfo.

Size of struct sockaddr is 16 bytes.

More interesting is third argument "dest->addrlen".

ai->ai_addrlen is assigned to dest->addrlen, where ai_addrlen is "int".
But then dest->addrlen is changed:


     if(dest->addrlen > sizeof(struct Curl_sockaddr_storage))
       dest->addrlen = sizeof(struct Curl_sockaddr_storage);


And size of Curl_sockaddr_storage could be much bigger than size of
integer, see lib/sockaddr.h:


   struct Curl_sockaddr_storage {
     union {
       struct sockaddr sa;
       struct sockaddr_in sa_in;
   #ifdef ENABLE_IPV6
       struct sockaddr_in6 sa_in6;
#endif
   #ifdef HAVE_STRUCT_SOCKADDR_STORAGE
       struct sockaddr_storage sa_stor;
#else
       char cbuf[256];   /* this should be big enough to fit a lot */
#endif
     } buffer;
   };


I suspect in some circumstances out-of-bounds access could happen

(size of struct sockaddr_storage is 128 bytes when macros
HAVE_STRUCT_SOCKADDR_STORAGE is enabled,

and size of cbuf is 256 bytes when macros HAVE_STRUCT_SOCKADDR_STORAGE
is  disabled),

and therefore this Coverity warning is valid, right?


1. https://github.com/tarantool/tarantool

2.
https://github.com/curl/curl/commit/8e6abece40#diff-e0b647f3e21a7fb58400dcec74f381d2fe16ca41e983240c15fc1243c574dbf9R275-R278

Sergey

-- 
Unsubscribe: https://lists.haxx.se/mailman/listinfo/curl-library
Etiquette:   https://curl.se/mail/etiquette.html

Received on 2023-09-25