Skip to content

Commit

Permalink
Check RP ID hash returned from CTAP tokens
Browse files Browse the repository at this point in the history
CTAP HID transport protocol uses 4 byte channel ID to check that the
message sent by the authenticator is only received and processed by the
correct client process. On the other hand, CTAP BLE transport protocol
defines no such mechanism to differentiate incoming BLE fragments. This,
under some circumstances, enables relying parties to receive
response from authenticators that was intended for different site.

In order to prevent malicious RP from receiving authenticator response
intended for different site, check relying party ID hash returned from
the authenticator in response to MakeCredential and GetAssertion
response.

Bug: 828507
Change-Id: I3b743fc9b9f79284ab4b979d17c75ccc9e5a889c
Reviewed-on: https://chromium-review.googlesource.com/1004118
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551789}
  • Loading branch information
Jun Choi authored and Commit Bot committed Apr 18, 2018
1 parent 5fe1469 commit 23bef1a
Show file tree
Hide file tree
Showing 16 changed files with 268 additions and 89 deletions.
20 changes: 10 additions & 10 deletions content/browser/webauth/webauth_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ using TestGetCallbackReceiver = ::device::test::StatusAndValueCallbackReceiver<

constexpr char kRelyingPartySecurityErrorMessage[] =
"SecurityError: The relying party ID 'localhost' is not a registrable "
"domain suffix of, nor equal to 'https://www.example.com";
"domain suffix of, nor equal to 'https://www.acme.com";

constexpr char kAlgorithmUnsupportedErrorMessage[] =
"NotSupportedError: None of the algorithms specified in "
Expand Down Expand Up @@ -114,7 +114,7 @@ constexpr char kRequiredVerification[] = "required";

