Skip to content
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

Handle falsley claimed security issues on patchstack #237

Open
heiglandreas opened this issue Sep 4, 2023 · 20 comments
Open

Handle falsley claimed security issues on patchstack #237

heiglandreas opened this issue Sep 4, 2023 · 20 comments

Comments

@heiglandreas
Copy link
Owner

@riodrwn of @zerobyte-id has reported 2 issues on patchstack. One XSS issue and one CSRF issue.

Both issues contain no information about what was actually found.

Also both issues contain the information that the issue was reported to the plugin developer - sadly neither here in the github issue-tracker nor in the plugins Support-channel was either of the issues reported.

The linked CVEs CVE-2023-41655 and CVE-2023-41654 have so far only been reserved and contain no further information.

Currently we do not have any information about those "security issues" what so ever. As the plugin works purely in the background and does not display any customer provided information during the login process it is highly unlikely that the claims are actually valid - nevertheless will we do our best to get hold of the report, analyze it and mitigate any possible issues.

@gramakri
Copy link
Contributor

gramakri commented Sep 6, 2023

@heiglandreas Is this the same as https://www.wordfence.com/threat-intel/vulnerabilities/wordpress-plugins/authldap/authldap-258-cross-site-request-forgery ? Per the description "This is due to missing or incorrect nonce validation on the authLdap_options_panel() function"

@heiglandreas
Copy link
Owner Author

heiglandreas commented Sep 6, 2023

Yeah. Looks like it's the same. At least a bit more information is provided there as to what someone thinks the issue is...

Thanks for pointing that out

@heiglandreas
Copy link
Owner Author

And as the funktion is only handled via the WordPress internal functions for settings (add_optios_page or add_submenu_page) that access level checking is already done before that code is reached.

So there is no need to do those checks on the plugin level again...

If that general check is not done, I would consider that a general issue in WordPress and not in every single plugin that is out there...

@riodrwn
Copy link

riodrwn commented Sep 7, 2023 via email

@heiglandreas
Copy link
Owner Author

Well. Some explanation or information at all before going public would be nice in the first place.

One of the issues you have raised (CVE-2023-41655) is not an issue at all as you have disclosed to WordFence:

This makes it possible for authenticated attackers, with administrator-level access and above, to inject arbitrary web scripts in pages that will execute whenever a user accesses an injected page.

Someone that is authenticated and has administrator level access is an Administrator. Whether that person has ill intentions is not up to code to check.

Regarding (CVE-2023-41654) it is fine that you sent a video of a proof of concept to someone else before disclosing this with the maintainers of the affected software. This is not a sensible disclosure at all.

Would I not assume best intent this would look very much like publishing a 0-day exploit with malicious intentions.

I would advise you to open a security advisory here on this repository for each of those issues that you supposedly have found, so that we can discuss the impact and how to handle them. Feel free to also link the video of your PoC there so that I can analyze what is happening and how to mitigate that.

Thank you

@riodrwn
Copy link

riodrwn commented Sep 7, 2023 via email

@heiglandreas
Copy link
Owner Author

heiglandreas commented Sep 7, 2023

CVE-2023-41654 has been adressed. Thanks @riodrwn for mentioning that!

CVE-2023-41655 still is not an issue in my opinion as that allows someone that can manage who can log into the WordPress site to also break the interface to edit those options. The first part is much more of an issue there.

@d19dotca
Copy link
Contributor

d19dotca commented Feb 16, 2024

