Hello Willy,
On 03/25/2016 01:37 PM, Willy Tarreau wrote:
> Hi Nenad,
>
> On Fri, Mar 25, 2016 at 11:35:01AM +0100, Nenad Merdanovic wrote:
>> diff --git a/src/ssl_sock.c b/src/ssl_sock.c
>> index 1017388..767d6e9 100644
>> --- a/src/ssl_sock.c
>> +++ b/src/ssl_sock.c
>> @@ -5406,7 +5406,7 @@ static int bind_parse_tls_ticket_keys(char **args, int
>> cur_arg, struct proxy *px
>> fclose(f);
>>
>> /* Use penultimate key for encryption, handle when TLS_TICKETS_NO = 1 */
>> - i-=2;
>> + i = (i - 2) % TLS_TICKETS_NO;
>> keys_ref->tls_ticket_enc_index = i < 0 ? 0 : i;
>
> I'm still seeing an issue here which is that i is an integer so
> (i - 2) % TLS_TICKETS_NO will be negative for values of i between
> 0 and 1.
>
Right, but this is covered with:
keys_ref->tls_ticket_enc_index = i < 0 ? 0 : i;
It can only happen in one case, if TLS_TICKET_NO = 1 (build time) and
there is only one key in the file (i = 1). There are explicit checks for
other cases:
#if (defined SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB && TLS_TICKETS_NO > 0)
and
if (i < TLS_TICKETS_NO) {
if (err)
memprintf(err, "'%s' : please supply at least %d
keys in the tls-tickets-file", args[cur_arg+1], TLS_TICKETS_NO);
return ERR_ALERT | ERR_FATAL;
}
The ternary operator here covers the case where the result is negative
and sets the index to 0 (which is the only key we have).
> If this is intended, then maybe it would be better do fix it this way
> instead so that there's no ambiguity regarding the validity of the
> above operation :
>
> - keys_ref->tls_ticket_enc_index = i < 0 ? 0 : i;
> + keys_ref->tls_ticket_enc_index = i < 0 ? 0 : i % TLS_TICKETS_NO;
>
I am afraid this would do the unwanted, because i % TLS_TICKET_NO, as
you said, can be negative, and we'd have a negative index being used as
an array index:
head =
objt_listener(conn->target)->bind_conf->keys_ref->tls_ticket_enc_index;
memcpy(key_name, keys[head].name, 16);
> What do you think ?
>
> Thanks,
> Willy
>
I might've missed something, but I think the fix should be as-is, but
please do not commit it yet, I need to do some more testing.
Regards,
Nenad
