[FFmpeg-devel] [PATCH] libavformat/tls_mbedtls: Changes the return code handling of mbedtls_x509_crt_parse_file

[FFmpeg-devel] [PATCH] libavformat/tls_mbedtls: Changes the return code handling of mbedtls_x509_crt_parse_file

[Mohit Gupta]

[-- Attachment #1.1.1: Type: text/plain, Size: 1197 bytes --]

mbedtls_x509_crt_parse_file returns an error with negative numbers, and
positive numbers indicate the number of failed certificates to load from
certificate specific issues, such as critical extensions.

This would fix ticket #11079.

Signed-off-by: Mohit Gupta <git@skybound.link>
---
  libavformat/tls_mbedtls.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavformat/tls_mbedtls.c b/libavformat/tls_mbedtls.c
index 567b95b..97094e3 100644
--- a/libavformat/tls_mbedtls.c
+++ b/libavformat/tls_mbedtls.c
@@ -223,9 +223,11 @@ static int tls_open(URLContext *h, const char *uri, 
int flags, AVDictionary **op
       // load trusted CA
      if (shr->ca_file) {
-        if ((ret = mbedtls_x509_crt_parse_file(&tls_ctx->ca_cert, 
shr->ca_file)) != 0) {
+        if ((ret = mbedtls_x509_crt_parse_file(&tls_ctx->ca_cert, 
shr->ca_file)) < 0) {
              av_log(h, AV_LOG_ERROR, "mbedtls_x509_crt_parse_file for 
CA cert returned %d\n", ret);
              goto fail;
+        } else if (ret > 0) {
+            av_log(h, AV_LOG_DEBUG, "mbedtls_x509_crt_parse_file 
skipped %d certificate(s)\n", ret);
          }
      }
  -- 2.45.2


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavformat/tls_mbedtls: Changes the return code handling of mbedtls_x509_crt_parse_file

[Marth64]

LGTM after checking documentation of
mbedtls_x509_crt_parse_file()
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavformat/tls_mbedtls: Changes the return code handling of mbedtls_x509_crt_parse_file

[Marth64]

> av_log(h, AV_LOG_DEBUG, "mbedtls_x509_crt_parse_file skipped %d
certificate(s)\n", ret);

Is it worth it making this a higher log level? Or is it too much noise?
Thinking if it’s important security information to share with the user.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavformat/tls_mbedtls: Changes the return code handling of mbedtls_x509_crt_parse_file

[Mohit Gupta]

[-- Attachment #1.1.1: Type: text/plain, Size: 608 bytes --]

Could do. What level were you thinking? WARN?

On 14/07/2024 17:29, Marth64 wrote:
>> av_log(h, AV_LOG_DEBUG, "mbedtls_x509_crt_parse_file skipped %d
> certificate(s)\n", ret);
>
> Is it worth it making this a higher log level? Or is it too much noise?
> Thinking if it’s important security information to share with the user.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavformat/tls_mbedtls: Changes the return code handling of mbedtls_x509_crt_parse_file

[Marth64]

> Could do. What level were you thinking? WARN?

How about,
```
av_log(h, AV_LOG_WARNING, "Failed to process %d certificate(s) from
the CA bundle, ignoring these certificates\n", ret);
```


Thank you,
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavformat/tls_mbedtls: Changes the return code handling of mbedtls_x509_crt_parse_file

[Mohit Gupta]

Sounds good. Sorry first time contributing, should I make another patch
with the change and send that through again with git send-email?

Thanks
Mohit

On Tue, Jul 16, 2024 at 2:26 AM Marth64 <marth64@proxyid.net> wrote:

> > Could do. What level were you thinking? WARN?
>
> How about,
> ```
> av_log(h, AV_LOG_WARNING, "Failed to process %d certificate(s) from
> the CA bundle, ignoring these certificates\n", ret);
> ```
>
>
> Thank you,
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] libavformat/tls_mbedtls: Changes the return code handling of mbedtls_x509_crt_parse_file

[Marth64]

Disclaimer: I am a light contributor and do not have access
to actually merge this patch, only making suggestions based
on my learnings.

> Sounds good. Sorry first time contributing, should I make another patch
> with the change and send that through again with git send-email?
Yes, except when you do git format-patch, specify "-v2" at the end
and provide the message ID of your original email, so it processes as a reply.

This will prefix the patch subject with "V2" to let readers know you have
updated it.

Thank you!
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH v2] libavformat/tls_mbedtls: Changes the return code handling of mbedtls_x509_crt_parse_file

[Mohit Gupta]

[-- Attachment #1.1.1: Type: text/plain, Size: 1237 bytes --]

mbedtls_x509_crt_parse_file returns an error with negative numbers, and 
positive numbers indicate the number of failed certificates to load from 
certificate specific issues, such as critical extensions.

This would fix ticket #11079.

Signed-off-by: Mohit Gupta <git@skybound.link>
---
  libavformat/tls_mbedtls.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavformat/tls_mbedtls.c b/libavformat/tls_mbedtls.c
index 567b95b129..ccf5ee38ad 100644
--- a/libavformat/tls_mbedtls.c
+++ b/libavformat/tls_mbedtls.c
@@ -223,9 +223,11 @@ static int tls_open(URLContext *h, const char *uri, 
int flags, AVDictionary **op
       // load trusted CA
      if (shr->ca_file) {
-        if ((ret = mbedtls_x509_crt_parse_file(&tls_ctx->ca_cert, 
shr->ca_file)) != 0) {
+        if ((ret = mbedtls_x509_crt_parse_file(&tls_ctx->ca_cert, 
shr->ca_file)) < 0) {
              av_log(h, AV_LOG_ERROR, "mbedtls_x509_crt_parse_file for 
CA cert returned %d\n", ret);
              goto fail;
+        } else if (ret > 0) {
+            av_log(h, AV_LOG_WARNING, "Failed to process %d 
certificate(s) from the CA bundle, ignoring these certificates\n", ret);
          }
      }
  -- 2.45.2


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] libavformat/tls_mbedtls: Changes the return code handling of mbedtls_x509_crt_parse_file

[Marth64]

Hi, good day,

I think something is wrong with this v2 patch, it is showing as corrupt in
the patchwork system.
See here:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/69b8a7a5-3826-4aef-88e1-2984d61a58bb@skybound.link/
By chance did you modify the .patch file directly?
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2] libavformat/tls_mbedtls: Changes the return code handling of mbedtls_x509_crt_parse_file

[Mohit Gupta]

[-- Attachment #1.1.1: Type: text/plain, Size: 841 bytes --]

Interesting, I did not personally. I sent it by generating an .eml file 
with git and then opening that in Thunderbird to send. I wonder if 
Thunderbird editor mangled it or something. Let me try sending directly 
via git as a -v3.

On 01/08/2024 05:50, Marth64 wrote:
> Hi, good day,
>
> I think something is wrong with this v2 patch, it is showing as corrupt in
> the patchwork system.
> See here:
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/69b8a7a5-3826-4aef-88e1-2984d61a58bb@skybound.link/
> By chance did you modify the .patch file directly?
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH v3] libavformat/tls_mbedtls: Changes the return code handling of mbedtls_x509_crt_parse_file

[Mohit Gupta]

mbedtls_x509_crt_parse_file returns an error with negative numbers, and positive numbers indicate the number of failed certificates to load from certificate specific issues, such as critical extensions.

This would fix ticket #11079.

Signed-off-by: Mohit Gupta <git@skybound.link>
---
 libavformat/tls_mbedtls.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavformat/tls_mbedtls.c b/libavformat/tls_mbedtls.c
index 567b95b129..ccf5ee38ad 100644
--- a/libavformat/tls_mbedtls.c
+++ b/libavformat/tls_mbedtls.c
@@ -223,9 +223,11 @@ static int tls_open(URLContext *h, const char *uri, int flags, AVDictionary **op
 
     // load trusted CA
     if (shr->ca_file) {
-        if ((ret = mbedtls_x509_crt_parse_file(&tls_ctx->ca_cert, shr->ca_file)) != 0) {
+        if ((ret = mbedtls_x509_crt_parse_file(&tls_ctx->ca_cert, shr->ca_file)) < 0) {
             av_log(h, AV_LOG_ERROR, "mbedtls_x509_crt_parse_file for CA cert returned %d\n", ret);
             goto fail;
+        } else if (ret > 0) {
+            av_log(h, AV_LOG_WARNING, "Failed to process %d certificate(s) from the CA bundle, ignoring these certificates\n", ret);
         }
     }
 
-- 
2.45.2

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".