WordPress.org

Make WordPress Core

Opened 6 weeks ago

Last modified 53 minutes ago

#49956 accepted defect (bug)

Spammers able to share unmoderated comments

Reported by: jonkolbert Owned by: peterwilsoncc
Milestone: 5.4.2 Priority: normal
Severity: major Version: 5.1
Component: Comments Keywords: has-patch needs-dev-note
Focuses: Cc:

Description

Hello,

I am a volunteer contributor on Wikimedia projects, and we often see spam from WordPress blogs running 5.4 that are using "unapproved" and "moderation-hash" in the query string to effectively "unblock" their comments for the purpose of spamming on other websites.

example here

This should not be occuring, approved comments should be the only ones that shown publicly and site admins/moderators should be the only ones able to see unmoderated comments.

Attachments (5)

49956-1.patch (669 bytes) - added by ayeshrajans 5 weeks ago.
49956.2.patch (6.7 KB) - added by imath 4 weeks ago.
49956.3.diff (5.0 KB) - added by audrasjb 2 weeks ago.
Comments: Remove unapproved comments preview after 1 minute to avoid public access through the moderation hash
49956.diff (7.9 KB) - added by peterwilsoncc 4 days ago.
49956.2.diff (7.9 KB) - added by peterwilsoncc 27 hours ago.

Download all attachments as: .zip

Change History (39)

#1 @SergeyBiryukov
5 weeks ago

  • Component changed from General to Comments
  • Version changed from 5.4 to 5.1

Hi there, welcome to WordPress Trac! Thanks for the report.

Introduced in [44659] / #43857.

#2 @ayeshrajans
5 weeks ago

  • Severity changed from normal to major

Welcome to WordPress trac @jonkolbert.
This is pretty interesting issue, and this certainly looks like a clever way to spam any WordPress site!

I suppose the fix would be two-fold:

  • When a comment is placed by a user/spammer, we need to verify it is the same user that placed the comment. A cookie is placed in the users browser (it's OK if it's the same cookie throughout the session). When the same user opens the comment URL of an un-approved comment, the cookie must be present too.
  • For admins, we don't check the cookie, but check a different hash value.

I'd be happy to work on a prototype patch if others agree that this approach is ideal.

Marking severity=major because it can circumvent the moderation queue in a way that benefits spammers.

#3 @johnbillion
5 weeks ago

  • Milestone changed from Awaiting Review to 5.4.1

#4 @whyisjake
5 weeks ago

  • Milestone changed from 5.4.1 to 5.4.2

We are coming up on the RC1 candidate for 5.4.1 tomorrow, I am going to bump this to 5.4.2 as we don't have a patch or any testing in at this point.

#5 @Asif2BD
5 weeks ago

  • Keywords needs-patch added

I could confirm this is happening in the current version as well. This needs to be patched very soon.

#6 @ayeshrajans
5 weeks ago

I will be working on a patch. However, this will be a breaking change in a semantic sense, although we are in reality fixing a security (-ish?) issue.

#7 @ayeshrajans
5 weeks ago

It turns out that WordPress is hesitant on setting any cookies on sites without explicit consent, so this leaves cookies from the user verification. Here is a very simple approach of validating the user has same IP address as the IP address the comment was original left at. This should take away the incentive for spammer because outside users will not see this comment even though the exact same URL is used.

This is a very short-sighted fix, and I do not recommend we go down this road:

  1. It uses $_SERVER['REMOTE_ADDR'], which is not concrete. Reverse proxies sometimes do not forward the user IP and this IP needs to be taken into account.
  1. If the WordPress site is caching static responses, or is behind a load-balancer that caches responses, everyone with the unique URL will get the comment even though they come from different IP addresses.

I think the security issue the OP mentioned is a serious one that can affect majority of the WordPress sites out there. I think our step would be to expose an admin configuration option to disable the URL-scoped comment preview feature. I know I will immediately disable this feature instead of dealing with fragile scoping mechanisms.

Alternately, the comment preview can be enabled only for those who consent to cookies, and the comment preview emits a Vary: cookie header to bust load-balancer/proxy caching.

@ayeshrajans
5 weeks ago

#8 @peterwilsoncc
5 weeks ago

@ayeshrajans Thanks for the patch.

As you mention, obtaining the remote address can be difficult for sites using a CDN/reverse proxy as they're not included in the request to the app server to improve caching.

How about this for an approach?

  • The moderation is valid once only, ie deleted once the commenter is redirected to the page.
  • If not already the case, when the query string includes unapproved and/or moderation-hash the nocache headers are included in the page - refer to the `nocache_headers()` function
  • That the hash is valid could be indicated by the existence of some comment meta data, it would only be added if a commenter denies the cookie request and deleted upon display -- this will help keep the comment meta table clean.

