Closed Bug 1407789 Opened 7 years ago Closed 6 years ago

Web Authentication - Prohibit cross-site iframes

Categories

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

Product:
Core
Component:
DOM: Device Interfaces
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Future
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox59 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

(
URL
)

Details

(Keywords: dev-doc-needed, Whiteboard: [webauthn][webauthn-wd07])

Attachments

(1 file)

Bug 1407789 - Prohibit cross-site iframes for Credential Management
59 bytes, text/x-review-board-request
baku
: review+
keeler
: review+
Details 
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 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 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?
Attachment #8918506 - Flags: review?(ttaubert)
Attachment #8918506 - Flags: review?(kyle) → review?(amarchesini)
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.
See Also: → 1409202
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 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.
Attachment #8918506 - Flags: review?(ttaubert) → review+
Assignee: nobody → jjones
Status: NEW → ASSIGNED
Comment on attachment 8918506 [details]
Bug 1407789 - Prohibit cross-site iframes for Credential Management

I prefer smaug to review this.
Attachment #8918506 - Flags: review?(amarchesini) → review?(bugs)
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.
Attachment #8918506 - Flags: review?(bugs) → review-
(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.
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 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 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
Attachment #8918506 - Flags: review?(dkeeler) → review+
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 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
Attachment #8918506 - Flags: review?(amarchesini) → review-
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 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);
Attachment #8918506 - Flags: review?(amarchesini) → review+
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!
Keywords: checkin-needed
Summary: Web Authentication - Restrict to top-level browsing contexts → Web Authentication - Prohibit cross-site iframes
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d67a47719c80
Prohibit cross-site iframes for Credential Management r=baku,keeler,ttaubert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d67a47719c80
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
See Also: → 1430875
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: