Thanks a lot, this patch also fixes the problem in my setup with the TLS 
1.2-only nginx config.

On 5/16/24 20:17, Theo Buehler wrote:
> On Thu, May 16, 2024 at 04:41:05PM +0200, Andreas Bartelt wrote:
>> Hi,
>>
>> I've chosen to report to ports@ first since I'm not yet sure what's the best
>> mailing list for the kind of problem I've been observing recently.
>>
>> I've deliberately been using a TLS 1.2-only (i.e., no TLS 1.3 support)
>> configuration with nginx for multiple years now and my (previously working)
>> TLS 1.2 config from nginx.conf looks like the following:
>>          ssl_protocols TLSv1.2;
>>          ssl_ciphers ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384;
>>          ssl_prefer_server_ciphers   off;
>>          ssl_ecdh_curve prime256v1;
>>
>> On OpenBSD current and with the following application versions (nginx
>> 1.26.0, LibreSSL 3.9.0, chromium-124.0.6367.201), I've noticed that a
>> significant fraction of HTTPS connections via chrome fail via illegal
>> parameter alert (but not all of them). All HTTPS connections still work
>> perfectly fine via firefox-125.0.3 as well as SSL Labs site from Qualys.
>>
>> I could get rid of these problems (with TLS access to nginx via chrome) by
>> also enabling TLS 1.3 in nginx.conf:
>>          ssl_protocols TLSv1.2 TLSv1.3;
>>          ssl_ciphers TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384;
>>          ssl_prefer_server_ciphers   off;
>>          ssl_ecdh_curve prime256v1;
>>
>> It might be possible that also enabling TLS 1.3 simply masks the current
>> problem with TLS 1.2 support.
>>
>> Can anybody reproduce this problem?
>> Any ideas on how to locate the actual source of this bug?
> 
> It's a side effect of
> 
> https://github.com/openbsd/src/commit/be89428ce5fb176ef7adb0cb4f297af5f2b0fb91
> 
> Specifically, this check is hit:
> 
> 		if (!client_sent_group) {
> 			*alert = SSL_AD_ILLEGAL_PARAMETER;
> 			return 0;
> 		}
> 
> https://github.com/openbsd/src/blob/master/lib/libssl/ssl_tlsext.c#L1587-L1590
> 
> One workaround is to move this check a few lines down so it is only
> triggered for TLSv1.3.
> 
> The problem is that with TLSv1.2 s->hit can be set and at the same time
> no supported groups available in s->session, so
> 
> 	tls1_get_group_list(s, 1, &client_groups, &client_groups_len);
> 
> returns NULL client_groups and length 0, so client_sent_group is never set.
> 
> I can no longer reproduce the problem with the diff below, but it seems
> just as plausible that tls_keyshare_server_process() needs special
> s->hit handling.
> 
> Index: ssl_tlsext.c
> ===================================================================
> RCS file: /cvs/src/lib/libssl/ssl_tlsext.c,v
> diff -u -p -r1.149 ssl_tlsext.c
> --- ssl_tlsext.c	16 Apr 2024 17:46:30 -0000	1.149
> +++ ssl_tlsext.c	16 May 2024 18:06:56 -0000
> @@ -248,7 +248,7 @@ tlsext_supportedgroups_server_process(SS
>   	if (groups_len > TLSEXT_MAX_SUPPORTED_GROUPS)
>   		goto err;
>   
> -	if (s->hit)
> +	if (s->hit && s->session->tlsext_supportedgroups_length > 0)
>   		goto done;
>   
>   	if (s->s3->hs.tls13.hrr) {