Closed
Bug 1275479
Opened 8 years ago
Closed 8 years ago
Refactor FIDO U2F to use token interfaces
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: jcj, Assigned: jcj)
Details
(Whiteboard: btpp-active)
Attachments
(2 files)
The dom/u2f/U2F.cpp class has hardcoded logic to specifically call the NSS Token; it should be refactored to use an interface based on nsINSSU2FToken.
Assignee | ||
Comment 1•8 years ago
|
||
Note: One of the reasons for this is to support community contributors who would like to provide alternative token implementations, such as Bluetooth Smart or NFC.
Assignee | ||
Comment 2•8 years ago
|
||
Rework U2F.cpp to use a collection of nsINSSU2FToken for U2F/WebAuth operations. Review commit: https://reviewboard.mozilla.org/r/55918/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/55918/
Attachment #8757493 -
Flags: review?(dkeeler)
Attachment #8757494 -
Flags: review?(dkeeler)
Assignee | ||
Comment 3•8 years ago
|
||
Create a base "nsIU2FToken" interface that all tokens must implement. This patch does not change U2F.cpp from initializing tokens monolithically, but if/when future tokens are added, the implementer may want to do that. Review commit: https://reviewboard.mozilla.org/r/55920/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/55920/
Assignee | ||
Comment 4•8 years ago
|
||
Try run in progress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55259772e6e6
Assignee: nobody → jjones
Status: NEW → ASSIGNED
Updated•8 years ago
|
Whiteboard: btpp-active
Assignee | ||
Comment 5•8 years ago
|
||
Better try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=157aaa8ec16b
Attachment #8757493 -
Flags: review?(dkeeler) → review+
Comment on attachment 8757493 [details] MozReview Request: Bug 1275479 - Refactor U2F Token Interface (Part 1) r=keeler https://reviewboard.mozilla.org/r/55918/#review53354 This looks good. ::: dom/u2f/NSSU2FTokenRemote.cpp:52 (Diff revision 1) > +{ > + NS_ENSURE_ARG_POINTER(aKeyHandle); > + NS_ENSURE_ARG_POINTER(aIsRegistered); > + > + nsTArray<uint8_t> keyHandle; > + keyHandle.ReplaceElementsAt(0, keyHandle.Length(), aKeyHandle, aKeyHandleLen); This is fallible, right? We should check OOM here. ::: dom/u2f/NSSU2FTokenRemote.cpp:76 (Diff revision 1) > + NS_ENSURE_ARG_POINTER(aChallenge); > + NS_ENSURE_ARG_POINTER(aRegistration); > + NS_ENSURE_ARG_POINTER(aRegistrationLen); > + > + nsTArray<uint8_t> application; > + application.ReplaceElementsAt(0, application.Length(), aApplication, Same idea here and elsewhere. ::: dom/u2f/NSSU2FTokenRemote.cpp:89 (Diff revision 1) > + if (!cc->SendNSSU2FTokenRegister(application, challenge, > + ®istrationBuffer)) { > + return NS_ERROR_FAILURE; > + } > + > + uint32_t dataLen = registrationBuffer.Length(); I think Length() returns a size_t here, so we should check for overflow. ::: dom/u2f/NSSU2FTokenRemote.cpp:129 (Diff revision 1) > + if (!cc->SendNSSU2FTokenSign(application, challenge, keyHandle, > + &signatureBuffer)) { > + return NS_ERROR_FAILURE; > + } > + > + uint32_t dataLen = signatureBuffer.Length(); Same here. ::: dom/u2f/U2F.cpp:154 (Diff revision 1) > bool isCompatible = false; > bool isRegistered = false; > > // Determine if the provided keyHandle is registered at any device. If so, > // then we'll return DEVICE_INELIGIBLE to signify we're already registered. > - if (softTokenEnabled) { > + for (size_t a = 0; a < mAuthenticators.Length(); ++a) { Can you do |for (auto token : mAuthenticators) {| here? ::: dom/u2f/U2F.cpp:163 (Diff revision 1) > if (NS_FAILED(rv)) { > ReturnError(ErrorCode::OTHER_ERROR); > return NS_ERROR_FAILURE; > } > > - rv = NSSTokenIsRegistered(mNSSToken, keyHandle, &isRegistered); > + rv = token->IsRegistered(keyHandle.Elements(), keyHandle.Length(), hmmm - if the token isn't compatible, do we even want to call IsRegistered? ::: dom/u2f/U2F.cpp:227 (Diff revision 1) > // Get the registration data from the token > CryptoBuffer regData; > bool registerSuccess = false; > bool isCompatible = false; > > - if (!registerSuccess && softTokenEnabled) { > + for (size_t a = 0; a < mAuthenticators.Length() && !registerSuccess; ++a) { Same idea with |for (auto token: mAuthenticators) {| (I guess registerSuccess will have to be checked separately...) ::: dom/u2f/U2F.cpp:402 (Diff revision 1) > if (NS_FAILED(rv)) { > ReturnError(ErrorCode::OTHER_ERROR); > return NS_ERROR_FAILURE; > } > > - rv = NSSTokenIsRegistered(mNSSToken, keyHandle, &isRegistered); > + rv = token->IsRegistered(keyHandle.Elements(), keyHandle.Length(), Same question about if !isCompatible
Attachment #8757494 -
Flags: review?(dkeeler) → review+
Comment on attachment 8757494 [details] MozReview Request: Bug 1275479 - Create nsIU2FToken base interface (Part 2) r=keeler https://reviewboard.mozilla.org/r/55920/#review53388
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8757493 [details] MozReview Request: Bug 1275479 - Refactor U2F Token Interface (Part 1) r=keeler Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55918/diff/1-2/
Attachment #8757493 -
Attachment description: MozReview Request: Bug 1275479 - Refactor U2F Token Interface (Part 1) r?keeler → MozReview Request: Bug 1275479 - Refactor U2F Token Interface (Part 1) r=keeler
Attachment #8757494 -
Attachment description: MozReview Request: Bug 1275479 - Refactor U2F Token Interface (Part 2) r?keeler → MozReview Request: Bug 1275479 - Create nsIU2FToken base interface (Part 2) r=keeler
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8757494 [details] MozReview Request: Bug 1275479 - Create nsIU2FToken base interface (Part 2) r=keeler Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55920/diff/1-2/
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8757493 [details] MozReview Request: Bug 1275479 - Refactor U2F Token Interface (Part 1) r=keeler Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55918/diff/2-3/
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8757494 [details] MozReview Request: Bug 1275479 - Create nsIU2FToken base interface (Part 2) r=keeler Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55920/diff/2-3/
Assignee | ||
Comment 12•8 years ago
|
||
Thank you for the review, Keeler! Excellent comments as always. Update posted, kept your r+, and try run looks good [1]. Marking checkin-needed. 1) https://treeherder.mozilla.org/#/jobs?repo=try&revision=108817479038
Keywords: checkin-needed
Comment 13•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d82acfe62e Refactor U2F Token Interface (Part 1). r=keeler https://hg.mozilla.org/integration/mozilla-inbound/rev/52bd4c6b7f7c Create nsIU2FToken base interface (Part 2). r=keeler
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7d82acfe62e https://hg.mozilla.org/mozilla-central/rev/52bd4c6b7f7c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•