Fork me on GitHub
Subscribe 6

Ticket #871 (fixed bug)

Caching breaks on concurrent access, possible information disclosure

  • Created: 2013-07-09 08:36:39
  • Reported by: Bluewind
  • Assigned to: Franz
  • Milestone: 1.5.4
  • Component: caching
  • Priority: normal

Cache files are opened with fopen in "wb" mode and no exclusive flag or locking. Therefore multiple concurrent processes will be able to open the same cache file at the same time.

If both processes have different data to write to the file and the longer data gets written before the shorter one, the excessive part of the longer data will not be removed and it will be displayed in the output.

Happened on the Arch Linux BBS bbs.archlinux.org/viewtopic.php?id=166425 for the cache_bans.php file.

You should either use some variable cache like memcache that can handle concurrent access or use a lock file or use O_EXCL when opening the cache file to writing.

History

adaur 2013-07-09 11:07:42

  • Milestone set to 1.5.4.

Thanks for the report. Should we use "x" or "c" instead of "wb" ?

http://php.net/manual/en/function.fopen.php
http://pubs.opengroup.org/onlinepubs/00 … fopen.html

Bluewind 2013-07-09 12:25:30

I'd go with using fopen("wb"), flock(LOCK_EX), ftruncate(0), fwrite(), flock(LOCK_UN), fclose().

You should also create a dedicated function instead of duplicating the fopen, fwrite, fclose code everywhere.

Edit: You could use fopen("x"), but that would introduce another race condition when you check if the file exists before including it.

Also know that flock might not work over NFS.

Comment edited 1 times (Diff)

adaur 2013-07-10 09:09:03

Thanks for the details, we'll look into it. Have a nice day!

Franz 2013-07-11 20:12:03

  • Owner set to adaur.

Can you take care of that? smile

Thanks for the report, good catch!

Franz 2013-08-07 16:23:18

  • Owner changed from adaur to Franz.

Okay, I just implemented locking when writing the cache files.

Can somebody please take a look at and test my implementation? It seemed to work here, but you never know. Plus it's obviously hard to test the exact scenario that was mentioned by the ticket creator.

Franz 2013-08-07 16:25:44

Commit f042ec5 to fluxbb master

#871: Improve caching to not break on concurrent write access on the files.

Bluewind 2013-08-07 16:56:17

Looks good, you can easily test this in a php shell (php -a).

Franz 2013-08-07 21:25:52

Yeah, I tested the writing of the files. What I cannot test is having the two concurrent accesses of the file.

Franz 2013-08-08 22:02:59

Anybody?

adaur 2013-08-09 19:49:57

Sorry about this one, Franz. I've been on holidays for too long tongue.

Franz 2013-08-09 20:46:25

No problem. Does the code look okay to you?

adaur 2013-08-09 21:00:39

I don't know much how to handle files in PHP but it looks OK for all I know.

quy 2013-08-10 14:42:50

Update extern.php???? (// Output feed as PHP code; lines 439-451)

Franz 2013-08-10 16:16:54

Thank you, done.

Franz 2013-08-10 16:17:07

Commit 9ff26f4 to fluxbb master

#871: Make use of fluxbb_write_cache_file() function in extern.php, too.

Franz 2013-08-10 19:34:33

  • Status changed from open to fixed.

Okay, this is running on our server now. Seems to work. smile

Visman 2013-08-11 08:00:29

$pun_user - an excess global variable in generate_quickjump_cache()

Franz 2013-08-11 21:24:19

Done, thanks.