Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support verifying a sender using messagingToken #28295

Closed
wants to merge 3 commits into from

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented May 8, 2020

Currently, WindowPortEmulator inside messaging.js compares the event.origin to its this.origin_ set by the amp-viewer-integration code:

const origin = viewer.getParam('origin') || '';

const port = new WindowPortEmulator(
this.win,
origin,

This causes issues when the parent window of the viewer does not have an origin (#28225).

This PR will allow the amp-story-player to send a messagingToken in the waitForHandshakeFromDocument method when establishing a handshake with the STAMP. Paired with the #messagingToken=storyPlayerToken parameter set on the STAMP's url, the WindowPortEmulator can then use those to verify the communication instead of comparing the origins of the window and event.

return Messaging.waitForHandshakeFromDocument(
this.win_,
iframeEl.contentWindow,
this.getEncodedLocation_(url).origin
);

Copy link
Member

@cramforce cramforce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR will need a security review.

@Enriqe
Copy link
Contributor Author

Enriqe commented May 11, 2020

@zhangsu since I know you implemented #20634 to introduce a secure way to embed a sandboxed iframe, I was wondering if you could review this PR? Hoping I'm not introducing a vulnerability of the attack described in #20634 (review) by @choumx with this PR

@Enriqe Enriqe requested a review from zhangsu May 11, 2020 14:58
@dreamofabear
Copy link

Security review should come from our ISE consult @molnarg.

if (event.origin == this.origin_ && event.source == this.target_) {
if (
(event.data.messagingToken === this.token_ ||
event.origin == this.origin_) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small suggestion: if I understand correctly, this means having a messaging token completely disables the origin checking behaviour.

Would it be possible to keep both (i.e. change || to && here)? Then you can just use 'null' as the origin parameter when you create a WindowPortEmulator when there's no origin (this is what we do for the AMP for Email viewer).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually tried passing 'null' as the origin parameter #28225 (comment), but the problem is that when the WindowPortEmulator receives a message from the viewer's parent window, event.origin actually has the origin of the parent window, making this check fail.

Copy link

@molnarg molnarg May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

token_ is null if it was not specified, so a message with messagingToken=null could bypass the origin check, if the token was not expected.

Btw, other token checking is done in the Messaging class, not sure how this interacts with that, would be better to have all this logic in a single class. Also, the Messaging class implemented this check in a better way here without the flaw mentioned above:

if (
this.token_ &&
this.verifyToken_ &&
message.messagingToken !== this.token_
) {

@molnarg
Copy link

molnarg commented May 15, 2020

See #28225 (comment), maybe we won't need this.

@Enriqe Enriqe closed this May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants