New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 880520 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Oct 31
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Fix HID command type to CTAPHID_MSG when sending U2F register command to devices with client pin set

Project Member Reported by hongjunchoi@chromium.org, Sep 4

Issue description

Currently CTAPHID_CBOR command type is used when we send U2F register command for to devices with client pin set as a fallback. Fix this to be CTAPHID_MSG instead.
 
Cc: agl@chromium.org
Labels: -Pri-1 OS-Chrome OS-Linux OS-Mac OS-Windows Pri-0
Adam, Balazs,

Jun is going to have a patch today, but this effectively means that Yubikeys with clientPin set cannot service a makeCredential request in Chrome m69.

What are our options to address this?

Background: some Yubikeys are dual U2F/CTAP2, and since Chrome does not yet support ClientPin, our implementation would fall back to U2F if the device had a pin set. However, the fallback wasn't actually working because we were continuing to use a CTAP2 message type with a U2F-formatted message. 

This only affects makeCredential. GetAssertion is not affected.

I don't have an estimate for the scope of affected users. These yubikeys are on the market, and yubikeys that have been used with Edge commonly have clientPin set on them (the Edge flow encourages it). It's unclear to me how many users would register credentials via Edge and then go on to register credentials via Chrome.
Here is Jun's CL: https://chromium-review.googlesource.com/c/chromium/src/+/1204919

It's a small fix, and I've verified that he manually tested it with the affected keys.
Spoke with agl@. We're going to let this hang out in Canary and request merge to 69 and 70 later in the week.
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3de6bb76cf3fa8f6f7dd8182a05629b2a4f88db3

commit 3de6bb76cf3fa8f6f7dd8182a05629b2a4f88db3
Author: Jun Choi <hongjunchoi@chromium.org>
Date: Tue Sep 04 21:50:09 2018

Use CTAPHID_MSG command when sending U2F register request

When we send U2F register to CTAP2 devices as a fallback when the
connected device has client pin set, we use CTAPHID_CBOR msg. Fix this
so that CTAPHID_MSG command is used instead.

Bug:  880520 
Change-Id: I86343696c70f27867edd820b11a3ac4270051980
Reviewed-on: https://chromium-review.googlesource.com/1204919
Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588663}
[modify] https://crrev.com/3de6bb76cf3fa8f6f7dd8182a05629b2a4f88db3/device/fido/make_credential_task.cc
[modify] https://crrev.com/3de6bb76cf3fa8f6f7dd8182a05629b2a4f88db3/device/fido/make_credential_task_unittest.cc

Labels: -Pri-0 Merge-Request-70 Merge-Request-69 Pri-1
Requesting merge to M70 and, if we're going to do a respin, M69 too.

Risk: low. There's only one meaningful line added to set a variable. It's in Canary and Jun or Kim should be able to verify the fix in a released Canary version now. (I don't have a PIN-locked token to hand.)

Benefits: probably fairly low. A CTAP2 device with a PIN set is certainly a corner case. I believe we should merge this for M70, but the case for M69 is weaker. I don't know that we've received any user reports of this problem yet, although I think it counts as a regression for M69 since we didn't support CTAP2 at all in M68, as far as I know.
Project Member

Comment 6 by sheriffbot@chromium.org, Sep 6

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-69
IMed with govind. This isn't critical enough to make the M69 respin so dropping merge request for M69.
Thanks, Adam.
Project Member

Comment 9 by sheriffbot@chromium.org, Sep 7

Labels: -Merge-Request-70 Hotlist-Merge-Approved Merge-Approved-70
Your change meets the bar and is auto-approved for M70. Please go ahead and merge the CL to branch 3538 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 7

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dcd0879099b4befe826588c63254591278bac6b9

commit dcd0879099b4befe826588c63254591278bac6b9
Author: Jun Choi <hongjunchoi@chromium.org>
Date: Fri Sep 07 20:41:27 2018

Use CTAPHID_MSG command when sending U2F register request

When we send U2F register to CTAP2 devices as a fallback when the
connected device has client pin set, we use CTAPHID_CBOR msg. Fix this
so that CTAPHID_MSG command is used instead.

Bug:  880520 
Change-Id: I86343696c70f27867edd820b11a3ac4270051980
Reviewed-on: https://chromium-review.googlesource.com/1204919
Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#588663}(cherry picked from commit 3de6bb76cf3fa8f6f7dd8182a05629b2a4f88db3)
Reviewed-on: https://chromium-review.googlesource.com/1214168
Reviewed-by: Jun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#158}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/dcd0879099b4befe826588c63254591278bac6b9/device/fido/make_credential_task.cc
[modify] https://crrev.com/dcd0879099b4befe826588c63254591278bac6b9/device/fido/make_credential_task_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment