Skip to content

Commit

Permalink
webauthn: prompt for attestation permission when needed.
Browse files Browse the repository at this point in the history
This change implements a user consent prompt before returning
attestation information from a device. (Thus making webauthn act like
U2F currently does.) Unlike U2F, however, it is a fatal error if a user
denies consent, as required by the spec.

The attestation behavior is also affected by the
SecurityKeyPermitAttestation[1] enterprise policy. This list can contain
either U2F AppIDs (which are full URLs) or webauthn RP IDs (which are
domains). Its affect on attestation is detailed in the following table:

"attestation" value | RP ID not listed in policy | RP ID listed
--------------------+----------------------------+---------------------
"none" / not given  | Empty, "none" attestation  | Empty, "none"
                    | returned.                  | attesation returned.
--------------------+----------------------------+---------------------
"indirect"/"direct" | User prompted for consent. | Attestation from
                    | If granted, attestation    | device is returned.
                    | from device is returned.   |
                    | Otherwise a permission     |
                    | error is generated.        |

(The behavior of "indirect" attestation in webauthn may change in the
future but, for now, it is identical to "direct".)

[1] https://www.chromium.org/administrators/policy-list-3#SecurityKeyPermitAttestation

Bug: 803829,793985
Change-Id: I4e1d15a93ebc067869df7656016990b29fe12b59
Reviewed-on: https://chromium-review.googlesource.com/900452
Reviewed-by: Timothy Loh <timloh@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Kim Paulhamus <kpaulhamus@chromium.org>
Commit-Queue: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536206}
  • Loading branch information
Adam Langley authored and Commit Bot committed Feb 12, 2018
1 parent 103883f commit 85339f6
Show file tree
Hide file tree
Showing 10 changed files with 223 additions and 68 deletions.
2 changes: 2 additions & 0 deletions chrome/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -2564,6 +2564,8 @@ split_static_library("browser") {
"page_load_metrics/observers/session_restore_page_load_metrics_observer.h",
"pdf/pdf_extension_util.cc",
"pdf/pdf_extension_util.h",
"permissions/attestation_permission_request.cc",
"permissions/attestation_permission_request.h",
"picture_in_picture/picture_in_picture_window_controller.cc",
"picture_in_picture/picture_in_picture_window_controller.h",
"policy/local_sync_policy_handler.cc",
Expand Down
63 changes: 54 additions & 9 deletions chrome/browser/chrome_content_browser_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@
#include "chrome/browser/page_load_metrics/page_load_metrics_util.h"
#include "chrome/browser/password_manager/chrome_password_manager_client.h"
#include "chrome/browser/payments/payment_request_display_manager_factory.h"
#include "chrome/browser/permissions/attestation_permission_request.h"
#include "chrome/browser/permissions/permission_context_base.h"
#include "chrome/browser/permissions/permission_request_manager.h"
#include "chrome/browser/platform_util.h"
#include "chrome/browser/plugins/pdf_iframe_navigation_throttle.h"
#include "chrome/browser/prerender/prerender_final_status.h"
Expand Down Expand Up @@ -809,6 +811,25 @@ chrome::mojom::PrerenderCanceler* GetPrerenderCanceller(
return prerender::PrerenderContents::FromWebContents(web_contents);
}

// Returns true iff |rp_id| is listed in the SecurityKeyPermitAttestation
// policy.
bool IsWebauthnRPIDListedInEnterprisePolicy(
content::BrowserContext* browser_context,
const std::string& rp_id) {
#if defined(OS_ANDROID)
return false;
#else
const Profile* profile = Profile::FromBrowserContext(browser_context);
const PrefService* prefs = profile->GetPrefs();
const base::ListValue* permit_attestation =
prefs->GetList(prefs::kSecurityKeyPermitAttestation);

return std::any_of(
permit_attestation->begin(), permit_attestation->end(),
[&rp_id](const base::Value& v) { return v.GetString() == rp_id; });
#endif
}

} // namespace

ChromeContentBrowserClient::ChromeContentBrowserClient()
Expand Down Expand Up @@ -3914,18 +3935,42 @@ bool ChromeContentBrowserClient::
ShouldPermitIndividualAttestationForWebauthnRPID(
content::BrowserContext* browser_context,
const std::string& rp_id) {
// If the RP ID is listed in the policy, signal that individual attestation is
// permitted.
return IsWebauthnRPIDListedInEnterprisePolicy(browser_context, rp_id);
}