Just wanted to check in on this. It looks like the nonce was added (#240) which should clear the CSRF reported in Patchstack & Wordfence. https://github.com/heiglandreas/authLdap/blob/2.6.0/authLdap.php#L71-L82.

For the XSS one... Patchstack believes this exists still in the current 2.6.0 release. https://patchstack.com/database/vulnerability/authldap/wordpress-authldap-plugin-2-5-8-cross-site-scripting-xss-vulnerability?_s_id=cve -- "WordPress authLdap Plugin <= 2.6.0 is vulnerable to Cross Site Scripting (XSS)". Is there any update on this one in particular... is this a legitimate vulnerability or not? It seems the author feels this isn't legitimate and is more of a WordPress issue than a plugin issue, so wanted to make sure I understood correctly.

I came across this with a manual scan by Wordfence today.

@heiglandreas
Copy link
Owner Author

The XSS is only relevant in very special circumstances in multisite installations where site-admins are allowed to edit LDAP settings and by doing so might inject code that can then be executed with increased privileges when a super-admin also has access to those same settings.

By doing so the site-admin will also break the authentication as the XSS code will cause the ldap features to break.

Circumventing this is almost impossible as the informarion entered is required to be kept as entered to allow propper usage in normal proceeding so encoding isn't possible as it would break the intended behaviour.

As that is not relevant in the usual cases (in default setups there is no super-admin and in multi-sites usualy only the super-admin has access to the ldap config) I have so far not invested time into fixing an esoteric edge case that some "security researcher" thinks is relevant when solving it most probably will break a valid use-case.

@heiglandreas
Copy link
Owner Author

Today I was informed by the plugin team that the plugin has been "temporarily withdrawn from the Plugin Directory due to a security bug".

Your plugin has had to be temporarily withdrawn from the WordPress.org Plugin Directory due to a security bug.

https://wordpress.org/plugins/authldap/

For the next 60 days, your plugin will simply say that it is no longer available for download. After that time, it will state that it was closed for a security issue.

What to Do Next

We understand this can be a shocking and painful email to receive. We do not close plugins lightly, and when it comes to security issues we attempt to balance the volume of users and the history of the developers with the severity and potential for damage of the report. We believe that leaving plugins open would put users at risk if we allowed them to download code that could be exploited, and once an exploit is reported, it is often acted upon by persons nefarious.

To help restore your plugin as quickly as possible, you are required to do the following:

    Review the report (listed below) and make corrections to prevent it from being exploitable
    Perform a full security and standards review on your own code
    Increase the plugin version
    Ensure the 'tested up to' version in your readme is the latest release of WordPress
    Update the code in SVN
    Reply to this email and request a re-review


If you believe the report is not valid, and that your plugin is secure, please reply to this email to let us know. If the vulnerability is XSS or CSRF related, know that Chrome actually prevents those from working in their browser and you may need to check in Firefox or another browser.

Should you, for any reason, find you are unable to update the plugin, please let us know promptly so we can decide on the best course of action to take in order to protect the users. It's okay if you just can't fix this or don't want to.

Plugins are closed immediately and the developer contacted when this happens, in part because we have an imperfect system of notifications. This means until your plugin is corrected to meet our guidelines, we will not reopen it.

Please review our documentation on how to use SVN - https://developer.wordpress.org/plugins/wordpress-org/how-to-use-subversion/#best-practices - as improper SVN usage can delay our reviews.

When we re-review your code we will look at not just the changes, but the entire plugin, so there may be a delay. Rest assured, we prioritize reviews of security related issues above all else.

Vulnerability Report

Plugin: https://wordpress.org/plugins/authldap/
Vulnerability type: Cross Site Scripting (XSS)
CVSS: 5.9
Prerequisite: Administrator
Vulnerability report URL: https://patchstack.com/database/report-preview/51aedebf-f013-4779-b287-445fea5c7871?pin=RNAGGQsWkHgzNtGv



This is not a full review of your plugin.

Once you've replied, we will re-scan your entire plugin, looking for both security issues and guideline violations. Should we find other issues on a re-review, you will be required to fix those before we reopen your plugin.

We require this because if we found another security issue down the road, we would have to close your plugin again. We feel it's better for your reputation to have a plugin closed once and fixed rather than multiple times. In addition, there are some less than ethical companies who will absolutely 0-day your plugin if we reopen it while you're still working on security issues.

If you have any questions, please let us know.

@heiglandreas
Copy link
Owner Author

heiglandreas commented Mar 7, 2024

The video doesn't show anything new.

When someone is able to enter XSS sruff in the configuration, they can do far worse things.

And the only place where the XSS is executed is the administration interface that can only be accessed with administrative (resp. Super-Admin in case of a multisite install) rights.

An XSS is the least of your problems in case an attacker has access to that interface.

Yes! There is a possibility to inject JavaScript! I'm not denying it.

But the impact is completely irrelevant.

This JS-injection is not a security issue. It can not he used for privilege escalation as one already has the highest privilege available to be able to add the JS-injection.

@gramakri
Copy link
Contributor

gramakri commented Mar 8, 2024

@heiglandreas right, I agree that once you are already in admin, the whole test is a bit moot :-) After all, one can do whatever.

It does highlight though that somewhere data (the input) is treated as code. Is this because of a missing escape of data somewhere? What makes the 'alert' entered in the input fields get executed? As simple input validation, maybe we should check that there are no double quotes and back slash in input. Then, we just make sure we quote all the data values.

@heiglandreas
Copy link
Owner Author

heiglandreas commented Mar 8, 2024

The data is put back into the input fields. With carefully crafted information one can prematurely end the input field and the rest is then added as HTML.

My main problem is that the input fields contain information that needs to be passed into LDAP- And that can have these weird informations. (They will then be properly escaped). So I need people to be able to enter them in the fields. And so far I was not able to always retrieve the data unescaped from the fields after adding them escaped...

It's a dilemma of having to handle non-HTML values in a HTML way with broken browsers.

And as mentioned at other places:

