On Mon, 25 Sep 2017, Denis Kenzior wrote:
Hi Mat,
>>> - strncpy(family->name, name, GENL_NAMSIZ); >>> +
>>> l_strlcpy(family->name, name, GENL_NAMSIZ);
>>
>> It would seem that l_genl_family_new should sanitize the name. There's no
>> point in truncating a name silently and trying to use it afterwards just
>> to fail spectacularly anyway.
>
> How about checking for truncation here in family_alloc using the return
> value of l_strlcpy? family_alloc could return NULL, and l_genl_family_new
> already knows what to do with that.
>
But then we're allocating a family object, memsetting it, etc unnecessarily,
no?
Yes, in the very rare case of a programmer error during development. Are
we that worried about the microsecond-level efficiency of a very rarely
executed error path?
If family_alloc uses strcpy, then you depend on reviewers giving
family_alloc the same level of scrutiny they do strcpy. You can't depend
on static analysis to catch it in the future after flagging it as "ok"
now.
If family_alloc uses l_strlcpy, then the common case gets an extra call to
strlen.
>>
>>> family->op_list = l_queue_new();
>>> family->mcast_list = l_queue_new();
>>> @@ -232,7 +232,7 @@ static void family_add_mcast(struct l_genl_family
>>> *family, const char *name,
>>> mcast = l_new(struct genl_mcast, 1);
>>> - strncpy(mcast->name, name, GENL_NAMSIZ);
>>> + l_strlcpy(mcast->name, name, GENL_NAMSIZ);
>>> mcast->id = id;
>>> mcast->users = 0;
>>> @@ -1047,7 +1047,7 @@ static void get_family_callback(struct l_genl_msg
>>> *msg, void *user_data)
>>> family->id = *((uint16_t *) data);
>>> break;
>>> case CTRL_ATTR_FAMILY_NAME:
>>> - strncpy(family->name, data, GENL_NAMSIZ);
>>> + l_strlcpy(family->name, data, GENL_NAMSIZ);
>>> break;
>>> case CTRL_ATTR_VERSION:
>>> family->version = *((uint32_t *) data);
>>>
>>
>> The other two are not called with user input but get their values from the
>> kernel, no?
>
> Mostly (one use of family_add_mcast uses a hard-coded string). The current
> use of strncpy is incorrect no matter where the name comes from.
>
why is it incorrect? We're getting the name from the kernel and copying it
into our own data structure. If the kernel uses GENL_NAMSIZ max-size
buffers, then using strncpy is just fine, no?
It's incorrect because you aren't guaranteed a null-terminated destination
buffer. It would be correct to use strncpy with GENL_NAMSIZ-1 and write a
'\0' to to the end of the buffer if it's not known to be zeroed.
At least I don't see how strlcpy is saving you here. If you want to be
paranoid, you'd be taking the tlv length value into account as well.
It's making the current code work as intended.
strlcpy guarantees two things: that you don't write past the end of your
destination buffer (corrupting other data) while copying, and that the
destination buffer is null-terminated after the copy. strncpy does the
former, but not the latter.
--
Mat Martineau
Intel OTC