// Default values for kCreatePublicKeyTemplate.
struct CreateParameters {
const char* rp_id = "example.com";
const char* rp_id = "acme.com";
bool require_resident_key = false;
const char* user_verification = kPreferredVerification;
const char* authenticator_attachment = kCrossPlatform;
Expand All @@ -135,7 +135,7 @@ std::string BuildCreateCallWithParameters(const CreateParameters& parameters) {
constexpr char kGetPublicKeyTemplate[] =
"navigator.credentials.get({ publicKey: {"
" challenge: new TextEncoder().encode('climb a mountain'),"
" rp: 'example.com',"
" rp: 'acme.com',"
" timeout: 60000,"
" userVerification: '$1',"
" $2}"
Expand Down Expand Up @@ -234,7 +234,7 @@ class WebAuthBrowserTestBase : public content::ContentBrowserTest {
https_server().ServeFilesFromSourceDirectory("content/test/data");
ASSERT_TRUE(https_server().Start());

NavigateToURL(shell(), GetHttpsURL("www.example.com", "/title1.html"));
NavigateToURL(shell(), GetHttpsURL("www.acme.com", "/title1.html"));
}

GURL GetHttpsURL(const std::string& hostname,
Expand Down Expand Up @@ -293,7 +293,7 @@ class WebAuthLocalClientBrowserTest : public WebAuthBrowserTestBase {
webauth::mojom::PublicKeyCredentialCreationOptionsPtr
BuildBasicCreateOptions() {
auto rp = webauth::mojom::PublicKeyCredentialRpEntity::New(
"example.com", "example.com", base::nullopt);
"acme.com", "acme.com", base::nullopt);

std::vector<uint8_t> kTestUserId{0, 0, 0};
auto user = webauth::mojom::PublicKeyCredentialUserEntity::New(
Expand Down Expand Up @@ -331,7 +331,7 @@ class WebAuthLocalClientBrowserTest : public WebAuthBrowserTestBase {

std::vector<uint8_t> kTestChallenge{0, 0, 0};
auto mojo_options = webauth::mojom::PublicKeyCredentialRequestOptions::New(
kTestChallenge, base::TimeDelta::FromSeconds(30), "example.com",
kTestChallenge, base::TimeDelta::FromSeconds(30), "acme.com",
std::move(credentials),
webauth::mojom::UserVerificationRequirement::PREFERRED, base::nullopt);

Expand Down Expand Up @@ -367,7 +367,7 @@ IN_PROC_BROWSER_TEST_F(WebAuthLocalClientBrowserTest,
create_callback_receiver.callback());

fake_hid_discovery->WaitForCallToStartAndSimulateSuccess();
NavigateToURL(shell(), GetHttpsURL("www.example.com", "/title2.html"));
NavigateToURL(shell(), GetHttpsURL("www.acme.com", "/title2.html"));
WaitForConnectionError();

// The next active document should be able to successfully call
Expand All @@ -389,7 +389,7 @@ IN_PROC_BROWSER_TEST_F(WebAuthLocalClientBrowserTest,
get_callback_receiver.callback());

fake_hid_discovery->WaitForCallToStartAndSimulateSuccess();
NavigateToURL(shell(), GetHttpsURL("www.example.com", "/title2.html"));
NavigateToURL(shell(), GetHttpsURL("www.acme.com", "/title2.html"));
WaitForConnectionError();

// The next active document should be able to successfully call
Expand All @@ -408,7 +408,7 @@ IN_PROC_BROWSER_TEST_F(WebAuthLocalClientBrowserTest,
ScopedNavigationCancellingThrottleInstaller navigation_canceller(
shell()->web_contents());

NavigateToURL(shell(), GetHttpsURL("www.example.com", "/title2.html"));
NavigateToURL(shell(), GetHttpsURL("www.acme.com", "/title2.html"));

auto* fake_hid_discovery = discovery_factory()->ForgeNextHidDiscovery();
TestCreateCallbackReceiver create_callback_receiver;
Expand All @@ -432,7 +432,7 @@ IN_PROC_BROWSER_TEST_F(WebAuthLocalClientBrowserTest,
}));

auto* fake_hid_discovery = discovery_factory()->ForgeNextHidDiscovery();
NavigateToURL(shell(), GetHttpsURL("www.example.com", "/title2.html"));
NavigateToURL(shell(), GetHttpsURL("www.acme.com", "/title2.html"));
WaitForConnectionError();

// Normally, when the request is serviced, the implementation retrieves the
Expand Down
4 changes: 4 additions & 0 deletions device/fido/attestation_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) AttestationObject {
// "attStmt": attestation statement bytes }
std::vector<uint8_t> SerializeToCBOREncodedBytes() const;

const std::vector<uint8_t>& rp_id_hash() const {
return authenticator_data_.application_parameter();
}

private:
AuthenticatorData authenticator_data_;
std::unique_ptr<AttestationStatement> attestation_statement_;
Expand Down
5 changes: 5 additions & 0 deletions device/fido/authenticator_data.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) AuthenticatorData {
return attested_data_;
}

const std::vector<uint8_t>& application_parameter() const {
return application_parameter_;
}

bool obtained_user_presence() const {
return flags_ & base::strict_cast<uint8_t>(Flag::kTestOfUserPresence);
}
Expand All @@ -79,6 +83,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) AuthenticatorData {
private:
// The application parameter: a SHA-256 hash of either the RP ID or the AppID
// associated with the credential.
// TODO(hongjunchoi): Replace fixed size vector components with std::array.
std::vector<uint8_t> application_parameter_;

// Flags (bit 0 is the least significant bit):
Expand Down
9 changes: 7 additions & 2 deletions device/fido/authenticator_get_assertion_response.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ AuthenticatorGetAssertionResponse::CreateFromU2fSignResponse(
if (signature.empty())
return base::nullopt;

auto response = AuthenticatorGetAssertionResponse(
std::move(authenticator_data), std::move(signature));
AuthenticatorGetAssertionResponse response(std::move(authenticator_data),
std::move(signature));
response.SetCredential(PublicKeyCredentialDescriptor(
to_string(CredentialType::kPublicKey), key_handle));

Expand All @@ -71,6 +71,11 @@ AuthenticatorGetAssertionResponse& AuthenticatorGetAssertionResponse::operator=(
AuthenticatorGetAssertionResponse::~AuthenticatorGetAssertionResponse() =
default;

const std::vector<uint8_t>& AuthenticatorGetAssertionResponse::GetRpIdHash()
const {
return authenticator_data_.application_parameter();
}

AuthenticatorGetAssertionResponse&
AuthenticatorGetAssertionResponse::SetCredential(
PublicKeyCredentialDescriptor credential) {
Expand Down
5 changes: 4 additions & 1 deletion device/fido/authenticator_get_assertion_response.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) AuthenticatorGetAssertionResponse
AuthenticatorGetAssertionResponse(AuthenticatorGetAssertionResponse&& that);
AuthenticatorGetAssertionResponse& operator=(
AuthenticatorGetAssertionResponse&& other);
~AuthenticatorGetAssertionResponse();
~AuthenticatorGetAssertionResponse() override;

// ResponseData:
const std::vector<uint8_t>& GetRpIdHash() const override;

AuthenticatorGetAssertionResponse& SetCredential(
PublicKeyCredentialDescriptor credential);
Expand Down
5 changes: 5 additions & 0 deletions device/fido/authenticator_make_credential_response.cc
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,9 @@ bool AuthenticatorMakeCredentialResponse::
.IsAttestationCertificateInappropriatelyIdentifying();
}

const std::vector<uint8_t>& AuthenticatorMakeCredentialResponse::GetRpIdHash()
const {
return attestation_object_.rp_id_hash();
}

} // namespace device
5 changes: 4 additions & 1 deletion device/fido/authenticator_make_credential_response.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) AuthenticatorMakeCredentialResponse
AuthenticatorMakeCredentialResponse&& that);
AuthenticatorMakeCredentialResponse& operator=(
AuthenticatorMakeCredentialResponse&& other);
~AuthenticatorMakeCredentialResponse();
~AuthenticatorMakeCredentialResponse() override;

std::vector<uint8_t> GetCBOREncodedAttestationObject() const;

Expand All @@ -51,6 +51,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) AuthenticatorMakeCredentialResponse
// not indended to be trackable.)
bool IsAttestationCertificateInappropriatelyIdentifying();

// ResponseData:
const std::vector<uint8_t>& GetRpIdHash() const override;

private:
AttestationObject attestation_object_;

Expand Down
Loading

0 comments on commit 23bef1a

Please sign in to comment.