1407789 - Web Authentication - Prohibit cross-site iframes

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, enhancement, P1)

Description

[J.C. Jones [:jcj] (he/they)]

Credential Management does not permit use of navigator.credentials from within iframes -- it only permits usage from top-level browsing contexts.

We should prohibit (for now).

Comment 1

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1407789 - Prohibit cross-site iframes for Credential Management

Credential Management defines that if the responsible document is not the active
document in a top-level browsing context, then operations should return
NotSupportedError [1][2]. However, we have to also allow iframes from the
same origin for Mochitests, and it's just a reasonable expansion that's been
discussed in-person a number of times.

This ensures that

  https://webauthn.bin.coffee/iframe.html

works, but the cross-origin

  https://u2f.bin.coffee/iframe-webauthn.html

does not.

qdot: Please check my logic for IsNotInAFrameOrIsSameOrigin - it appears to
work, but I'm no DOM expert!

[1] https://w3c.github.io/webappsec-credential-management/#algorithm-request
[2] https://w3c.github.io/webappsec-credential-management/#algorithm-create

Review commit: https://reviewboard.mozilla.org/r/189002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189002/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

https://reviewboard.mozilla.org/r/189002/#review194630


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/credentialmanagement/CredentialsContainer.cpp:58
(Diff revision 1)
> +    return true;
> +  }
> +
> +  // We're in some kind of frame, so let's get the parent and start checking
> +  // the same origin policy
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aParent);

Error: Unused "kungfudeathgrip" 'nscomptr<nsiglobalobject>' objects constructed from temporary values are prohibited [clang-tidy: mozilla-kungfu-death-grip]

Comment 3

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189002/diff/1-2/

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

https://reviewboard.mozilla.org/r/189002/#review194816

