Blank rp.id in `navigator.credentials.create` crashes Chrome tab
Reported by
sra...@duosecurity.com,
Apr 9 2018
|
|||||||||
Issue description
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36
Steps to reproduce the problem:
```
challenge = Array.from(Array(32)).map(() => "a").join("")
challenge = Uint8Array.from(challenge, c => c.charCodeAt(0));
userId = "some-id";
userId = Uint8Array.from(userId, c => c.charCodeAt(0));
options = {
publicKey: {
challenge,
rp: {
name: "foo",
id: "",
},
user: {
id: userId,
name: 'username',
displayName: 'displayName'
},
pubKeyCredParams: [{alg: 'ES256', 'type': 'public-key'}]
}
}
navigator.credentials.create(options)
```
What is the expected behavior?
For the browser to not crash.
What went wrong?
The tab crashes and displays the "Aw snap" screen
Did this work before? N/A
Does this work in other browsers? N/A
Chrome version: 65.0.3325.181 Channel: stable
OS Version: OS X 10.13.3
Flash Version:
,
Apr 16 2018
sraman@, Could you please respond on C#1. Thanks..!
,
Apr 17 2018
Mind triaging this, engedy@?
,
Apr 17 2018
Kim, there seems to be a discrepancy between the renderer-side and browser-side relying party id checks. Namely, the former lets through an empty rpId, while the latter does not, and kills the renderer for the bad IPC message. Can you please look into fixing this and adding a regression layout test? We want to merge this into M67.
,
Apr 17 2018
,
Apr 18 2018
Just uploaded a crash report! Uploaded Crash Report ID 1cc22eda5be8640f (Local Crash ID: 83d5f91d-2e9f-4dfd-be51-3325a2688954) Crash report captured on Wednesday, April 18, 2018 at 1:30:01 PM, uploaded on Wednesday, April 18, 2018 at 1:30:02 PM
,
Apr 19 2018
The problem is that the renderer only checks for null strings, not empty strings. Crashing if an empty string is presented to the browser-side is technically correct, but it should be caught as a SecurityError in the renderer first. Getting a fix and test ready.
,
Apr 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/051979a8dd6adbc6319cbae327b846bf06fbd17f commit 051979a8dd6adbc6319cbae327b846bf06fbd17f Author: Kim Paulhamus <kpaulhamus@chromium.org> Date: Mon Apr 23 18:37:31 2018 [webauthn] Fix crash when rp.id is empty string The renderer now checks for empty as well as null strings. Crashing if an empty string is presented to the browser-side is technically correct, but it is now caught as a SecurityError in the renderer first. Bug: 830855 Change-Id: Id904864b8f6d57e9e0eb342d287337ba96d368d4 Reviewed-on: https://chromium-review.googlesource.com/1020187 Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org> Reviewed-by: Balazs Engedy <engedy@chromium.org> Cr-Commit-Position: refs/heads/master@{#552764} [modify] https://crrev.com/051979a8dd6adbc6319cbae327b846bf06fbd17f/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/051979a8dd6adbc6319cbae327b846bf06fbd17f/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-basics.html [modify] https://crrev.com/051979a8dd6adbc6319cbae327b846bf06fbd17f/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-origins.html [modify] https://crrev.com/051979a8dd6adbc6319cbae327b846bf06fbd17f/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-with-virtual-authenticator.html [modify] https://crrev.com/051979a8dd6adbc6319cbae327b846bf06fbd17f/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-origins.html [modify] https://crrev.com/051979a8dd6adbc6319cbae327b846bf06fbd17f/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-with-virtual-authenticator.html [modify] https://crrev.com/051979a8dd6adbc6319cbae327b846bf06fbd17f/third_party/blink/renderer/modules/credentialmanager/credentials_container.cc
,
Apr 25 2018
Requesting merge to M67 to address crash.
,
Apr 25 2018
How is the change listed at #9 looking in canary?
,
Apr 25 2018
Good! I verified that stable crashes and canary with the fix does not.
,
Apr 25 2018
Thank you kpaulhamus@. Lets wait until Auto approval script to kick it in. If not, I will approve the merge tomorrow morning.
,
Apr 26 2018
Your change meets the bar and is auto-approved for M67. Please go ahead and merge the CL to branch 3396 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 26 2018
Pls merge your change to M67 branch 3396 ASAP so we can pick it up for next M67 Beta release. Thank you.
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/08c2dd48d52ba326e6b23c5e26f9bdd22edb0ae3 commit 08c2dd48d52ba326e6b23c5e26f9bdd22edb0ae3 Author: Kim Paulhamus <kpaulhamus@chromium.org> Date: Thu Apr 26 20:54:58 2018 [webauthn] Fix crash when rp.id is empty string The renderer now checks for empty as well as null strings. Crashing if an empty string is presented to the browser-side is technically correct, but it is now caught as a SecurityError in the renderer first. Bug: 830855 Change-Id: Id904864b8f6d57e9e0eb342d287337ba96d368d4 Reviewed-on: https://chromium-review.googlesource.com/1020187 Commit-Queue: Kim Paulhamus <kpaulhamus@chromium.org> Reviewed-by: Balazs Engedy <engedy@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#552764}(cherry picked from commit 051979a8dd6adbc6319cbae327b846bf06fbd17f) Reviewed-on: https://chromium-review.googlesource.com/1031251 Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#342} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/08c2dd48d52ba326e6b23c5e26f9bdd22edb0ae3/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/08c2dd48d52ba326e6b23c5e26f9bdd22edb0ae3/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-basics.html [modify] https://crrev.com/08c2dd48d52ba326e6b23c5e26f9bdd22edb0ae3/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-origins.html [modify] https://crrev.com/08c2dd48d52ba326e6b23c5e26f9bdd22edb0ae3/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-create-with-virtual-authenticator.html [modify] https://crrev.com/08c2dd48d52ba326e6b23c5e26f9bdd22edb0ae3/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-origins.html [modify] https://crrev.com/08c2dd48d52ba326e6b23c5e26f9bdd22edb0ae3/third_party/WebKit/LayoutTests/http/tests/credentialmanager/credentialscontainer-get-with-virtual-authenticator.html [modify] https://crrev.com/08c2dd48d52ba326e6b23c5e26f9bdd22edb0ae3/third_party/blink/renderer/modules/credentialmanager/credentials_container.cc |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by rsesek@chromium.org
, Apr 9 2018Labels: Stability-Crash