New Defects reported by Coverity Scan for Osmocom

[Neels Hofmeyr]

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

On Wed, Mar 08, 2017 at 08:39:07AM -0800, scan-admin at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.osmocom.org/pipermail/openbsc/attachments/20170308/94bb8dd3/attachment.bin>