[squid-dev] [PATCH] ext_edirectory_userip_acl refactoring

Alex Rousskov rousskov at measurement-factory.com
Wed May 9 14:42:21 UTC 2018

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

* 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

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



More information about the squid-dev mailing list