And I stopped investing more time into it as the "problem" only surfaces when someone has admin (or super-admin in case of multi-site) access already. Having an XSS is the least of your problems then.
(https://phpc.social/@heiglandreas/112059010679232326)

@d19dotca
Copy link
Contributor

d19dotca commented Mar 8, 2024

But without fixing it, doesn’t this mean your plugin won’t ever be able to be back in the Wordpress.org plugin repository? Not a biggie I guess since we can still pull from GitHub, but a lot less exposure in that case to new users.

@heiglandreas
Copy link
Owner Author

heiglandreas commented Mar 10, 2024

Yeah. That might happen.

The trouble is that there is nothing to "fix". As there is nothing "broken".

@heiglandreas
Copy link
Owner Author

I received a message from the WordPress plugin-team:

Hello,

This is a valid security issue, you will need to patch it before your plugin is re-enabled in the WordPress.org repository.

Admin accounts are not always permitted to post arbitrary HTML, such as when the "DISALLOW_UNFILTERED_HTML" constant is set to true on a site.

Reviewing the github discussion, it is stated correctly that it is an unlikely security concern, but this is still a security concern. Even unlikely or lower severity security concerns must be patched.



--
WordPress Plugin Review Team | plugins@wordpress.org
https://make.wordpress.org/plugins/
https://developer.wordpress.org/plugins/wordpress-org/detailed-plugin-guidelines/ 

@heiglandreas
Copy link
Owner Author

heiglandreas commented Mar 10, 2024

And my answer to them:


Am 08.03.24 um 17:43 schrieb WordPress.org Plugin Directory:
> Hello,
> 
> This is a valid security issue, you will need to patch it before your 
> plugin is re-enabled in the WordPress.org repository.
There are 2 major issues that we are having here!

A security issue is none of them!

> 
> Admin accounts are not always permitted to post arbitrary HTML, such as 
> when the "DISALLOW_UNFILTERED_HTML" constant is set to true on a site.
The Javascript that can be injected will nowhere be posted apart from the admin screen where it can be entered. 

According to the Core Handbook[1] "Users with Administrator or Editor roles are allowed to publish  unfiltered HTML in post titles, post content, and comments, and upload HTML files to the media library."

Stating the DISALLOW_UNFILTERED_HTML flag in the context of this issue shows me that it is not entirely clear WHERE JS can be injected and therefore also how this affects the security of the site.

> 
> Reviewing the github discussion, it is stated correctly that it is an 
> unlikely security concern, but this is still a security concern. Even 
> unlikely or lower severity security concerns must be patched.
The discussion also states that it "allows someone that can manage who can log into the WordPress site to also break the interface to edit those options. The first part is much more of an issue there."

When someone that can manage the *access* to roles is able to inject some JS-code to escalate their privileges, then they can much easier just asign themselves those privileges. It is much easier to use the plugins intended way of working to escalate your privileges than to use some injected JS to hope to gain that privilege.

Yes! XSS is much more than escalating ones privileges, but with the maximum escalated privileges (which is possible using the plugin as intended) one can do whatever one likes anyhow.

SO according to this argumentation the whole plugin (and any other plugin that allows role-modifications) is a security risk per se and needs to be removed from the plugin-list.

So the definition of "Security Risk" is the first problem I see here!

The second - and perhaps much more relevant - problem I see is the way of communication!

I discovered by chance that there were two CVEs published about a plugin I created without having been contacted. So there was a security issue published without having the chance to fix it before publishing. That is called a 0-Day if I am not mistaken. 

And then it takes **7 months** for the plugin team to realize that there is a security issue with a plugin and to take the plugin from the registry? And again without contacting the plugin author *beforehand*? 

Instead of the security researcher having to explain why they think it is a problem, **I** as a plugin author have to explain why I think it is **not* a problem!. 

By now I am so frustrated and - excuse my language - pissed off that I am not sure whether I actually **want** to continue working on a WordPress plugin. If that is the way the WordPress team treats volunteers I don't really want to contribute any more to this ecosystem.

**That** is the much more concerning problem I have learned from in this situation.

I might continue working on the plugin. I might even add code that removes the possibility to inject JS code in the options-page.

But I am not sure yet whether I will actually share that via SVN.

Cheers

Andreas

[1] https://make.wordpress.org/core/handbook/testing/reporting-security-vulnerabilities/#why-are-some-users-allowed-to-post-unfiltered-html

@heiglandreas
Copy link
Owner Author

I have just (again) tried using the WordPress preferred way to escape HTML-Attributes like value - esc_attr and the problem is that it correctly escapes the content of the field.

A "becomes \".

And when you store that again it becomes \\\".

And after a further storage it becomes \\\\\\\"

... and so on.

So the value that I require is modified in a way that I do not want. So now I have to apply code that reverses this. and removes backslashes. Which is not something I want to do as that backslash might actually be required in the LDAP-context that these values are used in and entered for.

@heiglandreas
Copy link
Owner Author

After some more consideration I decided that escaping single and double quotes and backslashes is fine. They should not appear unencoded in any of the provided configuration settings. In Passwords they need to be URL-encoded anyhow, when used via an env-variable the escaping is irrelevant. and in all other cases quotes and backslashes should not be needed.

So while it escalates in the amount of backslashes, it seems to be a valid way of removing the possibility to inject JS.

This might break BC in some edge-cases though. Please get into contact in those cases and we will find a solution ASAP.

heiglandreas added a commit that referenced this issue Mar 10, 2024
This should eliminate the risk of injecting JS into form field values.

Adding backslashes or quotes in any of the fields will result in a
backslash-escaped value. SHould these values be stored more than once
the amount of backslashes will exponentially grow. This is a sideeffect
of these values not being expected in the fields in the first place.

This should also fix CVE-2023-41655 as now injecting JS will no longer
result in that being executed in the UI.

For more discussion around this CVE see
#237
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants