Opened 8 years ago
Closed 8 years ago
#34831 closed defect (bug) (fixed)
WP oEmbed: Validate the "Secret" When Used in `document.querySelectorAll()`
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.4 | Priority: | normal |
Severity: | normal | Version: | 4.4 |
Component: | Embeds | Keywords: | has-patch |
Focuses: | javascript | Cc: |
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)
Change History (13)
#2
@
8 years ago
Works for me. Updated patch: attachment:34831.2.diff
#3
@
8 years ago
- Owner set to pento
- Status changed from new to assigned
+1 to this enhancement and the second patch.
#5
follow-up:
↓ 6
@
8 years ago
Is /[^a-zA-Z0-9]/.test( data.secret )
or data.secret.match( /[^a-zA-Z0-9]/ )
better?
#6
in reply to:
↑ 5
@
8 years ago
Replying to pento:
Is
/[^a-zA-Z0-9]/.test( data.secret )
ordata.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.
#8
@
8 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.
How about just prepending
if we
return
anyways? Would make for a simpler logic, IMO.