[squid-dev] [PATCH] ext_edirectory_userip_acl refactoring

[Alex Rousskov]

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.

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.

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().


> +    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.


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


HTH,

Alex.