Skip to content

Commit

Permalink
Have sock_initaddress() return the list of addrinfo structures or NULL.
Browse files Browse the repository at this point in the history 
Its return address is currently 0 for success and -1 for failure, with a
pointer to the first element of the list of struct addrinfos returned
through a pointer on success; change it to return that pointer on
success and NULL on failure.

That way, we don't have to worry about what happens to the pointer
pointeed to by the argument in question on failure; we know that we got
NULL back if no struct addrinfos were found because getaddrinfo()
failed.  Thus, we know that we have something to free iff
sock_initaddress() returned a pointer to that something rather than
returning NULL.

This avoids a double-free in some cases.

This is apparently CVE-2023-40400.

(backported from commit 262e4f3)
  • Loading branch information
@guyharris
guyharris committed Oct 9, 2023
1 parent 7c23acc commit 2aa69b0
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 55 deletions.
48 changes: 25 additions & 23 deletions pcap-rpcap.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1024,17 +1024,16 @@ rpcap_remoteact_getsock(const char *host, int *error, char *errbuf)
{
struct activehosts *temp; /* temp var needed to scan the host list chain */
struct addrinfo hints, *addrinfo, *ai_next; /* temp var needed to translate between hostname to its address */
int retval;

/* retrieve the network address corresponding to 'host' */
addrinfo = NULL;
memset(&hints, 0, sizeof(struct addrinfo));
hints.ai_family = PF_UNSPEC;
hints.ai_socktype = SOCK_STREAM;

retval = sock_initaddress(host, NULL, &hints, &addrinfo, errbuf,
addrinfo = sock_initaddress(host, NULL, &hints, errbuf,
PCAP_ERRBUF_SIZE);
if (retval != 0)
if (addrinfo == NULL)
{
*error = 1;
return NULL;
Expand Down Expand Up @@ -1186,7 +1185,9 @@ static int pcap_startcapture_remote(pcap_t *fp)
hints.ai_flags = AI_PASSIVE; /* Data connection is opened by the server toward the client */

/* Let's the server pick up a free network port for us */
if (sock_initaddress(NULL, NULL, &hints, &addrinfo, fp->errbuf, PCAP_ERRBUF_SIZE) == -1)
addrinfo = sock_initaddress(NULL, NULL, &hints, fp->errbuf,
PCAP_ERRBUF_SIZE);
if (addrinfo == NULL)
goto error_nodiscard;

if ((sockdata = sock_open(NULL, addrinfo, SOCKOPEN_SERVER,
Expand Down Expand Up @@ -1311,7 +1312,9 @@ static int pcap_startcapture_remote(pcap_t *fp)
snprintf(portstring, PCAP_BUF_SIZE, "%d", ntohs(startcapreply.portdata));

/* Let's the server pick up a free network port for us */
if (sock_initaddress(host, portstring, &hints, &addrinfo, fp->errbuf, PCAP_ERRBUF_SIZE) == -1)
addrinfo = sock_initaddress(host, portstring, &hints,
fp->errbuf, PCAP_ERRBUF_SIZE);
if (addrinfo == NULL)
goto error;

if ((sockdata = sock_open(host, addrinfo, SOCKOPEN_CLIENT, 0, fp->errbuf, PCAP_ERRBUF_SIZE)) == INVALID_SOCKET)
Expand Down Expand Up @@ -2340,16 +2343,16 @@ rpcap_setup_session(const char *source, struct pcap_rmtauth *auth,
if (port[0] == 0)
{
/* the user chose not to specify the port */
if (sock_initaddress(host, RPCAP_DEFAULT_NETPORT,
&hints, &addrinfo, errbuf, PCAP_ERRBUF_SIZE) == -1)
return -1;
addrinfo = sock_initaddress(host, RPCAP_DEFAULT_NETPORT,
&hints, errbuf, PCAP_ERRBUF_SIZE);
}
else
{
if (sock_initaddress(host, port, &hints, &addrinfo,
errbuf, PCAP_ERRBUF_SIZE) == -1)
return -1;
addrinfo = sock_initaddress(host, port, &hints,
errbuf, PCAP_ERRBUF_SIZE);
}
if (addrinfo == NULL)
return -1;

if ((*sockctrlp = sock_open(host, addrinfo, SOCKOPEN_CLIENT, 0,
errbuf, PCAP_ERRBUF_SIZE)) == INVALID_SOCKET)
Expand Down Expand Up @@ -2950,19 +2953,19 @@ SOCKET pcap_remoteact_accept_ex(const char *address, const char *port, const cha
/* Do the work */
if ((port == NULL) || (port[0] == 0))
{
if (sock_initaddress(address, RPCAP_DEFAULT_NETPORT_ACTIVE, &hints, &addrinfo, errbuf, PCAP_ERRBUF_SIZE) == -1)
{
return (SOCKET)-2;
}
addrinfo = sock_initaddress(address,
RPCAP_DEFAULT_NETPORT_ACTIVE, &hints, errbuf,
PCAP_ERRBUF_SIZE);
}
else
{
if (sock_initaddress(address, port, &hints, &addrinfo, errbuf, PCAP_ERRBUF_SIZE) == -1)
{
return (SOCKET)-2;
}
addrinfo = sock_initaddress(address, port, &hints, errbuf,
PCAP_ERRBUF_SIZE);
}
if (addrinfo == NULL)
{
return (SOCKET)-2;
}


if ((sockmain = sock_open(NULL, addrinfo, SOCKOPEN_SERVER, 1, errbuf, PCAP_ERRBUF_SIZE)) == INVALID_SOCKET)
{
Expand Down Expand Up @@ -3122,7 +3125,6 @@ int pcap_remoteact_close(const char *host, char *errbuf)
{
struct activehosts *temp, *prev; /* temp var needed to scan the host list chain */
struct addrinfo hints, *addrinfo, *ai_next; /* temp var needed to translate between hostname to its address */
int retval;

temp = activeHosts;
prev = NULL;
Expand All @@ -3133,9 +3135,9 @@ int pcap_remoteact_close(const char *host, char *errbuf)
hints.ai_family = PF_UNSPEC;
hints.ai_socktype = SOCK_STREAM;

retval = sock_initaddress(host, NULL, &hints, &addrinfo, errbuf,
addrinfo = sock_initaddress(host, NULL, &hints, errbuf,
PCAP_ERRBUF_SIZE);
if (retval != 0)
if (addrinfo == NULL)
{
return -1;
}
Expand Down
8 changes: 6 additions & 2 deletions rpcapd/daemon.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2085,7 +2085,9 @@ daemon_msg_startcap_req(uint8 ver, struct daemon_slpars *pars, uint32 plen,
goto error;
}

if (sock_initaddress(peerhost, portdata, &hints, &addrinfo, errmsgbuf, PCAP_ERRBUF_SIZE) == -1)
addrinfo = sock_initaddress(peerhost, portdata, &hints,
errmsgbuf, PCAP_ERRBUF_SIZE);
if (addrinfo == NULL)
goto error;

if ((session->sockdata = sock_open(peerhost, addrinfo, SOCKOPEN_CLIENT, 0, errmsgbuf, PCAP_ERRBUF_SIZE)) == INVALID_SOCKET)
Expand All @@ -2096,7 +2098,9 @@ daemon_msg_startcap_req(uint8 ver, struct daemon_slpars *pars, uint32 plen,
hints.ai_flags = AI_PASSIVE;

// Make the server socket pick up a free network port for us
if (sock_initaddress(NULL, NULL, &hints, &addrinfo, errmsgbuf, PCAP_ERRBUF_SIZE) == -1)
addrinfo = sock_initaddress(NULL, NULL, &hints, errmsgbuf,
PCAP_ERRBUF_SIZE);
if (addrinfo == NULL)
goto error;

if ((session->sockdata = sock_open(NULL, addrinfo, SOCKOPEN_SERVER, 1 /* max 1 connection in queue */, errmsgbuf, PCAP_ERRBUF_SIZE)) == INVALID_SOCKET)
Expand Down
8 changes: 6 additions & 2 deletions rpcapd/rpcapd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,9 @@ void main_startup(void)
//
// Get a list of sockets on which to listen.
//
if (sock_initaddress((address[0]) ? address : NULL, port, &mainhints, &addrinfo, errbuf, PCAP_ERRBUF_SIZE) == -1)
addrinfo = sock_initaddress((address[0]) ? address : NULL,
port, &mainhints, errbuf, PCAP_ERRBUF_SIZE);
if (addrinfo == NULL)
{
rpcapd_log(LOGPRIO_DEBUG, "%s", errbuf);
return;
Expand Down Expand Up @@ -1350,7 +1352,9 @@ main_active(void *ptr)
memset(errbuf, 0, sizeof(errbuf));

// Do the work
if (sock_initaddress(activepars->address, activepars->port, &hints, &addrinfo, errbuf, PCAP_ERRBUF_SIZE) == -1)
addrinfo = sock_initaddress(activepars->address, activepars->port,
&hints, errbuf, PCAP_ERRBUF_SIZE);
if (addrinfo == NULL)
{
rpcapd_log(LOGPRIO_DEBUG, "%s", errbuf);
return 0;
Expand Down
58 changes: 33 additions & 25 deletions sockutils.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1069,20 +1069,21 @@ get_gai_errstring(char *errbuf, int errbuflen, const char *prefix, int err,
* \param errbuflen: length of the buffer that will contains the error. The error message cannot be
* larger than 'errbuflen - 1' because the last char is reserved for the string terminator.
*
* \return '0' if everything is fine, '-1' if some errors occurred. The error message is returned
* in the 'errbuf' variable. The addrinfo variable that has to be used in the following sockets calls is
* returned into the addrinfo parameter.
* \return a pointer to the first element in a list of addrinfo structures
* if everything is fine, NULL if some errors occurred. The error message
* is returned in the 'errbuf' variable.
*
* \warning The 'addrinfo' variable has to be deleted by the programmer by calling freeaddrinfo() when
* it is no longer needed.
* \warning The list of addrinfo structures returned has to be deleted by
* the programmer by calling freeaddrinfo() when it is no longer needed.
*
* \warning This function requires the 'hints' variable as parameter. The semantic of this variable is the same
* of the one of the corresponding variable used into the standard getaddrinfo() socket function. We suggest
* the programmer to look at that function in order to set the 'hints' variable appropriately.
*/
int sock_initaddress(const char *host, const char *port,
struct addrinfo *hints, struct addrinfo **addrinfo, char *errbuf, int errbuflen)
struct addrinfo *sock_initaddress(const char *host, const char *port,
struct addrinfo *hints, char *errbuf, int errbuflen)
{
struct addrinfo *addrinfo;
int retval;

/*
Expand All @@ -1094,9 +1095,13 @@ int sock_initaddress(const char *host, const char *port,
* as those messages won't talk about a problem with the port if
* no port was specified.
*/
retval = getaddrinfo(host, port == NULL ? "0" : port, hints, addrinfo);
retval = getaddrinfo(host, port == NULL ? "0" : port, hints, &addrinfo);
if (retval != 0)
{
/*
* That call failed.
* Determine whether the problem is that the host is bad.
*/
if (errbuf)
{
if (host != NULL && port != NULL) {
Expand All @@ -1108,7 +1113,7 @@ int sock_initaddress(const char *host, const char *port,
int try_retval;

try_retval = getaddrinfo(host, NULL, hints,
addrinfo);
&addrinfo);
if (try_retval == 0) {
/*
* Worked with just the host,
Expand All @@ -1117,28 +1122,31 @@ int sock_initaddress(const char *host, const char *port,
*
* Free up the address info first.
*/
freeaddrinfo(*addrinfo);
freeaddrinfo(addrinfo);
get_gai_errstring(errbuf, errbuflen,
"", retval, NULL, port);
} else {
/*
* Didn't work with just the host,
* so assume the problem is
* with the host.
* with the host; we assume
* the original error indicates
* the underlying problem.
*/
get_gai_errstring(errbuf, errbuflen,
"", retval, host, NULL);
}
} else {
/*
* Either the host or port was null, so
* there's nothing to determine.
* there's nothing to determine; report
* the error from the original call.
*/
get_gai_errstring(errbuf, errbuflen, "",
retval, host, port);
}
}
return -1;
return NULL;
}
/*
* \warning SOCKET: I should check all the accept() in order to bind to all addresses in case
Expand All @@ -1153,30 +1161,28 @@ int sock_initaddress(const char *host, const char *port,
* ignore all addresses that are neither? (What, no IPX
* support? :-))
*/
if (((*addrinfo)->ai_family != PF_INET) &&
((*addrinfo)->ai_family != PF_INET6))
if ((addrinfo->ai_family != PF_INET) &&
(addrinfo->ai_family != PF_INET6))
{
if (errbuf)
snprintf(errbuf, errbuflen, "getaddrinfo(): socket type not supported");
freeaddrinfo(*addrinfo);
*addrinfo = NULL;
return -1;
freeaddrinfo(addrinfo);
return NULL;
}

/*
* You can't do multicast (or broadcast) TCP.
*/
if (((*addrinfo)->ai_socktype == SOCK_STREAM) &&
(sock_ismcastaddr((*addrinfo)->ai_addr) == 0))
if ((addrinfo->ai_socktype == SOCK_STREAM) &&
(sock_ismcastaddr(addrinfo->ai_addr) == 0))
{
if (errbuf)
snprintf(errbuf, errbuflen, "getaddrinfo(): multicast addresses are not valid when using TCP streams");
freeaddrinfo(*addrinfo);
*addrinfo = NULL;
return -1;
freeaddrinfo(addrinfo);
return NULL;
}

return 0;
return addrinfo;
}

/*
Expand Down Expand Up @@ -2089,7 +2095,9 @@ int sock_present2network(const char *address, struct sockaddr_storage *sockaddr,

hints.ai_family = addr_family;

if (sock_initaddress(address, "22222" /* fake port */, &hints, &addrinfo, errbuf, errbuflen) == -1)
addrinfo = sock_initaddress(address, "22222" /* fake port */, &hints,
errbuf, errbuflen);
if (addrinfo == NULL)
return 0;

if (addrinfo->ai_family == PF_INET)
Expand Down
5 changes: 2 additions & 3 deletions sockutils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,8 @@ void sock_fmterrmsg(char *errbuf, size_t errbuflen, int errcode,
PCAP_FORMAT_STRING(const char *fmt), ...) PCAP_PRINTFLIKE(4, 5);
void sock_geterrmsg(char *errbuf, size_t errbuflen,
PCAP_FORMAT_STRING(const char *fmt), ...) PCAP_PRINTFLIKE(3, 4);
int sock_initaddress(const char *address, const char *port,
struct addrinfo *hints, struct addrinfo **addrinfo,
char *errbuf, int errbuflen);
struct addrinfo *sock_initaddress(const char *address, const char *port,
struct addrinfo *hints, char *errbuf, int errbuflen);
int sock_recv(SOCKET sock, SSL *, void *buffer, size_t size, int receiveall,
char *errbuf, int errbuflen);
int sock_recv_dgram(SOCKET sock, SSL *, void *buffer, size_t size,
Expand Down

0 comments on commit 2aa69b0

Please sign in to comment.