Use of include() et al for data files is not compatible with PHP opcache

[mikkorantalainen]

SimpleSAML PHP code contains lots of code that writes new PHP code in various files and then tries to load those files using include() (or some variant of it like require_once()). This is not a safe way to load data files because fully enabled opcache will not reload the files from disk after reading the files once.

Steps to reproduce the behavior:

1. Run SimpleSAML on Apache mod_php module to avoid restarting PHP process all the time.

2. Configure Apache mod_php module to see following php.ini settings:

   [opcache]
   opcache.enable=1
   opcache.enable_cli=1
   opcache.memory_consumption=512
   opcache.interned_strings_buffer=16
   opcache.max_accelerated_files=100000
   opcache.use_cwd=1
   opcache.validate_timestamps=0
   opcache.revalidate_path=0
   opcache.save_comments=0
   opcache.validate_permission=0
   

3. Configure e.g. federation metadata refresh to happen via Cron calling .../cron/cron.php through Apache mod_php.

Expected behavior
The metarefresh cron task should be able to successfully update federation metadata.

Actual behavior
The metarefresh cron task fails to update the federation metadata correctly because one of the following reasons:

1. File .../data/metarefresh-state.php contains stale data because PHP opcache will remember the first time contents for that file.
2. File .../metadata/$IDPKEY/saml20-idp-remote.php may be updated but never re-read by Apache childs so it will appear to contain the data it was read the first time since starting the Apache process.

Obviously, the first fetch of metadata will work because in that case no previous files exist so those cannot be cached by opcache, so this bug is about refreshing the data.

Additional information

The same logic is broken everywhere where files are written to filesystem and include() is used to attempt to read those again. When PHP opcache is fully enabled like above PHP code cannot assume that include() will actually re-read any files from the filesystem. The code must use e.g. file_get_contents() or some other functions that actually read files instead of blindly assuming that include() will do the trick.

In addition, use of include() causes huge potential security risk because any encoding bug in the process that writes new PHP files will be turned in server side code execution. This risk is not worth taking for data files.

Expecting include() to always reload the file from the filesystem is not a safe assumption in PHP!

The opcache.enable_cli is not needed to reproduce this with Apache, but using that may be good enough to make this reproducible via automated tests.

Workaround for the current implementation

To get SimpleSAML metadata refresh to work as designed while using fully tuned opcache, one must execute graceful restart of Apache, execute the trigger to run cron hooks and then re-execute graceful restart to make changes made by cron hooks visible to all SimpleSAML logic. The graceful restart before the cron hook is required to make sure that the PHP code sees the latest version of file .../data/metarefresh-state.php so that the conditional GET is correctly executed.

Another alternative is to change the PHP config to always revalidate files from the filesystem before using the PHP opcache but since this is Apache wide setting, any other services running on the same PHP server will take the performance hit from this change. As such, this is acceptable only if the served traffic is low enough to make this better solution than executing Apache graceful restart before and after the cron hooks.

[tvdijen]

I agree that we should minimize the use of include() and alike, but there will always be cases where this is not possible to mitigate. The fact that we use php files for metadata is both extremely flexible, but also problematic on the other hand, like you found out the hard way.

Your best bet is to exclude the data/ and metadata/ directories by using the opcache blacklist.

[thijskh]

Indeed as @tvdijen mentioned it's not an option to replace these files with plain configuration files because precisely one of the flexibilities of SimpleSAML is that the configuration can contain code where needed.

It seems you've disabled validate_timestamps. If you would enable this, I think the problem is also solved. I'm not sure the performance gain is huge from disabling validate_timestamps to begin with? If it is, you could consider to raise revalidate_freq to a bit higher value.

We could consider to call opcache_invalidate() on any file that we write out. This should work when calling metarefresh via the cron module, I think the most common scenario in which we actually write out files. Is this something that could work for you?

[mikkorantalainen]

Oh, I didn't even know about opcache_invalidate(). I think calling it after writing to any file that is later include()d should fix the stale file problem. Of course, it may be hard to track all the locations that write files that will be later included by some other logic.

The specific issue I used as an example should be fixed by calling opcache_invalidate() after modifying .../data/metarefresh-state.php and .../metadata/$IDPKEY/saml20-idp-remote.php.

Are there other known files that would be often automatically modifed and re-loading the correct version is important?

