search

Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.25k stars 2.56k forks source link

stat-then-open race in x509_crt.c #6108

Open daverodgman opened 2 years ago

daverodgman commented 2 years ago

In mbedtls_x509_crt_parse_file there are a few race conditions where a file could be modified/removed between directory iteration, calling stat and lstat, and opening/reading the file, as discussed in https://github.com/Mbed-TLS/mbedtls/pull/2602#discussion_r278707921 . No obvious security concerns here but would be nice to eliminate these.

migueltmsp commented 2 years ago

I'm starting to contribute on OSS and I've become familiar with this project. Is this issue still open so I may contribute to it?

gilles-peskine-arm commented 2 years ago

@migueltmsp Welcome, and thanks for offering! Yes, this issue is open, and has no one assigned to it, so you're welcome to take it up if you want.

We generally require non-regression tests for bug fixes. However at first glance this particular issue is only a problem if there's a race condition, so it might be difficult to test. If it's too difficult, we won't require a test.

migueltmsp commented 2 years ago

Thank you. May I ask to be assigned to it?

kartik9k commented 4 months ago

I see this as an interesting problem and I'm willing to contribute a patch for this issue. Since it's still open, I'm assuming the assignee has abandoned this activity.

I've done a bit of research and I could see that a patch was made in curl and there is a very interesting article which describes a probabilistic solution for this stat-then-open race condition.

Kindly let me know if you think you'd be willing to take a patch for this. I propose a timeline of ~3 weeks to be able to provide a patch. I do not think there could be tests added around it, but I'll try to do a POC around the fix on a local application setup mimicking the behaviour.

gilles-peskine-arm commented 4 months ago

@kartik9k We'd welcome a patch for this, thanks for offering! It isn't critical, since in our case we don't think there's a potential security vulnerability, but more robustness is always good.

Please do note that we don't want to reduce portability to platforms that don't have a full POSIX interface: Mbed TLS runs on many embedded OSes. So we often can't switch from traditional Unix functions to more modern alternatives that are less race-prone. I don't know if it's a concern in this specific case.