Race condition leading to circumvention of precondition checks

[untitaker]

Example

Two PUTs (called a and b) requests to the same filepath with If-None-Match: * are launched at the same time:

• RemoteStorage->putDocument is called for both requests. Precondition checking succeeds in both cases because the file does not exist yet, and in each request process/thread, the function proceeds to call DocumentStorage->putDocument.
• The routine for creating the necessary subfolders (the loop at the start of DocumentStorage->putDocument) may or may not happen in parallel. If it does happen in parallel, the upload will crash due to another (unimportant) race condition. But if it does not crash...
• file_put_contents is called for request a. Since the file doesn't exist, it is created. A filepointer to that file is opened, and the content of request a is written to it.
• file_put_contents is called for request b. Since the file does exist, a filepointer to it is opened. The content of request b is written to it.
• Any not-trivially-small file will require multiple writes to the same filepointer. The writes for both requests may become interleaved in arbitrary ways. Some writes may overwrite others. The resulting file becomes garbage. 201 Created. The end.

---

There are more race conditions in php-remote-storage, and there are more scenarios with the same race condition as root cause.

Solution

Locks everywhere. Basically lock the whole user when creating directories, and later only lock the target file. You need the same thing for GET, DELETE, etc.

I'm not very familiar with PHP, but I'd advise not to use builtin flock. It uses POSIX advisory locking, which (at least on Unix) means that your file lock is per-process, which means that one process can call flock repeatedly without waiting. This is totally fine if each request has its own process, but it renders the whole mechanism completely useless otherwise (every request is able to acquire the lock at the same time, since it's all in the same process). In short, whether flock actually works depends on what appserver you use.

[fkooman]

Thanks for your report!

As far as I can see, this only affects users that run the same application (or applications with the same category/folder permissions on different browsers/sessions at the same time)? So for example your bookmark application where you happen to add two bookmarks at the same time in both sessions. Is this a realistic scenario?

Regarding flock(): it seems PHP-FPM has multiple processes running at the same time, so if locks only work per process then it wouldn't work. It seems it is up to the OS whether or not to enforce locks. A quick test on my Fedora 23 and CentOS 7 machines show that it is enforced by running multiple PHP instances using the test script. See http://www.hackingwithphp.com/8/11/0/locking-files-with-flock.

Regarding locking directories seems a bit harder. I guess if multiple clients at the same time try to create directories the last one will just fail and return an error (or ignore the error and continue). Reaching the PUT it will block then on the aforementioned lock.

Did you test this on php-remote-storage? It would be interesting to write a large file to storage.tuxed.net and then also immediately upload a 1 byte file with the same name while the 8MB upload is still busy to trigger this behavior. Do you have a test application to trigger this behavior?

What error codes should be returned when an operation cannot be completed due to file locking? E.g. a GET to a resource should get what response code?

[fkooman]

I just tested this on storage.tuxed.net. Process A is a 1MB upload, process B is a 1kB upload.

I start A, and immediately after start B. A keeps going, B finishes first, a while after A completes as well. The actual file written is the contents of A. I checked with a sha256sum, which is kind interesting. Maybe it is by accident?

https://github.com/fkooman/php-remote-storage/blob/569b0ee189ebd8ee0831f2e436188b0d5f2ae9e2/src/fkooman/RemoteStorage/DocumentStorage.php#L103

I could enable locking for file_put_contents, see https://secure.php.net/manual/en/function.file-put-contents.php, but this will not be quite enough as we want to terminate the second attempt with an error instead of just waiting for the first to succeed and then overwrite the first one again, or not? The specification says nothing about the intended behavior...maybe we should make that explicit? What do you think? How does your server respond?

[fkooman]

I also tested on storage.5apps.com, the behavior there is exactly the same as on my server...

CC @skddc

[untitaker]

Are you sure A actually opens the connection first? Perhaps B finishes before A opens the connection. Are you sending "If-None-Match: *"?

That header is important, otherwise that behavior would be perfectly fine.

On 12 January 2016 11:59:35 CET, "François Kooman" [email protected] wrote:

I just tested this on storage.tuxed.net. Process A is a 1MB upload, process B is a 1kB upload.

I start A, and immediately after start B. A keeps going, B finishes first, a while after A completes as well. The actual file written is the contents of A. I checked with a sha256sum, which is kind interesting. Maybe it is by accident?

https://github.com/fkooman/php-remote-storage/blob/569b0ee189ebd8ee0831f2e436188b0d5f2ae9e2/src/fkooman/RemoteStorage/DocumentStorage.php#L103

I could enable locking for file_put_contents, see https://secure.php.net/manual/en/function.file-put-contents.php, but this will not be quite enough as we want to terminate the second attempt with an error instead of just waiting for the first to succeed and then overwrite the first one again, or not? The specification says nothing about the intended behavior...maybe we should make that explicit? What do you think? How does your server respond?

---

Reply to this email directly or view it on GitHub: https://github.com/fkooman/php-remote-storage/issues/40#issuecomment-170876121

Sent from my phone. Please excuse my brevity.

[untitaker]

I've just had an idea how to test for this. I'm at work atm but will be available in the evening, for writing a test. Do you have time then to discuss this further on IRC?

On 12 January 2016 12:01:40 CET, "François Kooman" [email protected] wrote:

I also tested on storage.5apps.com, the behavior there is exactly the same as on my server...

---

Reply to this email directly or view it on GitHub: https://github.com/fkooman/php-remote-storage/issues/40#issuecomment-170876688

Sent from my phone. Please excuse my brevity.

[fkooman]

Ah, I understand it now.

When uploading a file it is placed in the request body in its entirety. So, PHP will only see the data when the upload is complete basically. Process B has a "quick" upload while the request body is still not full from process A and just sneak through. By the time process A is done, it will simply overwrite process B's file again like it never happened.

Actually, if you add If-None-Match: * it would probably prevent process A from writing the file at all, by the time it is ready uploading, process B will have created the file and process A will just fail with the precondition failed because the file exists now...

[fkooman]

That is not to say file locking is not needed anymore, but it will be a lot harder to trigger and test for I think...

[untitaker]

Oh I get it, yeah. The request body in PHP is a string, so it is completely buffered before PHP starts php-remote-storage.

[fkooman]

tested with If-None-Match: * and the above behavior is indeed what happens...

[untitaker]

You're not trying hard enough :)

https://gist.github.com/untitaker/9db4e2faeee887fc1aa5

[untitaker]

Annotations in a minute

[untitaker]

Updated, see gist