WordPress.org

Make WordPress Core

Opened 17 months ago

Closed 17 months ago

Last modified 17 months ago

#44054 closed defect (bug) (fixed)

Privacy: Escape the comment link output in the wp_comments_personal_data_exporter() function.

Reported by: birgire Owned by: iandunn
Milestone: 4.9.6 Priority: normal
Severity: normal Version: 5.1
Component: Privacy Keywords: gdpr fixed-major commit dev-reviewed
Focuses: Cc:
PR Number:

Description

In the wp_comments_personal_data_exporter() function we have (src):

$value = get_comment_link( $comment->comment_ID );
$value = '<a href="' . $value . '" target="_blank" rel="noreferrer noopener">' . $value . '</a>';

but it looks like the escaping is missing here.

Also the output of get_comment_link() is filterable and not escaped.

We could consider instead:

$value = get_comment_link( $comment->comment_ID );
$value = sprintf( 
    '<a href="%1$s" target="_blank" rel="noreferrer noopener">%2$s</a>',
    esc_url( $value ),
    esc_html( $value )
);

Attachments (1)

44054.diff (631 bytes) - added by birgire 17 months ago.

Download all attachments as: .zip

Change History (12)

@birgire
17 months ago

#1 @birgire
17 months ago

  • Keywords has-patch added

44054.diff adds escaping as described above.

#2 @iandunn
17 months ago

  • Component changed from General to Administration
  • Milestone changed from Awaiting Review to 4.9.6
  • Owner set to iandunn
  • Status changed from new to accepted

Thanks for catching that!

At first glance, I don't see any way for an attacker to introduce malicious input to that URL (unless a plugin is filtering it and introduces some). Even if there were a way, the malicious script would execute in the context of localhost on the target's computer, rather than the site it was exported from, which I think would mitigate a lot of the damage it could do.

We should definitely still fix it, though.

In the future, it's best to report any security issues to HackerOne, even if they're only present in trunk or a beta/RC. There are some high-profile sites that run trunk or the latest branch in production.

#3 @iandunn
17 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 43245:

Privacy: Escape comment URLs in personal export file to prevent XSS.

There doesn't appear to be any way for an attacker to introduce malicious input into the URL, unless a plugin is filtering the URL to add it, but it's better to be safe than sorry.

Props birgire.
Fixes #44054.

#4 @iandunn
17 months ago

  • Keywords fixed-major added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.9 backport consideration.

#5 @birgire
17 months ago

@iandunn thanks for the heads-up, I actually didn't consider this a real security issue when I posted, but I will keep that in mind.

#6 @desrosj
17 months ago

  • Keywords dev-feedback added

#7 @azaozz
17 months ago

  • Keywords commit added

+1 for merging to 4.9.6.

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


17 months ago

#9 @iandunn
17 months ago

  • Keywords dev-reviewed added; dev-feedback removed

#10 @azaozz
17 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 43270:

Privacy: Escape comment URLs in personal export file to prevent XSS.

There doesn't appear to be any way for an attacker to introduce malicious input into the URL, unless a plugin is filtering the URL to add it, but it's better to be safe than sorry.

Props birgire.
Merges [43245] to the 4.9 branch.
Fixes #44054.

#11 @desrosj
17 months ago

  • Component changed from Administration to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.