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

Authenticator selection extension needs to define snapshotting behavior #294

Closed
bzbarsky opened this issue Nov 4, 2016 · 12 comments
Closed

Comments

@bzbarsky
Copy link

bzbarsky commented Nov 4, 2016

Presumably it snapshots the given BufferSources. When does this happen? This needs to be defined, because other script can run during makeCredential, and modify the buffers; the only way to get interop here is to clearly define when the snapshotting happens, if it happens.


update 23-Jul-2018: @equalsJeffH added the [technical] label because given @bzbarsky's clarification above in #294 (comment), it's pretty obvious it's technical given the phrase "this needs to be an explicit algorithm step".

@nadalin
Copy link
Contributor

nadalin commented Sep 6, 2017

Clearly the developer has done something wrong if they edit buffersource in place after makeCredential and we shouldn’t care about interoperability in this scenario so just document

@bzbarsky
Copy link
Author

bzbarsky commented Sep 7, 2017

and we shouldn’t care about interoperability in this scenario

That is unacceptable. The behavior needs to be defined so that it can be interoperably implemented, because otherwise developers will in fact do this and come to depend on UA behaviors.

@nadalin nadalin modified the milestones: PR, CR Sep 11, 2017
@equalsJeffH
Copy link
Contributor

@bzbarsky

snapshots the given BufferSources

A couple of questions if you please:

  1. does https://html.spec.whatwg.org/multipage/structured-data.html#performing-structured-clones-from-other-specifications provide the tools for what you term "snapshot(ing)" ? If not, where might we find such?

  2. Is this issue specific to Authenticator selection extension or does it also apply to all values of type BufferSource in webauthn?

thx.

@bzbarsky
Copy link
Author

Sorry for the lag here...

What I term "snapshotting" is https://heycam.github.io/webidl/#dfn-get-buffer-source-copy and its behavior is defined at https://heycam.github.io/webidl/#ref-for-dfn-get-buffer-source-copy%E2%91%A0

Like the text at that first link says (though you have to scroll up to start of paragraph to read it):

To inspect or manipulate the bytes inside the buffer, specification prose must first either get a reference to the bytes held by the buffer source or get a copy of the bytes held by the buffer source.

and this needs to be an explicit algorithm step that happens at some point before the bytes can be used for anything. This applies to all BufferSource uses.

As for the other uses in webauthn.... Taking as an example authenticatorMakeCredential, it walks the list of PublicKeyCredentialDescriptor it has and for each one does user interaction... before returning? Or is all this running in parallel? I think this is all running in parallel.... If so, then it probably can't access a BufferSource directly and needs to have the bytes in some parallel-friendly form instead.

@equalsJeffH
Copy link
Contributor

I added the [technical] label because given @bzbarsky's clarification above in #294 (comment), it's pretty obvious it's technical given the phrase "this needs to be an explicit algorithm step".

@emlun
Copy link
Member

emlun commented Aug 7, 2018

@akshayku @agl Can you comment on how your implementations behave in regard to this?

@equalsJeffH
Copy link
Contributor

Ok, so we're thinking that in order to navigate the process shoals with our spec boat here, we can add non-normative notes to the algs that say to make copies of buffersources in implementation-appropriate fashion before going async. @jcjones is following up on this.

@jcjones
Copy link
Contributor

jcjones commented Sep 19, 2018

Potential non-normative language. I propose adding this note to

  • 5.1.3. Create a New Credential - PublicKeyCredential’s [[Create]](origin, options, sameOriginWithAncestors) Method
  • 5.1.4. Use an Existing Credential to Make an Assertion - PublicKeyCredential’s [[Get]](options) Method

...in both cases next to the "Note: This algorithm is synchronous"

Proposed text:

Note: All BufferSource objects used in this algorithm must be snapshotted when the algorithm begins, to avoid potential synchronization issues. The algorithms' steps, when encountering a BufferSource, thus should be read to be acting on the state at the time the algorithm began.

jcjones added a commit to jcjones/webauthn that referenced this issue Sep 19, 2018
jcjones added a commit that referenced this issue Sep 19, 2018
…rces (#1074)

* Issue #294 - Add a non-normative comment about snapshotting BufferSources

* Add cross-link to WebIDL for "get a copy of the bytes held by the buffer source"

[=get a copy of the bytes held by the buffer source=] should have autolinked
to WebIDL, but it wasn't. I tried debugging it for a bit, but ultimately
decided to just manually specify the link. I think if a bikeshed expert wants
to fix this, go right ahead, but this works presently.
WebAuthnBot pushed a commit that referenced this issue Sep 19, 2018
WebAuthnBot pushed a commit that referenced this issue Sep 19, 2018
@akshayku
Copy link
Contributor

Fixed by #1074

@bzbarsky
Copy link
Author

I don't understand how that fixes things. This was a normative problem. How does non-normative text help?

@bzbarsky
Copy link
Author

But also, the notes that were added don't do the right thing, afaict. They snapshot at the start of the [[Create]] or [[DiscoverFromExternalSource]] algorithm, but that algorithm is invoked in a parallel section to start with, so by that point the buffer source may already have been modified from whatever was passed into the original navigator.credentials.create or navigator.credentials.get call.

@equalsJeffH
Copy link
Contributor

equalsJeffH commented Oct 5, 2018

@bzbarsky & @mikewest - please see this credential-management (credman) issue for a suggested solution to this issue:

w3c/webappsec-credential-management#128

It'd be good to get your review of the suggested algorithm and overall approach before crafting PR(s) to incorporate it. thanks :)

see also:
Fwd: Double-check WebAuthn buffersource issue resolution - Issue #294

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants