WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#34831 closed defect (bug) (fixed)

WP oEmbed: Validate the "Secret" When Used in `document.querySelectorAll()`

Reported by: mdawaffe Owned by: mdawaffe
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: has-patch
Focuses: javascript Cc:
PR Number:

Description

In the data sent to us from the embedded iframe by postMessage(), the secret value is being used directly in a document.querySelectorAll() call without first being validated or escaped.

In theory, this could lead to some broken embeds.

Suggested hardening patch attached: There's no reason to try and escape this data correctly. Let's just reject if the secret does not conform to the format we expect.

Attachments (3)

34831.diff (972 bytes) - added by mdawaffe 4 years ago.
34831.2.diff (532 bytes) - added by mdawaffe 4 years ago.
TobiasBg's patch
34831.3.diff (767 bytes) - added by mdawaffe 4 years ago.
Compare window objects

Download all attachments as: .zip

Change History (13)

@mdawaffe
4 years ago

#1 @TobiasBg
4 years ago

How about just prepending

if ( /[^a-zA-Z0-9]/.test( data.secret ) ) { 
    return; 
}

if we return anyways? Would make for a simpler logic, IMO.

@mdawaffe
4 years ago

TobiasBg's patch

#2 @mdawaffe
4 years ago

Works for me. Updated patch: attachment:34831.2.diff

#3 @swissspidy
4 years ago

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

+1 to this enhancement and the second patch.

#4 @wonderboymusic
4 years ago

  • Keywords commit added

Looks good.

#5 follow-up: @pento
4 years ago

Is /[^a-zA-Z0-9]/.test( data.secret ) or data.secret.match( /[^a-zA-Z0-9]/ ) better?

#6 in reply to: ↑ 5 @swissspidy
4 years ago

Replying to pento:

Is /[^a-zA-Z0-9]/.test( data.secret ) or data.secret.match( /[^a-zA-Z0-9]/ ) better?

test() returns a bool, whereas match() returns all the matches. For that reason, test() is much faster.

MDN recommends using search() if you need to know if a string matches a regular expression. search() returns the index of the first match or -1.

This comparison between all those functions shows that search() and test() are both much, much faster than the other methods.

#7 @pento
4 years ago

  • Owner changed from pento to mdawaffe

Well, I'm convinced. Let's commit it.

@mdawaffe
4 years ago

Compare window objects

#8 @mdawaffe
4 years ago

  • Keywords commit removed

Another piece of hardening: We can't do the normal postMessage() origin checks (sandboxed iframes have sandboxed origins), but we can ensure that the message event's source (a window object) is the same as the iframe's window.

This protects against some potential, weird information disclosure bug with the secret. That is, with this extra check, the secret does not need to be private; it just becomes a unique ID.

Combined patch attached: attachment:34831.3.diff.

#9 @swissspidy
4 years ago

I've tested the latest patch, even by embedding from different domains as well. Works as advertised!

#10 @wonderboymusic
4 years ago

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

In 35761:

WP oEmbed: validate the secret send via postMessage in wp.receiveEmbedMessage. Also, compare window instances.

In the data sent to us from the embedded iframe by postMessage(), the secret value is being used directly in a document.querySelectorAll() call without first being validated or escaped.

In theory, this could lead to some broken embeds.

Props mdawaffe.
Fixes #34831.

Note: See TracTickets for help on using tickets.