Re: New Defects reported by Coverity Scan for Osmocom - OpenBSC

8 Mar 2017

From libosmo-abis beb10ef02a10d73537a97f6f21aad36664c9b266
"add basic unixsocket support":

On Wed, Mar 08, 2017 at 08:39:07AM -0800, scan-admin(a)coverity.com wrote:
...
  *** CID 163920:    (TAINTED_SCALAR)
 /source-Osmocom/libosmo-abis/src/input/unixsocket.c: 90 in unixsocket_read_cb()
 84     	uint8_t controldata;
 85     	int ret;
 86     
 87     	if (!msg)
 88     		return -ENOMEM;
 89     
 >>     CID 163920:    (TAINTED_SCALAR)
>>     Calling function "read" taints argument "msg->data".
 90     	ret = read(bfd->fd, msg->data, UNIXSOCKET_ALLOC_SIZE - 16); 
What this seems to complain about is that we're writing arbitrary data to
msg->data, and in lapd_receive() pass this to a LOGP that prints a hexdump of
it. In osmo_hexdump, we of course do hex_chars[buf[i] >> 4] which we know to
always work out to one of '0'..'f' for *any* data. This point seems to be
missed by covertiy scan and it believes we may end up with an invalid index to
hex_chars[]. So this is a false positive. Marked it as such.

...
  *** CID 163919:  Security best practices violations 
(STRING_OVERFLOW)
 /source-Osmocom/libosmo-abis/src/input/unixsocket.c: 236 in unixsocket_line_update()
 230     	struct unixsocket_line *config;
 231     	char sock_path[PATH_MAX];
 232     	int ret = 0;
 233     	int i;
 234     
 235     	if (line->sock_path)
 >>     CID 163919:  Security best practices
violations  (STRING_OVERFLOW)
>>     Note: This defect has an elevated risk because the source argument is a
parameter of the current function.  236     		strcpy(sock_path, line->sock_path);

Please always use osmo_strlcpy(), here with sizeof(sock_path).

...
  237     	else
 238     		sprintf(sock_path, "%s%d", UNIXSOCKET_SOCK_PATH_DEFAULT,
 239     			line->num);
 240     
 241     	LOGP(DLINP, LOGL_NOTICE, "line update (line=%p)\n", line);

 ** CID 163918:  Memory - illegal accesses  (BUFFER_SIZE_WARNING)
 /source-Osmocom/openbsc/openbsc/src/libbsc/bsc_subscriber.c: 83 in bsc_subscr_set_imsi()

The next one is from me, in 6d804b1a7e375213cb4b3e437c2b9b8c68872164
"add struct bsc_subscr, separating libbsc from gsm_subscriber":

...

________________________________________________________________________________________________________
 *** CID 163918:  Memory - illegal accesses  (BUFFER_SIZE_WARNING)
 /source-Osmocom/openbsc/openbsc/src/libbsc/bsc_subscriber.c: 83 in bsc_subscr_set_imsi()
 77     }
 78     
 79     void bsc_subscr_set_imsi(struct bsc_subscr *bsub, const char *imsi)
 80     {
 81     	if (!bsub)
 82     		return;
 >>     CID 163918:  Memory - illegal
accesses  (BUFFER_SIZE_WARNING)
>>     Calling strncpy with a maximum size argument of 16 bytes on destination array
"bsub->imsi" of size 16 bytes might leave the destination string
unterminated. 
 83     	strncpy(bsub->imsi, imsi, sizeof(bsub->imsi)); 
Again, I should use osmo_strlcpy(), which correctly handles sizeof(buf), other
than strncpy which might leave the string unterminated.

~N