svn commit: r295188 - user/ngie/bsnmp_cleanup/usr.sbin/bsnmpd/tools/libbsnmptools

[Bruce Evans]

On Wed, 3 Feb 2016, Garrett Cooper wrote:

> Log:
>  Use destination buffer instead of source buffer size to mute valid
>  security concerns with strlcpy related to their respective buffer
>  sizes (-Wstrlcpy-strlcat-size)

Why not fix the bug instead of not hear the warning about it?  The
change might actually be a fix, but the log message makes it sound
like it isn't.

> ==============================================================================
> --- user/ngie/bsnmp_cleanup/usr.sbin/bsnmpd/tools/libbsnmptools/bsnmpmap.c	Wed Feb  3 01:58:37 2016	(r295187)
> +++ user/ngie/bsnmp_cleanup/usr.sbin/bsnmpd/tools/libbsnmptools/bsnmpmap.c	Wed Feb  3 02:00:20 2016	(r295188)
> @@ -283,7 +283,7 @@ enum_pair_insert(struct enum_pairs *head
> 	}
>
> 	e_new->enum_val = enum_val;
> -	strlcpy(e_new->enum_str, enum_str, strlen(enum_str) + 1);
> +	strlcpy(e_new->enum_str, enum_str, strlen(e_new->enum_str));
> 	STAILQ_INSERT_TAIL(headp, e_new, link);
>
> 	return (1);

This actually completely breaks the code.  It applies strlen() to garbage
memory that was just allocated to hold the string.

The old code was correct except being obfuscated using strlcpy().

> @@ -569,7 +569,7 @@ snmp_enumtc_init(char *name)
> 		free(enum_tc);
> 		return (NULL);
> 	}
> -	strlcpy(enum_tc->name, name, strlen(name) + 1);
> +	strlcpy(enum_tc->name, name, sizeof(enum_tc->name));
>
> 	return (enum_tc);
> }

This also completely breaks the code.  It uses sizeof on the pointer
to the memory that will become the new string when it is initialized.

The old code was correct except being obfuscated using strlcpy().

These bugs show why strlen() shouldn't exist.  Its use in the old code
was just an obfuscation.  It only looks wrong because its return value
is not checked, so it looks like truncation is not detected.  However,
since the buffers were just allocated with the correct size, truncation
is impossible and ordinary strcpy() works perfectly.

The unbroken version was essentially a home made version of strdup().
Using strdup() would be slightly easier.  But I don't like it either.
Using strdup() saves having to write a couple of strlen() expressions
and on strcpy() statement and prevents obfuscating the strcpy() as an
unchecked strlcpy().  But this is easy and I like it to be explicit.
The hard part is the error handling, especially in libraries.

Bruce