WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 3 years ago

#21523 closed enhancement (fixed)

Add additional escaping to credit.php

Reported by: Viper007Bond Owned by: chriscct7
Milestone: 4.3 Priority: normal
Severity: normal Version: 3.4
Component: Help/About Keywords: needs-refresh
Focuses: Cc:

Description

/wp-admin/credits.php doesn't fully escape all of the data that it displays. It should be properly escaped like any other third party data.

What if WordPress.org were somehow compromised?

Attachments (8)

21523.patch (2.4 KB) - added by Viper007Bond 6 years ago.
21523.2.patch (4.2 KB) - added by gtuk 3 years ago.
21523.3.patch (5.2 KB) - added by chriscct7 3 years ago.
Improves upon second patch by adding more strict checking on API return
21523.4.patch (5.5 KB) - added by chriscct7 3 years ago.
We should make sure the user objects are populated before using them
21523.5.patch (4.3 KB) - added by obenland 3 years ago.
21523.6.patch (3.5 KB) - added by obenland 3 years ago.
21523.7.patch (2.5 KB) - added by obenland 3 years ago.
21523.8.patch (2.4 KB) - added by obenland 3 years ago.

Download all attachments as: .zip

Change History (30)

@Viper007Bond
6 years ago

#1 @ryan
6 years ago

  • Milestone changed from Awaiting Review to 3.5

#2 @nacin
6 years ago

esc_url()'s seem fine. We send back entities for some names, though, and I would want to make sure that we aren't stomping any future solution to solve encoding issues — #17487.

If wordpress.org were somehow compromised, I feel like XSS on the credits page would be our least concern.

#3 @nacin
6 years ago

  • Component changed from Security to Help/About

#4 @nacin
6 years ago

  • Milestone changed from 3.5 to Future Release

#5 follow-up: @chriscct7
3 years ago

  • Keywords needs-refresh good-first-bug added; has-patch removed
  • Version changed from 3.4.1 to 3.4

While malicious use of other parts of WordPress.org could do more damage, they are significantly harder to use without explicit knowledge of how the .org architecture works. In light of the jQuery.com hack, this is pretty low hanging to do. Patch needs refresh. Good first bug though.

#6 in reply to: ↑ 5 @protechig
3 years ago

Replying to chriscct7:

While malicious use of other parts of WordPress.org could do more damage, they are significantly harder to use without explicit knowledge of how the .org architecture works. In light of the jQuery.com hack, this is pretty low hanging to do. Patch needs refresh. Good first bug though.

I would love to try to tackle this bug. It would be my first patch, I do want to get started contributing to WP. Would you be kind enough to point me in the right direction (an article) on the proper way to satanize this data, or even an example in another WP file that I can reference?

#7 follow-ups: @chriscct7
3 years ago

@protechig see the patch on this ticket, which needs to be refreshed. That covers most of the sanitization that would need to be done. See https://codex.wordpress.org/Data_Validation & https://codex.wordpress.org/Validating_Sanitizing_and_Escaping_User_Data for references

#8 in reply to: ↑ 7 @protechig
3 years ago

Replying to chriscct7:

@protechig see the patch on this ticket, which needs to be refreshed. That covers most of the sanitization that would need to be done. See https://codex.wordpress.org/Data_Validation & https://codex.wordpress.org/Validating_Sanitizing_and_Escaping_User_Data for references

Thanks for the swift reply! I'll go through those articles and update the patch. I took a look at this before I asked the question, these changes should be pretty straightforward.

@gtuk
3 years ago

#9 in reply to: ↑ 7 @gtuk
3 years ago

Replying to chriscct7:

@protechig see the patch on this ticket, which needs to be refreshed. That covers most of the sanitization that would need to be done. See https://codex.wordpress.org/Data_Validation & https://codex.wordpress.org/Validating_Sanitizing_and_Escaping_User_Data for references

I updated the file as you suggested. Hope everything is as expected, otherwise let me know

#10 @gtuk
3 years ago

  • Keywords has-patch added

#11 follow-up: @DrewAPicture
3 years ago

  • Owner set to gtuk
  • Status changed from new to assigned

#12 in reply to: ↑ 11 ; follow-up: @gtuk
3 years ago

Replying to DrewAPicture:

What is the next part of the workflow if i'm the owner of this ticket now?

#13 in reply to: ↑ 12 @DrewAPicture
3 years ago

  • Keywords dev-feedback added; needs-refresh removed

Replying to gtuk:

Replying to DrewAPicture:

What is the next part of the workflow if i'm the owner of this ticket now?

Assigning the ticket to you just has the effect of moving the ticket off of the "Unclaimed" list on the good-first-bug report.

At this point, you'd wait for developer feedback on your patch.

#14 @chriscct7
3 years ago

  • Keywords good-first-bug removed
  • Owner changed from gtuk to chriscct7
  • Status changed from assigned to reviewing

Will review this weekend, unless someone else gets to it first.

Early Review Re-Assignment: Open

@chriscct7
3 years ago

Improves upon second patch by adding more strict checking on API return

#15 @chriscct7
3 years ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Future Release to 4.3
  • Status changed from reviewing to accepted

Looks good. Added some strict checking on the API return, so if wp.org's api ever sent back an incomplete response we don't save that and then subsequently try to use that bad result.

@chriscct7
3 years ago

We should make sure the user objects are populated before using them

#16 @obenland
3 years ago

  • Keywords commit removed

Let's keep this to escaping. If the API result is screwed up in a way where properties are missing, it should spit out errors like that.

#17 @chriscct7
3 years ago

  • Keywords commit added

@obenland in that case use patch 2

#18 @obenland
3 years ago

esc_html( sprintf( __( 'WordPress is created by a <a href="%1$s">worldwide team</a> of passionate individuals. <a href="%2$s">Get involved in WordPress</a>.' )… will not work since it contains html.

#19 @obenland
3 years ago

Could you refresh the patch and create it from root (rather than /trunk/)?

#20 @chriscct7
3 years ago

  • Keywords needs-refresh added; has-patch commit removed

Yeah. Good catch re-esc_html. Didn't even notice that section was missing somehow

@obenland
3 years ago

@obenland
3 years ago

@obenland
3 years ago

@obenland
3 years ago

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


3 years ago

#22 @obenland
3 years ago

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

In 33032:

Add additional escaping to credits page.

Props Viper007Bond, gtuk for initial patch.
Fixes #21523.

Note: See TracTickets for help on using tickets.