(A poor man's fix would be to call opcache_invalidate() before calling include() for any file that should be re-read from the disk in all cases. That might be easier to implement but it would cause worse performance.)

[thijskh]

As far as I'm aware metarefresh does this and maybe another module or so, but it's not very common.

[mikkorantalainen]

I noticed this issue with version 1.19.4 which already includes that call here https://github.com/simplesamlphp/simplesamlphp/blob/v1.19.4/lib/SimpleSAML/Utils/System.php#L236 so this is not enough to fix the problem for metadata refresh. I was running PHP 7.4 in case it makes a difference.

[mikkorantalainen]

Also note that according to comments in https://www.php.net/manual/en/function.opcache-invalidate.php the opcache_invalidate() only marks the cache entry as stale but it cannot free RAM so using this for lots of big files will cause rapid memory leak to fill the cache. The only way to free the RAM used for cache is to call opcache_reset() (or restart the PHP process).

As such, opcache_invalidate() should be considered a workaround only instead of proper fix. The proper fix is to not use include() to read data files.

I think it's okay to use include() for files that will be modified manually, such as config.php because in that case the system admin would be expected to know that they need to do graceful reload for the Apache since they have configured system to use cached versions always.

However, when SimpleSAML creates files in automated way, there should be zero need to make those as executable files and you should save and read real data instead. JSON is a good option because it doesn't have similar possibility to accidentally execute unintended PHP code like unserialize() or include(). Something like $mydata = json_decode(file_get_contents($filename)) should work fine.

[tvdijen]

I wrote a PoC trigged by #1550 to move away from php-files for config, but to do this for the entire project + modules in a backwards compatible way is going to take a loooooong time. In fact, I don't think I ever got any feedback on the PoC...

[thijskh]

Also note that according to comments in https://www.php.net/manual/en/function.opcache-invalidate.php the opcache_invalidate() only marks the cache entry as stale but it cannot free RAM so using this for lots of big files will cause rapid memory leak to fill the cache. The only way to free the RAM used for cache is to call opcache_reset() (or restart the PHP process).

I don't think this is true. If the entry is invalidated that file will be re-read and added to the cache under that name. So it will not fill up since the filenames will be the same.

This problem would only exist if you want to use opcache_invalidate() to remove files from the cache which you are not going to cache again.

[thijskh]

I noticed this issue with version 1.19.4 which already includes that call here https://github.com/simplesamlphp/simplesamlphp/blob/v1.19.4/lib/SimpleSAML/Utils/System.php#L236 so this is not enough to fix the problem for metadata refresh. I was running PHP 7.4 in case it makes a difference.

Indeed and this was added precisely to enable the metarefresh use case (cd8179d). So probably we should find out why this didn't solve the problem for you.

[thijskh]

However, when SimpleSAML creates files in automated way, there should be zero need to make those as executable files and you should save and read real data instead. JSON is a good option because it doesn't have similar possibility to accidentally execute unintended PHP code like unserialize() or include(). Something like $mydata = json_decode(file_get_contents($filename)) should work fine.

I agree that this would be possible for this specific use case. I just want to clarify why in general the config files are now in this way so that if we want to write out config automatically we now use this format.

I believe a solution here is possible but for reasons stated before I would not want to replace all config with static files because this loses essential flexibility. We could make use of static files as an addition to the interpreted ones.

For this case you could use the already present MetadataStoragehandlerSerialize which metarefresh supports and SimpleSAML supports aswell. If this somehow not suffices one option is to add e.g. also a JSON flatfile metadata storage handler which should not be hard. The only thing we would then need to change is the format of the statefile which metarefresh can decide to change on its own.

[tvdijen]

I believe a solution here is possible but for reasons stated before I would not want to replace all config with static files because this loses essential flexibility. We could make use of static files as an addition to the interpreted ones.

Maybe this issue is not the place to discuss this, but my PoC moves that flexibility to a configuration class. If you want to do something custom/dynamically you can extend that class, overload any method to do your magic, and add a class alias so SSP knowns to use your custom class instead of the default one. I'd be happy to hear what you think of the PoC code.

[mikkorantalainen]

I don't think this is true. If the entry is invalidated that file will be re-read and added to the cache under that name. So it will not fill up since the filenames will be the same.

I think PHP opcode cache implementation has very simple memory management routine which basically allocates new memory range for each recompilation of a file and it simply marks old version as invalidated. It may be able to re-use the old memory range if the new version is smaller in opcode form. There's no logic to defrag the opcode cache so with many enough reloads, the opcode cache will be so fragmented that new version of the file can no longer be stored anywhere.

Basically opcode cache as one big block of RAM and it puts compiled versions of file A, B and C in sequence in the RAM. When B is later updated and the updated version (one byte longer) needs to be written in the cache, the end result will be A, B, C, B' where B is the the old version invalidated and B' is the new version which didn't fit in the same space as B. If later a small enough file D is added to cache, it will be placed as A, D, C, B' and the RAM between end of D and start of C is basically lost to memory fragmentation.