Welcome! Log In Create A New Profile

Advanced

BUG/MEDIUM ssl: Fix loading of dhaparams in multicert setups.

Posted by Francisco Blas Izquierdo Riera (klondike) 
Francisco Blas Izquierdo Riera (klondike)
BUG/MEDIUM ssl: Fix loading of dhaparams in multicert setups.
September 09, 2018 06:20PM
When using multicertificate bundles (i.e. .rsa, .ecdsa and .dsa files) HAProxy
fails to load certificates at random.

This is caused by an attempt to load the DH parameters from the NULL pointer
instead of the corresponding bundle which leaves an error in the queue.

This patch makes ssl_sock_load_mutli_cert use instead the correct bundle
identifier which in turn prevents the error (after the BIO tries to
open NULL in read only mode).

For any legal matters, please consider this contribution on the public domain.

Please backport to 1.8 and 1.7 it will apply correctly at least on 1.8.

--- src/ssl_sock.c
+++ src/ssl_sock.c
@@ -3131,11 +3131,11 @@ static int ssl_sock_load_multi_cert(const char *path, struct bind_conf *bind_con
if (ssl_dh_ptr_index >= 0)
SSL_CTX_set_ex_data(cur_ctx, ssl_dh_ptr_index, NULL);

- rv = ssl_sock_load_dh_params(cur_ctx, NULL);
+ rv = ssl_sock_load_dh_params(cur_ctx, cur_file);
if (rv < 0) {
if (err)
memprintf(err, "%sunable to load DH parameters from file '%s'.\n",
- *err ? *err : "", path);
+ *err ? *err : "", cur_file);
rv = 1;
goto end;
}
Hi Francisco,

On 09/10/2018 02:43 PM, klondike wrote:
> El 10/09/18 a las 11:25, Emeric Brun escribió:
>> Hi Fransisco,
> Hi Emeric!
>
> First of all thanks for taking the time to review my patch. It is my
> first time contributing (and it was also my first time using) HAProxy so
> It took me quite a few hours to pinpoint and "fix" the issue I was
> experiencing.
>> On 09/09/2018 06:10 PM, Francisco Blas Izquierdo Riera (klondike) wrote:
>>> When using multicertificate bundles (i.e. .rsa, .ecdsa and .dsa files) HAProxy
>>> fails to load certificates at random.
>>>
>>> This is caused by an attempt to load the DH parameters from the NULL pointer
>>> instead of the corresponding bundle which leaves an error in the queue.
>>>
>>> This patch makes ssl_sock_load_mutli_cert use instead the correct bundle
>>> identifier which in turn prevents the error (after the BIO tries to
>>> open NULL in read only mode).
>>>
>>> For any legal matters, please consider this contribution on the public domain.
>>>
>>> Please backport to 1.8 and 1.7 it will apply correctly at least on 1.8.
>>>
>>> --- src/ssl_sock.c
>>> +++ src/ssl_sock.c
>>> @@ -3131,11 +3131,11 @@ static int ssl_sock_load_multi_cert(const char *path, struct bind_conf *bind_con
>>> if (ssl_dh_ptr_index >= 0)
>>> SSL_CTX_set_ex_data(cur_ctx, ssl_dh_ptr_index, NULL);
>>>
>>> - rv = ssl_sock_load_dh_params(cur_ctx, NULL);
>>> + rv = ssl_sock_load_dh_params(cur_ctx, cur_file);
>>> if (rv < 0) {
>>> if (err)
>>> memprintf(err, "%sunable to load DH parameters from file '%s'.\n",
>>> - *err ? *err : "", path);
>>> + *err ? *err : "", cur_file);
>>> rv = 1;
>>> goto end;
>>> }
>>>
>> I agree there is a bug but I think we should qualify the wanted behaviour and confirm doing some check.
>>
>>
>> I think DH param is global per SSL_CTX and not per key type (please confirm, may I wrong?)
> Yes it is.
>> So what happens in this case:
>>
>> cert.pem.rsa firstly loaded and contain a DH param => ssl_sock_load_dh_params will load the param in to SSL_CTX.
>> in a second time cert.pem.ecdsa is loaded and does not contain any DH param. => What happens ? previous param is kept or ssl_sock_load_dh_params resets the DH param to default one?
> As it is now, SSL_CTX_set_tmp_dh is called only once because the call is
> done outside the loop and the value of cur_file points to the .rsa file.
> More concerning is the case of what happens if there is no .rsa file as
> it will provoke an error and we go back to the problematic behaviour.
>
> As to what happens when we call SSL_CTX_set_tmp_dh you can see openssl's
> code itself:
> https://github.com/openssl/openssl/blob/HEAD/ssl/s3_lib.c#L3411 it will
> always take the last passed item.
>
> Going over this code I have also noticed that we may end up with a
> memory leak as we will never free the DH parameters once we are done
> with them.
>
>> I don't know what would be the wanted behavior but the last described (which I suppose is performed) doesn't seem to be the right choice.
> I can propose the following optionss:
> 1. Always load the global parameters (this is the previous behaviour). I
> just need to avoid opening the file when the filename is NULL to fix the
> bug.
> 2. Stop at the last hit (i.e. test dsa, ecdsa and rsa in that order, if
> they are supported by the specific context, and choose the last dh
> params struct) if none is found, fall-back to global.
> 3. Use a .dhparams file instead fallback to global if not found.
>
> Personally I think number 1 is the easiest one to implement followed by
> number 2 which provides the most flexibility but is the hardest one to
> understand.
>> So in my opinion, the current patch just fix a piece of the problem.
> Indeed, the patch only prevents the spurious errors and not even always.
>
> PS: I noticed your answer was not sent to the list so I'm uncertain if
> that was intentional or not.

It was a mistake, we may need people opinion.


R,
Emeric
Sorry, only registered users may post in this forum.

Click here to login