search

Arlodotexe / OwlCore.Storage

The most flexible file system abstraction, ever. Built in partnership with the UWP Community.
16 stars 4 forks source link

Use of File.Exists can lead to TOCTOU bugs #37

Closed sylveon closed 1 year ago

sylveon commented 1 year ago

File.Exists is used in various places in the code before performing an action. These checks should be removed and the operation attempted directly, as they are a source of TOCTOU bugs (which can potentially be security vulnerabilities).

Arlodotexe commented 1 year ago

In software development, time-of-check to time-of-use (TOCTOU, TOCTTOU or TOC/TOU) is a class of software bugs caused by a race condition involving the checking of the state of a part of a system (such as a security credential) and the use of the results of that check.

If you're referring to #28, TOCTOU bugs require both checking and accessing. We are only checking, not accessing.

Could you provide examples of where this is used in our codebase that might cause a check/access race condition? I'm not aware of any.

sylveon commented 1 year ago

We are only checking, not accessing.

Yes, but then the user does later access based on those checks.

Arlodotexe commented 1 year ago

I'm not sure i understand the concern. We do this in the constructor so that check/access race conditions can't happen.

Our API dictates the resource should exist with reasonable confidence on construction so that when you try to access the file and use the object, it doesn't throw. Since our constructor only takes a string, this was my best idea to ensure this.

Arlodotexe commented 1 year ago

Calling Exists is still a one-time constructor call with built-in caching, but for the purpose of https://github.com/Arlodotexe/OwlCore.Storage/issues/25#issuecomment-1504003923 we should look at setting up a simple benchmark to make sure it doesn't hurt performance when a lot of objects are created. There's a performance/safety tradeoff to test here, we need the safety but ideally without perf issues.

Since this issue is about TOCTOU bugs (which can't happen here by design), let's move over to the other issue.