diff mbox series

[FFmpeg-devel,4/6] lavf/tls_mbedtls: fix handling of certification validation failures

Message ID DU0PR03MB9567EEB5404C31C4A4AE7EF3ECEE2@DU0PR03MB9567.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,1/6] lavf/tls_mbedtls: handle more error codes for human-readable message | expand

Checks

Context Check Description
yinshiyou/configure_loongarch64 warning Failed to apply patch
andriy/configure_x86 warning Failed to apply patch

Commit Message

sfan5 May 17, 2024, 8:34 a.m. UTC
We manually check the verification status after the handshake has completed
using mbedtls_ssl_get_verify_result(). However with VERIFY_REQUIRED
mbedtls_ssl_handshake() already returns an error, so this code is never 
reached.
Fix that by using VERIFY_OPTIONAL, which performs the verification but
does not abort the handshake.

Signed-off-by: sfan5 <sfan5@live.de>
---
  libavformat/tls_mbedtls.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

mbedtls_ctr_drbg_random, &tls_ctx->ctr_drbg_context);
      mbedtls_ssl_conf_ca_chain(&tls_ctx->ssl_config, &tls_ctx->ca_cert, 
NULL);
  -- 2.45.1

Comments

Michael Niedermayer May 18, 2024, 7:53 p.m. UTC | #1
On Fri, May 17, 2024 at 10:34:41AM +0200, Sfan5 wrote:
> We manually check the verification status after the handshake has completed
> using mbedtls_ssl_get_verify_result(). However with VERIFY_REQUIRED
> mbedtls_ssl_handshake() already returns an error, so this code is never
> reached.
> Fix that by using VERIFY_OPTIONAL, which performs the verification but
> does not abort the handshake.
> 
> Signed-off-by: sfan5 <sfan5@live.de>
> ---
>  libavformat/tls_mbedtls.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/tls_mbedtls.c b/libavformat/tls_mbedtls.c
> index 9508fe3436..67d5c568b9 100644
> --- a/libavformat/tls_mbedtls.c
> +++ b/libavformat/tls_mbedtls.c
> @@ -263,8 +263,9 @@ static int tls_open(URLContext *h, const char *uri, int
> flags, AVDictionary **op
>          goto fail;
>      }
>  +    // not VERIFY_REQUIRED because we manually check after handshake
>      mbedtls_ssl_conf_authmode(&tls_ctx->ssl_config,
> -                              shr->verify ? MBEDTLS_SSL_VERIFY_REQUIRED :
> MBEDTLS_SSL_VERIFY_NONE);
> +                              shr->verify ? MBEDTLS_SSL_VERIFY_OPTIONAL :
> MBEDTLS_SSL_VERIFY_NONE);
>      mbedtls_ssl_conf_rng(&tls_ctx->ssl_config, mbedtls_ctr_drbg_random,
> &tls_ctx->ctr_drbg_context);
>      mbedtls_ssl_conf_ca_chain(&tls_ctx->ssl_config, &tls_ctx->ca_cert,
> NULL);

This patch looks corrupted by extra line breaks

[...]
sfan5 May 21, 2024, 10:13 a.m. UTC | #2
Am 18.05.24 um 21:53 schrieb Michael Niedermayer:
> On Fri, May 17, 2024 at 10:34:41AM +0200, Sfan5 wrote:
>> We manually check the verification status after the handshake has completed
>> using mbedtls_ssl_get_verify_result(). However with VERIFY_REQUIRED
>> mbedtls_ssl_handshake() already returns an error, so this code is never
>> reached.
>> Fix that by using VERIFY_OPTIONAL, which performs the verification but
>> does not abort the handshake.
>>
>> Signed-off-by: sfan5 <sfan5@live.de>
>> ---
>>   libavformat/tls_mbedtls.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/tls_mbedtls.c b/libavformat/tls_mbedtls.c
>> index 9508fe3436..67d5c568b9 100644
>> --- a/libavformat/tls_mbedtls.c
>> +++ b/libavformat/tls_mbedtls.c
>> @@ -263,8 +263,9 @@ static int tls_open(URLContext *h, const char *uri, int
>> flags, AVDictionary **op
>>           goto fail;
>>       }
>>   +    // not VERIFY_REQUIRED because we manually check after handshake
>>       mbedtls_ssl_conf_authmode(&tls_ctx->ssl_config,
>> -                              shr->verify ? MBEDTLS_SSL_VERIFY_REQUIRED :
>> MBEDTLS_SSL_VERIFY_NONE);
>> +                              shr->verify ? MBEDTLS_SSL_VERIFY_OPTIONAL :
>> MBEDTLS_SSL_VERIFY_NONE);
>>       mbedtls_ssl_conf_rng(&tls_ctx->ssl_config, mbedtls_ctr_drbg_random,
>> &tls_ctx->ctr_drbg_context);
>>       mbedtls_ssl_conf_ca_chain(&tls_ctx->ssl_config, &tls_ctx->ca_cert,
>> NULL);
> This patch looks corrupted by extra line breaks
>
> [...]

Thanks for pointing that out.

It looks like years later Microsoft is still incapable of leaving 
patches intact... Will send as attachments for v2.
diff mbox series

Patch

diff --git a/libavformat/tls_mbedtls.c b/libavformat/tls_mbedtls.c
index 9508fe3436..67d5c568b9 100644
--- a/libavformat/tls_mbedtls.c
+++ b/libavformat/tls_mbedtls.c
@@ -263,8 +263,9 @@  static int tls_open(URLContext *h, const char *uri, 
int flags, AVDictionary **op
          goto fail;
      }
  +    // not VERIFY_REQUIRED because we manually check after handshake
      mbedtls_ssl_conf_authmode(&tls_ctx->ssl_config,
-                              shr->verify ? MBEDTLS_SSL_VERIFY_REQUIRED 
: MBEDTLS_SSL_VERIFY_NONE);
+                              shr->verify ? MBEDTLS_SSL_VERIFY_OPTIONAL 
: MBEDTLS_SSL_VERIFY_NONE);
      mbedtls_ssl_conf_rng(&tls_ctx->ssl_config,