-
-
Notifications
You must be signed in to change notification settings - Fork 460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add verification checks against $_SERVER header/env reads. #2217
Comments
The above contains too little information to see what the actual problem was code-wise, so as things are, it is impossible to determine whether your proposed solution has any merit. Based on the limited information available, this feels off. If anything, I can imagine a security warning about the use of Something like that is against the principle that errors/warnings thrown by sniffs should be fixable via code changes, so unless there are ways to mitigate this in the code which the sniff could reliably check for, this is not a typical candidate for a WPCS based detection mechanism. |
Like Not only According to this test, many other indexes can be overwritten via the environment, whatever that may invoke or resolve into (explained here). The example I shared in my previous comment returns the So, I propose adding caution around using the superglobal. As you're imagining, the warning suggests writing a comment explaining why it is used, whether it's properly sanitized, and what measures are taken to ascertain the validity of the data. It should only be disabled via a Again, after my HackerOne report is patched and disclosed, I can show real-world examples of the broader implications that may make you consider making an exception to the principle. Unfortunately, sanitization functions won't satisfy; only proper code paths. |
The issue is not with In terms of sanitizing, that code is fine. |
The vulnerability I mentioned in my previous reply is fixed and released to the public, so I can give another real-world example. Again in an Automattic plugin, these changes in Jetpack remove calls to There's no workaround implemented, just a simple drop of functionality. Their changelog reads:
The reason is detailed in HackerOne report 1906271 (not sure if it's public, so eh): If we look at wp_is_json_request(), we get this code: function wp_is_json_request() {
# extraneous jump:
if ( isset( $_SERVER['HTTP_ACCEPT'] ) && wp_is_json_media_type( $_SERVER['HTTP_ACCEPT'] ) ) {
return true;
}
# extraneous jump:
if ( isset( $_SERVER['CONTENT_TYPE'] ) && wp_is_json_media_type( $_SERVER['CONTENT_TYPE'] ) ) {
return true;
}
return false;
# extraneous newline
} In that function, there are no checks against attacks other than function wp_is_json_media_type( $media_type ) {
static $cache = array(); # [] is not a function
if ( ! isset( $cache[ $media_type ] ) ) {
$cache[ $media_type ] = (bool) preg_match( '/(^|\s|,)application\/([\w!#\$&-\^\.\+]+\+)?json(\+oembed)?($|\s|;|,)/i', $media_type );
}
return $cache[ $media_type ];
} Practically, that performs a literal string test on an arbitrary value without any authentication for integrity. You can set
The functions To conclude, WordPress Core is vulnerable to manipulation via I consider those two functions as dangerous as To get back to this notion:
@kkmuffme is right; this could be resolved well with a |
To summarize: |
FYI: I've been thinking about this one and for now, my conclusion is that the specs for what should be checked for are too unclear to make this actionable at this time. If the specifications can be defined more precisely, we can revisit. |
As mentioned above, any |
@kkmuffme That's not the part which is unclear... the part which is unclear is what mitigation should be accepted, whether the mitigation only applies to these keys (and therefore would need to be special cased) etc A good set of code samples with "good" and "bad" code patterns may help clarify things. |
There are actually multiple security issues that are connected and extremely often done wrong in tons of plugins:
Technically, restricting to HTTP_ + a few others (content_type,...) would be sufficient, but I think using the "safe" approach is better by disallowing everything, except specified.
Good: anything that has Bad: anything that does not have Now you will obviously say - e.g. tons of developers/plugins added nonce checks, thinking that this is safe:
But obviously it's not. |
@kkmuffme Correct me if I'm wrong, but by your logic above, any kind of website search would be made impossible, Along the same lines, an anonymous user submitting a contact form would also be impossible.... |
Not the contact form/search itself is unsafe - any logic that is built around the user submitted data, which then is used in a context that would normally require authentication. In the above jetpack security issue, the problem is that
It's not about the values, but about how/where they're used. That's exactly the reason why there is no "automatic" way to check for it being safe with wpcs for unauthorized users, but we have to rely on phpcs:ignore, where "current user can" cannot be used, and a 2nd, use case specific validation method needs to be used (e.g. REQUEST_URI,...) that cannot be modified by the user for a specific action in that case (without current user can) Let me give you a very obvious example with $_GET, where you could accidentally end up creating a security issue without being aware, when user data is used to trigger a specific logic.
Suddenly anybody is able to get the apiKey/... by just setting the $_GET parameter. Why using a nonce in the above case is not sufficient, but a current user can check is required is obvious I guess? (if not, I'm happy to expand on that though) That's why a wpcs rule for current user can is needed. Mitigation is possible by extending the nonce rule to SERVER/COOKIE (or is cookie already included?) superglobals |
Is your feature request related to a problem?
Following the widespread vulnerability of WooCommerce Payments, I tried testing the vulnerable code against WPCS -- but WPCS reported nothing.
Describe the solution you'd like
I suggest adding a similar check that exists for
$_POST
:WordPress.Security.NonceVerification.Missing
.Additional context (optional)
The text was updated successfully, but these errors were encountered: