What am I missing?
Then, you added a few irrelevant changes that to the inexperienced eye look like security fixes https://plugins.trac.wordpress.org/changeset?old_path=%2Fadv...
However, these are no fixes. You just introduce a new variable, that you never use, and re-assign the same contents of that new variable back to the $_REQUEST
Unless you show proof of a security fix - which you could have pushed to users WITHOUT renaming the plugin, WITHOUT removing original, non-security related code, and WITHOUT breaking compatibility with the PRO features - you have LIED and STOLEN code in the name of WP.ORG
This will hopefully be recognized by WP Engine and if god wills, remove you from the equation once and for all legally speaking.
While this whole takeover thing is completely ridiculous, it's you who displays an "inexperienced eye" here. What do you think the $original_post variable (which was already there) is doing, huh?
Security issues can be fixed WITHOUT renaming the plugin or removing links and text even if the original author has no access anymore
And that “fix” is ridiculous. If anything it breaks code of users who were actually adding callbacks using that data. It’s the nature of php that you can access those details - it’s up to the caller to know what to do with it. If anything, the usage of user callback is an issue here.
And in any thinkable case this ain’t a security fix that was done. A security fix would include that and only that change.
Of course, relying on such simplistic measures is still brittle and inelegant, but that's another matter. Reworking it would likely be quite invasive to that codebase and far beyond the scope of a security patch.
(also, frankly, the entire WordPress ecosystem isn't particularly known for its high quality codebase... this kind of "fix" is exactly what you'd expect there even without all that drama around)
> Security issues can be fixed WITHOUT renaming the plugin or removing links and text even if the original author has no access anymore
Not sure who you're arguing with there, but certainly not with me.
You have plenty of shitty behavior to call out there, so not sure why you decided to announce that there's no security issue being handled at all instead. It only makes your point weaker for no good reason.
How on earth does emptying POST or REQUEST solve anything at all in regards? How on earth does, no matter what crap ACF added BEFORE the takeover, this "Fix" justify a hostile takeover? If or not there is a security issue with this code (which there IS, but not with POST or REQUEST data) is not even the matter anymore - it was and is posed and defended as a "urgent action to fix a security issue in a plugin the author has no access to"
And I repeat - there has not been any security fix!!
Read my root comment: > because the only relevant changes are actually neither introducing fixes, nor ever changing the plugin core code in a way that fixes security issues.
And I stand by that. Anyone reading this code can see it.
That said, you clearly seem to be confused about the nature of the issue being fixed there. It's clear from the existing code that the contract between this function and the callback getting executed is that the callback is expected to execute in a kind of a sanitized environment with restricted execution abilities.
You can argue that it's a really weak sandbox and that the whole design is smelly, and I'll agree with you. However, that's how this code was designed and that's what users are running on their servers, and that's how they expect it to behave. It prevents the callback from calling functions with `wp_` prefix and it disallows it from reading or making changes to user's POST data.
Now, if you find a way to circumvent those restrictions, it's an obvious security issue. Someone may have deployed some code that relied on that contract, and now that contract is known to be invalid (it always had been, but it wasn't known before). Therefore, stopping that from happening is a security fix.
It may be a poor fix - and in fact, this one is, as it's incomplete. But it's a fix. The upstream project recognized that and applied a similar, but a bit more thorough approach in its repo:
https://github.com/AdvancedCustomFields/acf/commit/a60034f8a...
It does exactly what the change we're discussing does, plus a bit more.
You still wouldn't convince me that it's a bulletproof sandbox and I already have some other ideas in my head on how it could potentially be circumvented after reading that code (though my PHP is so rusty I may very well be wrong), but the change in question is clearly a security fix, recognized and applied by both Automattic and WP Engine and I really can't understand why you're so keen on implying otherwise. As I already said, it only made your other (good) points seem weaker.
call_user_func was still called. Clearly, the idea behind is is not "some kind of sanitized env" since the start. The idea was "the user can throw in a callback and it will be executed". Then someone said "but that is unsafe" (And it is!) But the unsafely thing here is not "You have access to POST data or wp_ functions". That is the default in ANY code attached to WP anyway, and while being part of the danger here, the real danger is that an arbitrary POST EDITOR can throw in a callback and it executes that.
Which is why, yes, somehow, clearing posted data and excluding already existing methods like wp_ stuff is sort of a "fix" for that, since before the Post Editor did not even need to write the callbacks code: he could just have called a eventually bad (in context) registered method in core. So the "fix" does somehow mitigate, but not fix the issue.
I can see upstream builds on that idea even more. However the code still uses user callbacks, and those user callbacks can still be unsafe. You just need to throw in a callback that does something malicious, which does not even be an obvious malicious code. It could be a callback registered elsewhere, where the else context makes sense and is not flagged, but in conjunction with this ACF feature, would be malicious.
It should be clear that the security issue here is not what you can access during that callback - the security issue is that the callbacks are not whitelisted, and/or allowed at all (which can be considered a problem too, but would break potency of the features of course if removed)
Not putting my hands in fire for this, but I believe there is reason I Could not find a CVE yet for this alleged security issue, only a reserved one. I suspect that is, because the issue is still there, and publishing it, would immediately render it more dangerous. I again would love to learn that I am wrong here.
I do stand by my original post that said: > because the only relevant changes are actually neither introducing fixes, nor ever changing the plugin core code in a way that fixes security issues.
This stands true: the security issue was not fixed. If we start to call incomplete fixes a fix, then we can as well call anything anywhere a fix. Heck, I moved around some lines and cleared some of the data. It's fixed! That would never hold true. In all and every case, plugin review team would immediately review the FIX before you can even say "but", and they would immediately throw it back at you, asking for an _actual_ fix. Especially with call_user_func. This has not happened here at all, which just adds to the fun of the day.
I feel the discussion should evolve around this, and The Guys who yelled "Security" should come forward and explain to the public what they actually fixed, if they truly believe this fixes the issue, if they truly believe that ACF (sorry, SCF) is now _safe_, or not.
My point stands that it (the core issue) has not been fixed.
Sorry if it got a long post.
The real danger is that an arbitrary post editor can throw in a callback and it gets executed unsanitized. Having a proper sandbox would be a perfectly valid solution - in the end, that's the whole modus operandi of the web browser you're using to write these comments. And yes, I also have doubts whether the implemented measures are nearly enough to actually sanitize the input; I'm also not sure whether you can sandbox that feature properly without making it effectively useless - and while neither of those justify Automattic's behavior, it's a different accusation.
I think we might agree - and my original wording was tainted by emotions.
- indeed, there was changes in code that can be sold as “attempted security fix” - indeed, as I think we both agree, the main security issue still needs attention to this very day