void ChromeContentBrowserClient::ShouldReturnAttestationForWebauthnRPID(
content::RenderFrameHost* rfh,
const std::string& rp_id,
const url::Origin& origin,
base::OnceCallback<void(bool)> callback) {
#if defined(OS_ANDROID)
return false;
// Android is expected to use platform APIs for webauthn which will take care
// of prompting.
std::move(callback).Run(true);
#else
const Profile* profile = Profile::FromBrowserContext(browser_context);
const PrefService* prefs = profile->GetPrefs();
const base::ListValue* permit_attestation =
prefs->GetList(prefs::kSecurityKeyPermitAttestation);
WebContents* web_contents = WebContents::FromRenderFrameHost(rfh);
content::BrowserContext* browser_context = web_contents->GetBrowserContext();

// If the RP ID is listed in the policy, enable individual attestation.
return std::any_of(
permit_attestation->begin(), permit_attestation->end(),
[&rp_id](const base::Value& v) { return v.GetString() == rp_id; });
if (IsWebauthnRPIDListedInEnterprisePolicy(browser_context, rp_id)) {
std::move(callback).Run(true);
return;
}

// This does not use content::PermissionManager because that only works with
// content settings, while this permission is a non-persisted, per-attested-
// registration consent.
PermissionRequestManager* permission_request_manager =
PermissionRequestManager::FromWebContents(web_contents);
if (!permission_request_manager) {
std::move(callback).Run(false);
return;
}

// The created AttestationPermissionRequest deletes itself once complete.
permission_request_manager->AddRequest(
NewAttestationPermissionRequest(origin, std::move(callback)));
#endif
}

Expand Down
5 changes: 5 additions & 0 deletions chrome/browser/chrome_content_browser_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,11 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
bool ShouldPermitIndividualAttestationForWebauthnRPID(
content::BrowserContext* browser_context,
const std::string& rp_id) override;
void ShouldReturnAttestationForWebauthnRPID(
content::RenderFrameHost* rfh,
const std::string& rp_id,
const url::Origin& origin,
base::OnceCallback<void(bool)> callback) override;

std::unique_ptr<net::ClientCertStore> CreateClientCertStore(
content::ResourceContext* resource_context) override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,18 @@
#include "base/memory/ptr_util.h"
#include "base/strings/utf_string_conversions.h"
#include "build/build_config.h"
#include "chrome/app/vector_icons/vector_icons.h"
#include "chrome/browser/extensions/extension_tab_util.h"
#include "chrome/browser/permissions/permission_request.h"
#include "chrome/browser/permissions/attestation_permission_request.h"
#include "chrome/browser/permissions/permission_request_manager.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/pref_names.h"
#include "chrome/grit/generated_resources.h"
#include "components/pref_registry/pref_registry_syncable.h"
#include "components/prefs/pref_service.h"
#include "crypto/sha2.h"
#include "extensions/common/error_utils.h"
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
#include "ui/base/l10n/l10n_util.h"
#include "url/origin.h"

namespace {

Expand Down Expand Up @@ -57,52 +55,6 @@ bool ContainsAppIdByHash(const base::ListValue& list,
return false;
}

// AttestationPermissionRequest is a delegate class that provides information
// and callbacks to the PermissionRequestManager.
//
// PermissionRequestManager has a reference to this object and so this object
// must outlive it. Since attestation requests are never canceled,
// PermissionRequestManager guarentees that |RequestFinished| will always,
// eventually, be called. This object uses that fact to delete itself during
// |RequestFinished| and thus owns itself.
class AttestationPermissionRequest : public PermissionRequest {
public:
AttestationPermissionRequest(const GURL& app_id,
base::OnceCallback<void(bool)> callback)
: app_id_(app_id), callback_(std::move(callback)) {}

PermissionRequest::IconId GetIconId() const override {
return kUsbSecurityKeyIcon;
}

base::string16 GetMessageTextFragment() const override {
return l10n_util::GetStringUTF16(
IDS_SECURITY_KEY_ATTESTATION_PERMISSION_FRAGMENT);
}
GURL GetOrigin() const override { return app_id_; }
void PermissionGranted() override { std::move(callback_).Run(true); }
void PermissionDenied() override { std::move(callback_).Run(false); }
void Cancelled() override { std::move(callback_).Run(false); }

void RequestFinished() override {
if (callback_)
std::move(callback_).Run(false);
delete this;
}

PermissionRequestType GetPermissionRequestType() const override {
return PermissionRequestType::PERMISSION_SECURITY_KEY_ATTESTATION;
}

private:
~AttestationPermissionRequest() override = default;

const GURL app_id_;
base::OnceCallback<void(bool)> callback_;

DISALLOW_COPY_AND_ASSIGN(AttestationPermissionRequest);
};

} // namespace

