[squid-dev] [PATCH] ext_edirectory_userip_acl refactoring

[Amos Jeffries]

Opened PR 204 with the below changes...

On 10/05/18 02:42, Alex Rousskov wrote:
> On 05/09/2018 05:05 AM, Amos Jeffries wrote:
>> Proposed changes to this helper to fix strcat / strncat buffer overread
>> / overflow issues.
> 
> I have no objections overall.
> 
> * I am not excited about duplicating Ip::Address pieces, but such
> duplication cannot be prohibited while we do not allow Ip::Address into
> helpers.
> 
> * I am not excited about using so much error-prone low-level code
> instead of safer/modern approaches, but that is not enough to block
> these fixes, especially since they are confined to a helper.
> 
> 
> I cannot validate low-level code changes, but I did spot a few potential
> problems:
> 
> 
>> +        if (strlen(dn) > sizeof(l->dn))
>> +            return LDAP_ERR_OOB; /* DN too large */
> 
> If l->dn will be 0-terminated, then the condition should be ">=".
> 
> 
>> +    memset(&want, 0, sizeof(struct addrinfo));
> 
> Using sizeof(want) is safer in such contexts.

done.

> 
> makeHexString() is very dangerous because it does not really check for
> bufb overflows despite (misleadingly) implying otherwise. It should be
> given two size-related parameters (e.g., lena and sizeb) so that it can
> be implemented safely.

Added the missing checks.

> 
> makeHexString() assumes that bufb buffer is 0-terminated. It should at
> least document that fact. It should also document that it _appends_ to
> bufb (rather than naturally copying the answer into the provided
> buffer). I suggest renaming this function into appendAsHex().
> 

Changed the internals instead. This was not supposed to be an append
action and no callers use it that way. So it now begins by setting the
first char of bufb to null and works from there.

> 
>> +    struct addrinfo *dst = nullptr;
>> +    if (makeIpBinary(dst, ip)) {
> ...
>>      }
>>  
>> +    freeaddrinfo(dst);
> 
> makeIpBinary() API is not documented, but, AFAICT, freeing should be
> moved inside the above if-statement. You may also want to check whether
> dst is nil there. Given the current usage, I would change makeIpBinary()
> to take a single parameter and return a pointer instead.
> 

Done as suggested.

> 
> Many variables can (and, hence, should) be "const". Some should be
> eliminated completely (e.g., "err").
> 

Um, I'm not seeing any that are being changed by this patch and not
already const. err is now gone.

Amos