Re: [Openvpn-devel] [PATCH v3] platform: Retain CAP_NET_ADMIN when dropping privileges

[Gert Doering <gert@...1296...>]

[-- Attachment #1: Type: text/plain, Size: 2596 bytes --]

Hi,

On Thu, Apr 07, 2022 at 08:40:24PM +0200, Timo Rothenpieler wrote:
> +    else if (res < 0)
> +    {
> +        if (res == -3)
> +        {
> +            msg(M_NONFATAL, "Following error likely due to missing capability CAP_SETPCAP.");
> +        }
> +        msg(err_flags | M_ERRNO, "capng_change_id('%s','%s') failed retaining capabilities: %d",
> +            user_state->username, group_state->groupname, res);
> +        goto fallback;
> +    }

Wouldn't that overwrite errno for the "res == -3" case, given that
msg() will do stdio stuff?  Maybe reorder and print the "error likely due..."
message with a preceding "NOTE:" after the capng_change_id() message?

(That would be more typical for our logs - the "NOTE: this could be
because..." tends to come after the error message)

> +    if (new_uid >= 0)
> +    {
> +         msg(M_INFO, "UID set to %s", user_state->username);
> +    }
> +    if (new_gid >= 0)
> +    {
> +         msg(M_INFO, "GID set to %s", group_state->groupname);
> +    }
> +
> +    msg(M_INFO, "Capabilities retained: CAP_NET_ADMIN");
> +
> +    return;
> +fallback:

My inner whitespace dragon does would prefer to have the blank line
between "return" and "fallback:" (and no blank linke after the M_INFO).

> +    /* capng_change_id() can leave this flag clobbered on failure
> +     * This is working around a bug in libcap-ng, which can leave the flag set
> +     * on failure: https://github.com/stevegrubb/libcap-ng/issues/33 */
> +    if (prctl(PR_GET_KEEPCAPS) && prctl(PR_SET_KEEPCAPS, 0) < 0)
> +    {
> +        msg(M_ERR, "Clearing KEEPCAPS flag failed");
> +    }
> +#endif  /* HAVE_LIBCAPNG */

This one does not really look like it should be in "fallback:" - because
that way it always gets called, even if we jump there right at function
entry, if keep_caps == 0.

> +
> +    if (keep_caps)
> +    {
> +        msg(err_flags, "Unable to retain capabilities");
> +    }
> +
> +    platform_group_set(group_state);
> +    platform_user_set(user_state);
> +}
> +

Maybe "fallback:" should be right before platform_group_set()?


(Sorry for being late to the "complain about your code" party...)

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]