openssl icon indicating copy to clipboard operation
openssl copied to clipboard

Constness broken uselessly in API SSL_CTX_set1_sigalgs_list, and code not aligned with documentation

Open Pascal-Fremaux opened this issue 2 years ago • 1 comments

In C++ analysis, we got a constness broken when using that API with a const string.

Documentation describes SSL_CTX_set1_sigalgs_list as this: https://github.com/openssl/openssl/blob/1c0eede9827b0962f1d752fa4ab5d436fa039da4/doc/man3/SSL_CTX_set1_sigalgs.pod

long SSL_CTX_set1_sigalgs_list(SSL_CTX *ctx, const char *str);

But in real life, this is a macro that call a generic inner void* method https://github.com/openssl/openssl/blob/fdc5043d58900663b493147298e64f11353b35fe/include/openssl/ssl.h.in

# define SSL_CTX_set1_sigalgs_list(ctx, s) \
   SSL_CTX_ctrl(ctx,SSL_CTRL_SET_SIGALGS_LIST,0,(char *)(s))

This cast is here to (char * ), not to (const char* ) as documented. This cast is the cause of the constness broken.

But, if you go deeper, you can see that we come to this implementation method: https://github.com/openssl/openssl/blob/ee05588dabeac7b9d034bf16dad122a93d1688a4/ssl/ssl_lib.c

        case SSL_CTRL_SET_CLIENT_SIGALGS_LIST:
            return tls1_set_sigalgs_list(NULL, parg, 0);

defined by https://github.com/openssl/openssl/blob/dc45bfb4b452ba5a876ebf48791217b69d092ff9/ssl/ssl_local.h

int tls1_set_sigalgs_list(CERT *c, const char *str, int client)

Which use a const cahr*; not a char*.

So the macro cast is wrong, and should be aligned with the API documentation and the impl. low level method.

# define SSL_CTX_set1_sigalgs_list(ctx, s) \
   SSL_CTX_ctrl(ctx,SSL_CTRL_SET_SIGALGS_LIST,0,(const char *)(s))

Which solve the problem of breaking the constness.

Pascal-Fremaux avatar Nov 03 '22 10:11 Pascal-Fremaux

The last argument of SSL_CTX_ctrl() is void *. We cannot cast the argument to const char * since that would IMO at least with some compilers produce a warning that the const is lost when passing this to SSL_CTX_ctrl().

t8m avatar Nov 03 '22 11:11 t8m

Closing since this is marked as resolved.

mattcaswell avatar Jan 31 '24 11:01 mattcaswell