::: dom/credentialmanagement/CredentialsContainer.cpp:34
(Diff revision 2)
> +    return nullptr;
> +  }
> +
> +  ErrorResult rv;
> +  RefPtr<Promise> promise = Promise::Create(global, rv);
> +  if(NS_WARN_IF(rv.Failed())) {

nit: missing space after if

::: dom/credentialmanagement/CredentialsContainer.cpp:42
(Diff revision 2)
> +bool
> +IsNotInAFrameOrIsSameOrigin(nsPIDOMWindowInner* aParent)

The spec says we should disallow any iframe (same or cross-origin) and we have that only that make tests work... This seems like a thing we could regret later.

Maybe we should write a test runner for `dom/{u2f,webauthn}` tests that uses `window.open()` to run these in a separate window?

::: dom/credentialmanagement/CredentialsContainer.cpp:97
(Diff revision 2)
> +  if (!IsNotInAFrameOrIsSameOrigin(mParent)) {
> +    return CreateAndReject(mParent, NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +  }

Do we have a bug yet for the "active document" part? Do we currently allow background tabs to call these?

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

This one is out of my wheelhouse too, I'm not familiar enough with iframe/domain checking to feel I'd do a good job on this review. Passing to baku, who I think might be a bit better suited for it.

Comment 6

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

https://reviewboard.mozilla.org/r/189002/#review194816

Thanks for the review!

> The spec says we should disallow any iframe (same or cross-origin) and we have that only that make tests work... This seems like a thing we could regret later.
> 
> Maybe we should write a test runner for `dom/{u2f,webauthn}` tests that uses `window.open()` to run these in a separate window?

Possibly, I've started an email thread with the Chrome folks and included you; I agree we shouldn't land this until se have more of a plan.

> Do we have a bug yet for the "active document" part? Do we currently allow background tabs to call these?

Opened bug 1409202

Comment 7

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189002/diff/2-3/

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

https://reviewboard.mozilla.org/r/189002/#review195368

Code LGTM. r=me if we have agreement with Chrome about same-origin iframes.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

I prefer smaug to review this.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

https://reviewboard.mozilla.org/r/189002/#review196720

::: dom/credentialmanagement/CredentialsContainer.cpp:56
(Diff revision 3)
> +  if (aParent->IsTopInnerWindow()) {
> +    // Not in a frame or iframe
> +    return true;
> +  }
> +
> +  // We're in some kind of frame, so let's get the parent and start checking

This is against the spec, so we shouldn't add this.
Either get the spec changed to allow this, and remove this code.

If we just need this for mochitests, do what is done with other similar cases, just open a new window and run tests there.

Comment 11

[J.C. Jones [:jcj] (he/they)]

(In reply to Olli Pettay [:smaug] (work week Oct 16-20) from comment #10)
> This is against the spec, so we shouldn't add this.
> Either get the spec changed to allow this, and remove this code.

Agreed; there appears to be more of a discussion going on than I was led to believe from the last face-to-face.

Sorry for the false start, but thanks for taking a look.

Comment 12

[J.C. Jones [:jcj] (he/they)]

Results from TPAC:

We're going to permit iframes (due in part to 3-D Secure requirements).

The change to Credential Management to permit WebAuthn to permit iframes is https://github.com/w3c/webappsec-credential-management/issues/113

The change to WebAuthn is here https://github.com/w3c/webauthn/issues/374

Lets not close this yet as we might need to interact with Feature Policy https://wicg.github.io/feature-policy/

Comment 13

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189002/diff/3-4/

Comment 14

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

https://reviewboard.mozilla.org/r/189002/#review196720

> This is against the spec, so we shouldn't add this.
> Either get the spec changed to allow this, and remove this code.
> 
> If we just need this for mochitests, do what is done with other similar cases, just open a new window and run tests there.

Spec updated. :) Thanks!

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

https://reviewboard.mozilla.org/r/189002/#review213938

LGTM.

::: commit-message-98247:23
(Diff revision 4)
> +
> +  https://u2f.bin.coffee/iframe-webauthn.html
> +
> +does not.
> +
> +baku: Please check my logic for IsNotInAFrameOrIsSameOrigin - it appears to

I would just be sure to remove this comment in the final commit.

::: dom/credentialmanagement/CredentialsContainer.cpp:43
(Diff revision 4)
> +  promise->MaybeReject(aResult);
> +  return promise.forget();
> +}
> +
> +bool
> +IsNotInAFrameOrIsSameOrigin(nsPIDOMWindowInner* aParent)

Why not just call this `IsSameOriginWithAncestors`? (which would alleviate the "not is not in a frame..." logic puzzles when we call it...)

::: dom/credentialmanagement/CredentialsContainer.cpp:108
(Diff revision 4)
>  
>  already_AddRefed<Promise>
>  CredentialsContainer::Get(const CredentialRequestOptions& aOptions)
>  {
> +  if (!IsNotInAFrameOrIsSameOrigin(mParent)) {
> +    return CreateAndReject(mParent, NS_ERROR_DOM_NOT_SUPPORTED_ERR);

Should this be some sort of 'not allowed' error?

::: dom/credentialmanagement/tests/mochitest/frame_credman_iframes.html:5
(Diff revision 4)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<html>
> +<head>
> +  <title>Embedded Frame for Credential Management: Prohibit use in iframes</title>

... but only non-same-origin frames, right?

::: dom/credentialmanagement/tests/mochitest/frame_credman_iframes.html:58
(Diff revision 4)
> +  navigator.credentials.create({publicKey: makeCredentialOptions})
> +  .then(function crossOriginThen(aBad) {
> +    local_ok(false, "Should not have succeeded " + aBad);
> +  })
> +  .catch(function crossOriginCatch(aResult) {
> +    local_ok(aResult.toString().startsWith("NotSupportedError"), "Expecting a NotSupportedError, received " + aResult);

nit: this line is a bit long

Comment 16

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189002/diff/4-5/

Comment 17

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

https://reviewboard.mozilla.org/r/189002/#review213938

> Why not just call this `IsSameOriginWithAncestors`? (which would alleviate the "not is not in a frame..." logic puzzles when we call it...)

Alright.

> Should this be some sort of 'not allowed' error?

Oops, yes. Thanks.

Comment 18

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189002/diff/5-6/

Comment 19

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189002/diff/6-7/

Comment 20

[Andrea Marchesini [:baku]]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

https://reviewboard.mozilla.org/r/189002/#review195094

::: dom/credentialmanagement/CredentialsContainer.cpp:37
(Diff revision 2)
> +  ErrorResult rv;
> +  RefPtr<Promise> promise = Promise::Create(global, rv);
> +  if(NS_WARN_IF(rv.Failed())) {
> +    return nullptr;
> +  }
> +

MOZ_ASSERT(NS_FAILED(aResult))

::: dom/credentialmanagement/CredentialsContainer.cpp:105
(Diff revision 7)
>  {
>    return CredentialsContainerBinding::Wrap(aCx, this, aGivenProto);
>  }
>  
>  already_AddRefed<Promise>
>  CredentialsContainer::Get(const CredentialRequestOptions& aOptions)

CreateAndReject() can fail. You must mark this method as [Throw] and set the ErrorResult in case.

::: dom/credentialmanagement/tests/mochitest/frame_credman_iframes.html:2
(Diff revision 7)
> +<!DOCTYPE html>
> +<meta charset=utf-8>

move this into <head>

::: dom/credentialmanagement/tests/mochitest/test_credman_iframes.html:2
(Diff revision 7)
> +<!DOCTYPE html>
> +<meta charset=utf-8>

same here

Comment 21

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

https://reviewboard.mozilla.org/r/189002/#review195094

Thanks Baku! Updates in-progress.

> CreateAndReject() can fail. You must mark this method as [Throw] and set the ErrorResult in case.

Makes sense; this is cleaner, too. Thanks!

> move this into <head>

Oops, that's embarassing. Thanks!

Comment 22

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189002/diff/7-8/

Comment 23

[Andrea Marchesini [:baku]]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

https://reviewboard.mozilla.org/r/189002/#review216780

Apply this change, please, before landing.

::: dom/credentialmanagement/CredentialsContainer.cpp:25
(Diff revision 8)
>    NS_INTERFACE_MAP_ENTRY(nsISupports)
>  NS_INTERFACE_MAP_END
>  
> +already_AddRefed<Promise>
> +CreateAndReject(nsPIDOMWindowInner* aParent, ErrorResult& aRv,
> +                const nsresult& aResult)

You always reject with NS_ERROR_DOM_NOT_ALLOWED_ERR. You don't really need to pass aResult.

Call directly: romise->MaybeReject(NS_ERROR_DOM_NOT_ALLOWED_ERR);

Comment 24

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

https://reviewboard.mozilla.org/r/189002/#review216780

Will do, thanks again!

Comment 25

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189002/diff/8-9/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d67a47719c80
Prohibit cross-site iframes for Credential Management r=baku,keeler,ttaubert

Comment 28

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d67a47719c80