namespace extensions {
Expand Down Expand Up @@ -241,8 +193,9 @@ CryptotokenPrivateCanAppIdGetAttestationFunction::Run() {
}

// The created AttestationPermissionRequest deletes itself once complete.
permission_request_manager->AddRequest(new AttestationPermissionRequest(
app_id_url,
const url::Origin origin(url::Origin::Create(app_id_url));
permission_request_manager->AddRequest(NewAttestationPermissionRequest(
origin,
base::BindOnce(
&CryptotokenPrivateCanAppIdGetAttestationFunction::Complete, this)));
return RespondLater();
Expand Down
66 changes: 66 additions & 0 deletions chrome/browser/permissions/attestation_permission_request.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Copyright 2018 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "chrome/browser/permissions/attestation_permission_request.h"

#include "base/callback.h"
#include "chrome/app/vector_icons/vector_icons.h"
#include "chrome/browser/permissions/permission_request.h"
#include "chrome/grit/generated_resources.h"
#include "ui/base/l10n/l10n_util.h"
#include "url/origin.h"

// AttestationPermissionRequest is a delegate class that provides information
// and callbacks to the PermissionRequestManager.
//
// PermissionRequestManager has a reference to this object and so this object
// must outlive it. Since attestation requests are never canceled,
// PermissionRequestManager guarentees that |RequestFinished| will always,
// eventually, be called. This object uses that fact to delete itself during
// |RequestFinished| and thus owns itself.
class AttestationPermissionRequest : public PermissionRequest {
public:
AttestationPermissionRequest(const url::Origin& origin,
base::OnceCallback<void(bool)> callback)
: origin_(origin), callback_(std::move(callback)) {}

PermissionRequest::IconId GetIconId() const override {
return kUsbSecurityKeyIcon;
}

base::string16 GetMessageTextFragment() const override {
return l10n_util::GetStringUTF16(
IDS_SECURITY_KEY_ATTESTATION_PERMISSION_FRAGMENT);
}
GURL GetOrigin() const override { return origin_.GetURL(); }
void PermissionGranted() override { std::move(callback_).Run(true); }
void PermissionDenied() override { std::move(callback_).Run(false); }
void Cancelled() override { std::move(callback_).Run(false); }

void RequestFinished() override {
// callback_ may not have run if the prompt was ignored. (I.e. the tab was
// closed while the prompt was displayed.)
if (callback_)
std::move(callback_).Run(false);
delete this;
}

PermissionRequestType GetPermissionRequestType() const override {
return PermissionRequestType::PERMISSION_SECURITY_KEY_ATTESTATION;
}

private:
~AttestationPermissionRequest() override = default;

const url::Origin origin_;
base::OnceCallback<void(bool)> callback_;

DISALLOW_COPY_AND_ASSIGN(AttestationPermissionRequest);
};

PermissionRequest* NewAttestationPermissionRequest(
const url::Origin& origin,
base::OnceCallback<void(bool)> callback) {
return new AttestationPermissionRequest(origin, std::move(callback));
}
26 changes: 26 additions & 0 deletions chrome/browser/permissions/attestation_permission_request.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright 2018 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef CHROME_BROWSER_PERMISSIONS_ATTESTATION_PERMISSION_REQUEST_H_
#define CHROME_BROWSER_PERMISSIONS_ATTESTATION_PERMISSION_REQUEST_H_

#include "base/callback_forward.h"

class PermissionRequest;

namespace url {
class Origin;
}

// Returns a |PermissionRequest| that asks the user to consent to sending
// identifying information about their security key. The |origin| argument is
// used to identify the origin that is requesting the permission, and only the
// authority part of the URL is used. The caller takes ownership of the returned
// object because the standard pattern for PermissionRequests is that they
// delete themselves once complete.
PermissionRequest* NewAttestationPermissionRequest(
const url::Origin& origin,
base::OnceCallback<void(bool)> callback);

#endif // CHROME_BROWSER_PERMISSIONS_ATTESTATION_PERMISSION_REQUEST_H_
44 changes: 37 additions & 7 deletions content/browser/webauth/authenticator_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ void AuthenticatorImpl::MakeCredential(
}

url::Origin caller_origin = render_frame_host_->GetLastCommittedOrigin();
relying_party_id_ = options->relying_party->id;

if (!HasValidEffectiveDomain(caller_origin)) {
bad_message::ReceivedBadMessage(render_frame_host_->GetProcess(),
Expand All @@ -195,7 +196,7 @@ void AuthenticatorImpl::MakeCredential(
return;
}

if (!IsRelyingPartyIdValid(options->relying_party->id, caller_origin)) {
if (!IsRelyingPartyIdValid(relying_party_id_, caller_origin)) {
bad_message::ReceivedBadMessage(render_frame_host_->GetProcess(),
bad_message::AUTH_INVALID_RELYING_PARTY);
std::move(callback).Run(webauth::mojom::AuthenticatorStatus::INVALID_DOMAIN,
Expand Down Expand Up @@ -244,7 +245,7 @@ void AuthenticatorImpl::MakeCredential(
->browser()
->ShouldPermitIndividualAttestationForWebauthnRPID(
render_frame_host_->GetProcess()->GetBrowserContext(),
options->relying_party->id);
relying_party_id_);

attestation_preference_ = options->attestation;

Expand All @@ -255,9 +256,9 @@ void AuthenticatorImpl::MakeCredential(
// Among other things, the Client Data contains the challenge from the
// relying party (hence the name of the parameter).
u2f_request_ = device::U2fRegister::TryRegistration(
options->relying_party->id, connector_, protocols_, registered_keys,
relying_party_id_, connector_, protocols_, registered_keys,
ConstructClientDataHash(client_data_.SerializeToJson()),
CreateAppId(options->relying_party->id), individual_attestation,
CreateAppId(relying_party_id_), individual_attestation,
base::BindOnce(&AuthenticatorImpl::OnRegisterResponse,
weak_factory_.GetWeakPtr()));
}
Expand Down Expand Up @@ -336,19 +337,48 @@ void AuthenticatorImpl::OnRegisterResponse(
break;
case device::U2fReturnCode::SUCCESS:
DCHECK(response_data.has_value());
if (attestation_preference_ ==

if (attestation_preference_ !=
webauth::mojom::AttestationConveyancePreference::NONE) {
response_data->EraseAttestationStatement();
// Potentially show a permission prompt before returning the
// attestation data.
GetContentClient()->browser()->ShouldReturnAttestationForWebauthnRPID(
render_frame_host_, relying_party_id_,
render_frame_host_->GetLastCommittedOrigin(),
base::BindOnce(
&AuthenticatorImpl::OnRegisterResponseAttestationDecided,
weak_factory_.GetWeakPtr(), std::move(*response_data)));
return;
}

response_data->EraseAttestationStatement();
std::move(make_credential_response_callback_)
.Run(webauth::mojom::AuthenticatorStatus::SUCCESS,
CreateMakeCredentialResponse(std::move(client_data_),
std::move(*response_data)));
break;
}
Cleanup();
}

void AuthenticatorImpl::OnRegisterResponseAttestationDecided(
device::RegisterResponseData response_data,
bool attestation_permitted) {
DCHECK(attestation_preference_ !=
webauth::mojom::AttestationConveyancePreference::NONE);

if (!attestation_permitted) {
std::move(make_credential_response_callback_)
.Run(webauth::mojom::AuthenticatorStatus::NOT_ALLOWED_ERROR, nullptr);
} else {
std::move(make_credential_response_callback_)
.Run(webauth::mojom::AuthenticatorStatus::SUCCESS,
CreateMakeCredentialResponse(std::move(client_data_),
std::move(response_data)));
}

Cleanup();
}

void AuthenticatorImpl::OnSignResponse(
device::U2fReturnCode status_code,
base::Optional<device::SignResponseData> response_data) {
Expand Down
Loading

0 comments on commit 85339f6

Please sign in to comment.