Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
daverodgman opened this issue Jul 20, 2022 · 5 comments
Open

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

daverodgman opened this issue Jul 20, 2022 · 5 comments
Labels
bug component-x509 good-first-issue Good for newcomers size-s Estimated task size: small (~2d)

Comments

@daverodgman
Copy link
Contributor

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 #2602 (comment) . No obvious security concerns here but would be nice to eliminate these.

@migueltmsp
Copy link

migueltmsp commented Aug 26, 2022

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
Copy link
Contributor

@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
Copy link

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

@kartik9k
Copy link

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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-x509 good-first-issue Good for newcomers size-s Estimated task size: small (~2d)
Projects
None yet
Development

No branches or pull requests

4 participants