Re: trunk r11017: Avoid a lot of bufer overruns in ext_edirectory_userip_acl

On 11/08/2010 10:01 PM, Amos Jeffries wrote:
> On 09/11/10 10:44, Alex Rousskov wrote:
>> On 11/05/2010 06:47 AM, Amos Jeffries wrote:
>>> ------------------------------------------------------------
>>> revno: 11017
>>> committer: Amos Jeffries<squid3_at_treenet.co.nz>
>>> branch nick: trunk
>>
>>> - xstrncpy(prog, EDUI_PROGRAM_NAME, sizeof(prog));
>>> + xstrncpy(prog, EDUI_PROGRAM_NAME, strlen(EDUI_PROGRAM_NAME));
>>
>>
>>> - xstrncpy(prog, edui_conf.program, sizeof(prog));
>>> + xstrncpy(prog, edui_conf.program, strlen(edui_conf.program));
>>
>>> - xstrncpy(cbuf, prog, sizeof(cbuf));
>>> + xstrncpy(cbuf, prog, strlen(prog));
>>
>> ...
>>
>> These and probably other changes in this commit look like bugs to me:
>> The third argument to xstrncpy() should be the destination buffer size,
>> not the source string length. The string length is usually irrelevant
>> because xstrncpy() will not copy past the 0-terminator of the source
>> string.
>
> Aw hell. Thought I got rid of those all again. The patch was for
> strncat() and bcmp() bugs.
>
> Reverting.
>
>>
>> BTW, the following code (which was not changed by this commit), appears
>> to be buggy or weird and should be changed to use cbuf buffer size, not
>> cbuf string length:
>>
>>> memset(cbuf, '\0', strlen(cbuf));
>
> Ouch. Thanks.

FWIW, there are more similar memset() calls in that source file as far
as I can see:

> ./helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc: memset(bufa, '\0', strlen(bufa));
> ./helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc: memset(bufa, '\0', strlen(bufa));
> ./helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc: memset(bufb, '\0', strlen(bufb));
> ./helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc: memset(l->search_filter, '\0', strlen(l->search_filter));
> ./helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc: memset(l->userid, '\0', strlen(l->userid));
> ./helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc: memset(bufc, '\0', strlen(bufc));
> ./helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc: memset(bufc, '\0', strlen(bufc));
> ./helpers/external_acl/eDirectory_userip/ext_edirectory_userip_acl.cc: memset(bufa, '\0', strlen(bufa));

I would not be surprised if these can be rewritten as
        *buf = '\0';

> ./helpers/external_acl/kerberos_ldap_group/kerberos_ldap_group.cc: memset(optarg, 'X', strlen(optarg));

This last one may not be a bug but should be double checked since it
looks weird.

Cheers,

Alex.
Received on Tue Nov 09 2010 - 16:53:15 MST