@johnbillion Could I get a second opinion from you on the above approach -- I'm in two minds about changing meta data on a front end page view but certainly agree this needs to be fixed.

If it's all to much, I think I'd prefer the approach of you must have a cookie to preview...

#9 @ayeshrajans
4 weeks ago

Thank you @peterwilsoncc - a single-user token certainly sounds like a much cleaner approach.

nocache_headers() certainly seems to do the trick with proxies/CDNs.

The root problem is with HTTP's stateless nature, unless we show the comment as the direct response to an HTTP POST method, we cannot determine if the user indeed is the one who submitted the comment if we don't use a cookies, local storage, IP address, etc.

If we were to use a single-use token, an attacker can submit a comment with the POST request, and get the redirect URL with the single-use token from the Location header. Attacker can decide to not open this URL by themselves, and instead post this in a spam farm. A search engine bot might come across this, and because the token is not used yet, attacker still can get a positive attack because it will be the search engine bot who sees the single-use comment preview now. Although far-fetched, it is a technical possibility.

We can impose a time-limit of a few seconds that can practically make this attack useless. At this point, just sending the no-cache headers (without no token use) would be enough to prevent proxy/CDN poisoning.

@imath
4 weeks ago

#10 @imath
4 weeks ago

  • Keywords has-patch added; needs-patch removed

Hi @jonkolbert

Thanks a lot for your feedback about this sensitive issue. If I believe it's important the commenter gets a feedback once he posted a comment even if he hasn't consent to the comment cookie, I totally understand your concern about the potential wrong usage of the moderation hash spammers are doing.

In 49956.2.patch I'm suggesting to display the pending comment for 1 minute just after it has been posted and I suggest to remove all links from the comment's preview and from the author url.

After 1 minute, it's not possible to see the pending comment anymore.

Last edited 4 weeks ago by imath (previous) (diff)

@audrasjb
2 weeks ago

Comments: Remove unapproved comments preview after 1 minute to avoid public access through the moderation hash

#11 @audrasjb
2 weeks ago

  • Keywords needs-dev-note added

Hi there,

@imath ’s approach makes sense to me. It solves the issue and we still provide relevant feedback to commenters 👌

In 49956.3.diff, I removed the Twenty Nineteen part of the previous patch, and I refreshed it against trunk. I also added a couple of tiny coding standard fixes.

I tested the patch, and it works like a charm on my side: the comment preview is not accessible anymore after one minute.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 weeks ago

@peterwilsoncc
4 days ago

#14 follow-up: @peterwilsoncc
4 days ago

In 49956.diff:

  • refreshed against trunk (minor formatting change were causing it not to apply)
  • only pass querystring parameters if the user didn't consent to cookies, otherwise they are not required
  • for requests with the unapproved and moderation hash querystring parameters, I've added an expires header of 60 seconds for CDN and other cache hinting.
  • only display the requested comment if the querystring parameters are included in the request. In two minds on this one, as it can be problematic if the user replies to their own unmoderated comment. The new comment will display as the walker skips comments without parents.

@audrasjb @whyisjake Are you able to take a look over the changes and share your views, especially on the last item. There is a GitHub pull request you can comment on if it's easier.

Note: a seperate patch will need to be generated for the backport as it won't apply cleanly due to some code reformatting near the changes.

#15 in reply to: ↑ 14 @audrasjb
4 days ago

Thanks for the refresh and the various enhancements, it looks good to me.

Replying to peterwilsoncc:

  • only display the requested comment if the querystring parameters are included in the request. In two minds on this one, as it can be problematic if the user replies to their own unmoderated comment. The new comment will display as the walker skips comments without parents.

Good catch, worth handling this edge case 👍

#16 @ayeshrajans
4 days ago

I added some comments to the GitHub PR.

#17 @peterwilsoncc
4 days ago

  • Owner set to peterwilsoncc
  • Status changed from new to accepted

PR updated, I'll await any further feedback before regenerating the patch.

Thanks @ayeshrajans

#18 @whyisjake
42 hours ago

Changes look great @peterwilsoncc and @audrasjb. Getting some more eyes on this and let's get this shipped.

#19 follow-up: @peterwilsoncc
37 hours ago

I'm in two minds, I am wondering if this is a serious enough issue to back-port all the way to 5.1?

It doesn't require a security release, but as it allows spammers to share unapproved comments it's certainly in the realm.

What do y'all think?

#20 in reply to: ↑ 19 @jonkolbert
36 hours ago

Replying to peterwilsoncc:

