Hi,

su 17. marrask. 2019 klo 20.13 Arne Schwabe (a...@rfc2549.org) kirjoitti:
>
> -        if (!cipher_kt_get(translate_cipher_name_from_openvpn(token)))
> +        /* translate_cipher_name_from_openvpn also normalises the cipher
name,
> +         * e.g. replacing AeS-128-gCm with AES-128-GCM
> +         */

I think this comment is a bit misleading -
 translate_cipher_name_from_openvpn()
translates openvpn cipher name (value in --ncp-ciphers) to crypto library
cipher name,
for example from "aES-256-GCM" to "idaes_256_GCM" which contradicts the
comment.

Maybe we could factor this code out into separate normalize_cipher_name()
function, which

 - translates (possibly non-normalized) openvpn cipher name into cryptolib
cipher name
 - translates crypto cipher name back to openvpn cipher name, this time
normalized

> +        if (!ktc)
>          {
>              msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s",
token);
> -            unsupported_cipher_found = true;
> +            error_found = true;

It seems that mutate_ncp_cipher_list() returns NULL if error_found is true.
Maybe we could goto
out of the loop? The label could be added before free() calls.

> +            if (buf_len(&new_list)> 0)
> +            {
> +                /* The next if condition ensure there is always space for
> +                 * a :
> +                 */
> +                buf_puts(&new_list, ":");
> +            }
> +
> +            /* Ensure buffer has capacity for cipher name + : + \0 */
> +            if (!(buf_forward_capacity(&new_list) >
> +                  strlen(ovpn_cipher_name) + 2))

This doesn't handle the case when buffer capacity is just enough
to fit the last cipher - for that it is enough to fit cipher name and \0.
Could we move

  >          token = strtok(NULL, ":");

here and do something like

  /* for the last cipher, token is NULL, enough to fit cipher and \0 */
  strlen(ovpn_cipher_name) + (token ? 2 : 1);

> +            {
> +                msg(M_WARN, "Length of --ncp-ciphers is over the"
> +                    "limit of 127 chars");
> +                error_found = true;

Same as above, cannot we "goto out"?

We could put out: right before next line.

>      free(tmp_ciphers);
> +    free_buf(&new_list);
>
> -    return 0 < strlen(list) && !unsupported_cipher_found;
> +    return ret;
>  }

-Lev
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Reply via email to