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