I'm in two minds, I am wondering if this is a serious enough issue to back-port all the way to 5.1?

It doesn't require a security release, but as it allows spammers to share unapproved comments it's certainly in the realm.

What do y'all think?

I would be strongly in favour of this, having witnessed how widespread this is being exploited.

#21 @ayeshrajans
36 hours ago

I also think a backport would make more sense.

A quick Google search shows about 700K results with "moderation-hash" in the URL. These are indexed URLs that should not have made it there.

The patch is relatively simple so I'm sure it's not that extra work to backport to other versions too. I'd be happy to work on patches if the current one doesn't apply cleanly.

#22 @prbot
36 hours ago

Ayesh commented on PR #291:

Hi @peterwilsoncc - can we take a look at the robots tag again please?

It looks like Google has indeed indexed URLs containing "moderation-hash", and if we were to backport this to earlier versions, having a more strict robots tag would help to eventually de-index those URLs.

#23 @peterwilsoncc
36 hours ago

Replying to ayeshrajans:

A quick Google search shows about 700K results with "moderation-hash" in the URL. These are indexed URLs that should not have made it there.

For reference: https://www.google.com/search?q=inurl%3Amoderation-hash+inurl%3Aunapproved

#24 @prbot
36 hours ago

peterwilsoncc commented on PR #291:

Hi @peterwilsoncc - can we take a look at the robots tag again please?

It looks like Google has indeed indexed URLs containing "moderation-hash", and if we were to backport this to earlier versions, having a more strict robots tag would help to eventually de-index those URLs.

@jono-alderson are you able to help out with @Ayesh's question?

#25 @whyisjake
32 hours ago

+1 for moving this to 5.1.

#26 @audrasjb
29 hours ago

For what its worth, +1 to get this backported to 5.1 and more.

#27 @prbot
28 hours ago

jono-alderson commented on PR #291:

Assuming there's a valid canonical URL tag in place, and that the site/page doesn't suffer from any significant SEO issues, then Google shouldn't index the variant version.

I'm still nervous about adding robots controls (specifically, a noindex, follow directive) to the page because:

  • WordPress core doesn't do a good job of reconciling the relationship between canonical tags and meta robots tags; and having a noindex _and_ a canonical can cause problems. Without logic to handle this, I'd be nervous about making a bigger mess.
  • Legitimate (non-abusive) links which contain these types of parameters might be shared or linked to by users, in which case the page _shouldn't_ be noindex'd.

#28 follow-up: @peterwilsoncc
27 hours ago

49956.2.diff includes feedback on the pull request.

  • Cache-control and Expires HTTP headers used in response for unapproved comments.

I realised I made a key typo on my previous comment around ensuring only single requested comment is displayed...

If a user posts a reply to their own unmoderated comment, neither the parent or new comment will display. The new comment is considered an orphan and the comment walker discards it. I don't know of a good fix for this, suggestions are welcome.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


16 hours ago

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


14 hours ago

#31 in reply to: ↑ 28 @peterwilsoncc
13 hours ago

Replying to peterwilsoncc:

If a user posts a reply to their own unmoderated comment, neither the parent or new comment will display. The new comment is considered an orphan and the comment walker discards it. I don't know of a good fix for this, suggestions are welcome.

Night brain come up with a solution: hide the reply button on unapproved comment if it's displayed due to the moderation hash.

Replying to an unmoderated comment risks a comment that can never be displayed (reply is approved, parent is not) so it's arguable that it should never be displayed but that's beyond the scope of this ticket.

#32 follow-up: @whyisjake
6 hours ago

Night brain come up with a solution: hide the reply button on unapproved comment if it's displayed due to the moderation hash.

Do you think that spammers are submitting these comments manually, or perhaps programmatically? I'm wondering if it makes sense to remove the button at all.

#33 in reply to: ↑ 32 ; follow-up: @jonkolbert
6 hours ago

Replying to whyisjake:

Night brain come up with a solution: hide the reply button on unapproved comment if it's displayed due to the moderation hash.

Do you think that spammers are submitting these comments manually, or perhaps programmatically? I'm wondering if it makes sense to remove the button at all.

This is almost certainly automated.

I believe removing the reply button was intended for those legitimate users who may be trying to reply to their own comment, not to do with the spammers.

#34 in reply to: ↑ 33 @peterwilsoncc
53 minutes ago

Replying to jonkolbert:

I believe removing the reply button was intended for those legitimate users who may be trying to reply to their own comment, not to do with the spammers.

That's correct, the intention is not to confuse legitimate users.

PR updated, I will upload a patch to this ticket in about 12 hours.

Note: See TracTickets for help on using tickets.