WordPress.org

Make WordPress Core

Opened 11 days ago

Closed 8 days ago

Last modified 7 days 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: trunk
Component: Privacy Keywords: gdpr fixed-major commit dev-reviewed
Focuses: Cc:

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 11 days ago.

Download all attachments as: .zip

Change History (12)

@birgire
11 days ago

#1 @birgire
11 days ago

  • Keywords has-patch added

44054.diff adds escaping as described above.

#2 @iandunn
11 days 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
11 days 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
11 days 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
11 days 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
9 days ago

  • Keywords dev-feedback added

#7 @azaozz
9 days ago

  • Keywords commit added

+1 for merging to 4.9.6.

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


9 days ago

#9 @iandunn
9 days ago

  • Keywords dev-reviewed added; dev-feedback removed

#10 @azaozz
8 days 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
7 days ago

  • Component changed from Administration to Privacy

Moving to the new Privacy component.

Note: See TracTickets for help on using tickets.