certificate manager: Disable export option for TPM-backed certs.

certificate manager: Disable export option for TPM-backed certs.

Add a separate boolean property to indicate that a client certificate
is hardware (TPM) backed. Certificate manager should disable the export
button for such certificates because there is no way to extract the
private key from the TPM.

BUG=126886
TEST=lumpy

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138314

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138595

[hshi1, 2012-05-21 21:03:38]

Dear Reviewers -- could you please comment on the Patch Set #1 for
(crbug.com/126886). Thanks!

I've added xiyuan@ to comment on the certificate manager change, and rsleevi@,
wtc@ for the changes in net/base, and estade@ for changes in
certificate_manager.js.

[Evan Stade, 2012-05-21 21:14:58]

options2 lgtm

http://codereview.chromium.org/10407072/diff/7001/chrome/browser/resources/op...
File chrome/browser/resources/options2/certificate_manager.js (right):

http://codereview.chromium.org/10407072/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/certificate_manager.js:119: var hardwareBacked
= !!data && data.hardware_backed;
you don't need the !! as far as I'm aware (here or above)

[xiyuan, 2012-05-21 21:17:26]

certificate_manager_model.cc lgtm

[hshi1, 2012-05-21 21:17:44]

http://codereview.chromium.org/10407072/diff/7001/chrome/browser/resources/op...
File chrome/browser/resources/options2/certificate_manager.js (right):

http://codereview.chromium.org/10407072/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/certificate_manager.js:119: var hardwareBacked
= !!data && data.hardware_backed;
I'm not really familiar with this JS and would prefer to leave this logic as-is.

On 2012/05/21 21:14:58, Evan Stade wrote:
> you don't need the !! as far as I'm aware (here or above)

[Ryan Sleevi, 2012-05-21 21:19:23]

My understanding is that chromeos supports multiple types of certificate export.

While we certainly cannot support PKCS#12-with-key export, we can and should
support PKCS#7/PKCS#12-without-key/PEM/DER export.

Rather than disabling the interface altogether, it seems like we should instead
try to export it without the key.

[hshi1, 2012-05-21 21:25:17]

Ryan - but are you fine with the addition of bool IsHardwareBacked(cert) method
in cert database?

Regardless of how TPM-backed certs should be handled (disable export altogether
or export without key), I think it is necessary to report the HW-backed flag to
JS.

Adding dkrahn@ -- I will defer the decision on how best to handle TPM-backed
certs to you.

[hshi1, 2012-05-21 21:30:44]

+ Greg to the discussion as suggested by Zel.

My understanding so far is that on chromeos it is not possible to export any
TPM-backed cert as the private key is protected by hardware. Please let me know
what you think is the best course of action (disable export button in UI, or
allow export a dummy cert without keys).

[dkrahn, 2012-05-21 21:34:01]

I like the idea of allowing export of public cert only as long as it's
clear to the user that the private key will not be exported.

Sumit, do you want to weigh in on this?

-Darren

On Mon, May 21, 2012 at 2:30 PM, <hshi@chromium.org> wrote:

> + Greg to the discussion as suggested by Zel.
>
> My understanding so far is that on chromeos it is not possible to export
> any
> TPM-backed cert as the private key is protected by hardware. Please let me
> know
> what you think is the best course of action (disable export button in UI,
> or
> allow export a dummy cert without keys).
>
>
http://codereview.chromium.**org/10407072/<http://codereview.chromium.org/104...
>

[Greg Spencer (Chromium), 2012-05-21 21:39:40]

I think I agree with Ryan, but maybe what we should do is allow export only of
certs that don't have a private key associated with them.  That way, the user
can't get something they don't expect, and I'm not sure how useful a user cert
would be without the private key.

[sumitg, 2012-05-21 21:42:35]

On Mon, May 21, 2012 at 2:34 PM, Darren Krahn <dkrahn@google.com> wrote:

> I like the idea of allowing export of public cert only as long as it's
> clear to the user that the private key will not be exported.
>
>
It's fine to allow export of non-hardware certs as Ryan mentioned earlier.

If we want to allow exporting of public cert whose privacy key is in
hardware, that's reasonable but we should
indicate that to the user in some way.



> Sumit, do you want to weigh in on this?
>
> -Darren
>
> On Mon, May 21, 2012 at 2:30 PM, <hshi@chromium.org> wrote:
>
>> + Greg to the discussion as suggested by Zel.
>>
>> My understanding so far is that on chromeos it is not possible to export
>> any
>> TPM-backed cert as the private key is protected by hardware. Please let
>> me know
>> what you think is the best course of action (disable export button in UI,
>> or
>> allow export a dummy cert without keys).
>>
>>
http://codereview.chromium.**org/10407072/<http://codereview.chromium.org/104...
>>
>
>

[hshi, 2012-05-21 21:49:12]

Would it be OK to land this patch first?

I would suggest to file a separate Feature bug to track support for
exporting public-key-only (PKCS#12-without-privatekey) because that part of
work seem a much larger effort.

Thanks,
Haixia

On Mon, May 21, 2012 at 2:42 PM, Sumit Gwalani <sumitg@google.com> wrote:

>
>
> On Mon, May 21, 2012 at 2:34 PM, Darren Krahn <dkrahn@google.com> wrote:
>
>> I like the idea of allowing export of public cert only as long as it's
>> clear to the user that the private key will not be exported.
>>
>>
> It's fine to allow export of non-hardware certs as Ryan mentioned earlier.
>
> If we want to allow exporting of public cert whose privacy key is in
> hardware, that's reasonable but we should
> indicate that to the user in some way.
>
>
>
>> Sumit, do you want to weigh in on this?
>>
>> -Darren
>>
>> On Mon, May 21, 2012 at 2:30 PM, <hshi@chromium.org> wrote:
>>
>>> + Greg to the discussion as suggested by Zel.
>>>
>>> My understanding so far is that on chromeos it is not possible to export
>>> any
>>> TPM-backed cert as the private key is protected by hardware. Please let
>>> me know
>>> what you think is the best course of action (disable export button in
>>> UI, or
>>> allow export a dummy cert without keys).
>>>
>>>
http://codereview.chromium.**org/10407072/<http://codereview.chromium.org/104...
>>>
>>
>>
>

[Ryan Sleevi, 2012-05-21 21:55:33]

On 2012/05/21 21:39:40, Greg Spencer (Chromium) wrote:
> I think I agree with Ryan, but maybe what we should do is allow export only of
> certs that don't have a private key associated with them.  That way, the user
> can't get something they don't expect, and I'm not sure how useful a user cert
> would be without the private key.

Quite useful, especially if I need to register my cert with some external
provider as my "This is me" cert (eg: WebID)

Certs without keys are what you give to IdPs-et-al to know a-priori who you are.
Certs with keys are what you use for backups.

[Greg Spencer (Chromium), 2012-05-21 21:59:51]

On 2012/05/21 21:55:33, Ryan Sleevi wrote:
> On 2012/05/21 21:39:40, Greg Spencer (Chromium) wrote:
> > I think I agree with Ryan, but maybe what we should do is allow export only
of
> > certs that don't have a private key associated with them.  That way, the
user
> > can't get something they don't expect, and I'm not sure how useful a user
cert
> > would be without the private key.
> 
> Quite useful, especially if I need to register my cert with some external
> provider as my "This is me" cert (eg: WebID)
> 
> Certs without keys are what you give to IdPs-et-al to know a-priori who you
are.
> Certs with keys are what you use for backups.

Duh.  Of course it's useful.  Sorry, wasn't thinking. :-)

[hshi, 2012-05-21 22:00:32]

I agree too, this is essentially the difference between "backup
(export-personal-cert)" and "export".

Question: For the UI change would you prefer we add a new button ("backup"
vs "export") to differentiate between the two cases, or make it an option
in the export-cert dialog?

In any case, I think the changes in net/base in the current Patch Set is
still needed: we would still need to pass this flag (IsHardwareBacked) to
the UI layer so that the appropriate message can be displayed to the user.
How about I land this as part 1 of several incremental changes then?

On Mon, May 21, 2012 at 2:55 PM, <rsleevi@chromium.org> wrote:

> On 2012/05/21 21:39:40, Greg Spencer (Chromium) wrote:
>
>> I think I agree with Ryan, but maybe what we should do is allow export
>> only of
>> certs that don't have a private key associated with them.  That way, the
>> user
>> can't get something they don't expect, and I'm not sure how useful a user
>> cert
>> would be without the private key.
>>
>
> Quite useful, especially if I need to register my cert with some external
> provider as my "This is me" cert (eg: WebID)
>
> Certs without keys are what you give to IdPs-et-al to know a-priori who
> you are.
> Certs with keys are what you use for backups.
>
>
http://codereview.chromium.**org/10407072/<http://codereview.chromium.org/104...
>

[Ryan Sleevi, 2012-05-21 22:08:32]

http://codereview.chromium.org/10407072/diff/7001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/certificate_manager_handler2.cc (right):

http://codereview.chromium.org/10407072/diff/7001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/certificate_manager_handler2.cc:42: static
const char kHardwareBackedId[] = "hardware_backed";
See other comments, but all instances of 'hardware_backed' should really be
replaced by 'extractable', since that's what you're really wanting to gate on.

The fact that it's hardware backed is orthogonal to the extractability of the
key.

In PKCS#11 terms, this is the "CKA_EXTRACTABLE" attribute.

PK11_ReadRawAttribute(PK11_TypePrivKey, |key_handle|, CKA_EXTRACABLE, ...) will
tell you this. Note that the NSS PKCS#12 functions should already be reading
this and handle it.

See
http://mxr.mozilla.org/security/source/security/manager/ssl/src/nsPKCS12Blob....
for the NSS PSM implementation. In particular,
http://mxr.mozilla.org/security/source/security/manager/ssl/src/nsPKCS12Blob....

http://codereview.chromium.org/10407072/diff/7001/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/10407072/diff/7001/net/base/cert_database_nss....
net/base/cert_database_nss.cc:282: bool CertDatabase::IsHardwareBacked(const
X509Certificate* cert) const {
I'd prefer this method not be in CertDatabase, because CertDatabase is meant to
live on the IO thread.

IsTPMTokenReady() and GetPrivateModule() are only meant to be called on the UI
thread.

The fact that this /is/ called on the UI thread doesn't make it any safer.

See http://crbug.com/125848 for my general thoughts on this.

This also isn't "accurate" for != ChromeOS. The 'correct' function to determine
if a slot is hardware backed is to call PK11_IsHW(). However, you're passing an
|X509Certificate*|, so the real 'correct' solution is to figure out where the
/key/ lives, not where the /cert/ lives.

If the /key/ lives on a /non-exportable/ token, then we shouldn't allow the key
to be exported.

If this is kept in ChromeOS-only code (outside of the net/ layer), then sure,
this approach is 'fine' because CrOS doesn't support smart cards, but if
intended to be generic, then it's not really.

[hshi1, 2012-05-21 22:28:35]

Ryan, thanks for your suggestions. I uploaded Patch Set #3 which removed the
changes in net/ layer.

Notice that for now chromeos cert manager code treats "hardware-backed" and
"non-extractable" as the same. This is exemplified with the first line of
CertificateManagerHandler::ImportPersonalSlotUnlocked() where the extractability
of a cert is based on whether the cert is imported into a hardware module.

http://codereview.chromium.org/10407072/diff/7001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/certificate_manager_handler2.cc (right):

http://codereview.chromium.org/10407072/diff/7001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/certificate_manager_handler2.cc:42: static
const char kHardwareBackedId[] = "hardware_backed";
On 2012/05/21 22:08:32, Ryan Sleevi wrote:
> See other comments, but all instances of 'hardware_backed' should really be
> replaced by 'extractable', since that's what you're really wanting to gate on.
> 
> The fact that it's hardware backed is orthogonal to the extractability of the
> key.
> 
> In PKCS#11 terms, this is the "CKA_EXTRACTABLE" attribute.
> 
> PK11_ReadRawAttribute(PK11_TypePrivKey, |key_handle|, CKA_EXTRACABLE, ...)
will
> tell you this. Note that the NSS PKCS#12 functions should already be reading
> this and handle it.
> 
> See
>
http://mxr.mozilla.org/security/source/security/manager/ssl/src/nsPKCS12Blob....
> for the NSS PSM implementation. In particular,
>
http://mxr.mozilla.org/security/source/security/manager/ssl/src/nsPKCS12Blob....

Done.

http://codereview.chromium.org/10407072/diff/7001/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/10407072/diff/7001/net/base/cert_database_nss....
net/base/cert_database_nss.cc:282: bool CertDatabase::IsHardwareBacked(const
X509Certificate* cert) const {
On 2012/05/21 22:08:32, Ryan Sleevi wrote:
> I'd prefer this method not be in CertDatabase, because CertDatabase is meant
to
> live on the IO thread.
> 
> IsTPMTokenReady() and GetPrivateModule() are only meant to be called on the UI
> thread.
> 
> The fact that this /is/ called on the UI thread doesn't make it any safer.
> 
> See http://crbug.com/125848 for my general thoughts on this.
> 
> This also isn't "accurate" for != ChromeOS. The 'correct' function to
determine
> if a slot is hardware backed is to call PK11_IsHW(). However, you're passing
an
> |X509Certificate*|, so the real 'correct' solution is to figure out where the
> /key/ lives, not where the /cert/ lives.
> 
> If the /key/ lives on a /non-exportable/ token, then we shouldn't allow the
key
> to be exported.
> 
> If this is kept in ChromeOS-only code (outside of the net/ layer), then sure,
> this approach is 'fine' because CrOS doesn't support smart cards, but if
> intended to be generic, then it's not really.

Done.

[Ryan Sleevi, 2012-05-21 22:32:47]

Thanks hshi.

This works for me (for now :-p). I'd like to see us able to do the Right Thing,
especially on Linux, but as a short-term fix for CrOS, I think this is the least
of the available evils.

Can you file a bug to
1) Ensure that we're checking if the key is extractable (if we need to
disable/replace UI elements)
2) That we allow the user to omit exporting the key if the key is
non-extractable
3) That the above two happen on a thread other than the UI thread ;-)

The reason for #3 is that #1 or #2 may block the thread they're invoking on.
Determining the private key for a cert may involve waiting for the TPM to log in
and load the key (which may take several seconds), and extracting the cert
and/or key from a hardware module may further take several hundred ms
(particularly on Linux - CrOS keeps the certs cached in memory & disk)

[hshi, 2012-05-21 22:43:56]

I have filed (crbug.com/129077) to track this issue.

Thanks,
Haixia

On Mon, May 21, 2012 at 3:32 PM, <rsleevi@chromium.org> wrote:

> Thanks hshi.
>
> This works for me (for now :-p). I'd like to see us able to do the Right
> Thing,
> especially on Linux, but as a short-term fix for CrOS, I think this is the
> least
> of the available evils.
>
> Can you file a bug to
> 1) Ensure that we're checking if the key is extractable (if we need to
> disable/replace UI elements)
> 2) That we allow the user to omit exporting the key if the key is
> non-extractable
> 3) That the above two happen on a thread other than the UI thread ;-)
>
> The reason for #3 is that #1 or #2 may block the thread they're invoking
> on.
> Determining the private key for a cert may involve waiting for the TPM to
> log in
> and load the key (which may take several seconds), and extracting the cert
> and/or key from a hardware module may further take several hundred ms
> (particularly on Linux - CrOS keeps the certs cached in memory & disk)
>
>
http://codereview.chromium.**org/10407072/<http://codereview.chromium.org/104...
>

[commit-bot: I haz the power, 2012-05-21 22:53:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10407072/8005

[commit-bot: I haz the power, 2012-05-22 03:12:24]

Try job failure for 10407072-8005 (retry) on win_rel for steps "base_unittests,
sync_unit_tests".
It's a second try, previously, steps "base_unittests, sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-22 03:17:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10407072/8005

[commit-bot: I haz the power, 2012-05-22 07:57:32]

Try job failure for 10407072-8005 (retry) (previous was lost) on win_rel for
steps "base_unittests, sync_unit_tests".
It's a second try, previously, steps "base_unittests, browser_tests,
sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-22 07:59:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10407072/8005

[commit-bot: I haz the power, 2012-05-22 11:34:31]

Try job failure for 10407072-8005 (retry) (retry) on win_rel for steps
"base_unittests, sync_unit_tests".
It's a second try, previously, steps "base_unittests, sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-22 13:23:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10407072/8005

[commit-bot: I haz the power, 2012-05-22 15:24:07]

Try job failure for 10407072-8005 (retry) on win_rel for steps "base_unittests,
sync_unit_tests".
It's a second try, previously, steps "base_unittests, sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-22 16:20:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10407072/8005

[commit-bot: I haz the power, 2012-05-22 17:41:59]

Try job failure for 10407072-8005 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[wtc, 2012-05-22 23:43:46]

Patch set 3 LGTM. I suggest some comment changes.

https://chromiumcodereview.appspot.com/10407072/diff/8005/chrome/browser/cert...
File chrome/browser/certificate_manager_model.h (right):

https://chromiumcodereview.appspot.com/10407072/diff/8005/chrome/browser/cert...
chrome/browser/certificate_manager_model.h:103: // IsHardwareBacked returns true
if |cert| is hardware backed.

Please document that this function is only implemented for
Chrome OS and always returns false on other platforms.

https://chromiumcodereview.appspot.com/10407072/diff/8005/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/certificate_manager_handler2.cc (right):

https://chromiumcodereview.appspot.com/10407072/diff/8005/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/certificate_manager_handler2.cc:981:
!certificate_manager_model_->IsHardwareBacked(cert));

Please add a TODO comment to note that this should be
determined by testing for the PKCS #11 CKA_EXTRACTABLE
attribute.  We may need to use the NSS function
PK11_ReadRawAttribute to do that.

[hshi1, 2012-05-23 00:03:29]

https://chromiumcodereview.appspot.com/10407072/diff/8005/chrome/browser/cert...
File chrome/browser/certificate_manager_model.h (right):

https://chromiumcodereview.appspot.com/10407072/diff/8005/chrome/browser/cert...
chrome/browser/certificate_manager_model.h:103: // IsHardwareBacked returns true
if |cert| is hardware backed.
On 2012/05/22 23:43:46, wtc wrote:
> 
> Please document that this function is only implemented for
> Chrome OS and always returns false on other platforms.

Done.

https://chromiumcodereview.appspot.com/10407072/diff/8005/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/certificate_manager_handler2.cc (right):

https://chromiumcodereview.appspot.com/10407072/diff/8005/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/certificate_manager_handler2.cc:981:
!certificate_manager_model_->IsHardwareBacked(cert));
On 2012/05/22 23:43:46, wtc wrote:
> 
> Please add a TODO comment to note that this should be
> determined by testing for the PKCS #11 CKA_EXTRACTABLE
> attribute.  We may need to use the NSS function
> PK11_ReadRawAttribute to do that.

Done.

[commit-bot: I haz the power, 2012-05-23 00:04:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10407072/10009

[commit-bot: I haz the power, 2012-05-23 04:35:25]

Try job failure for 10407072-10009 (retry) on win_rel for step
"sync_unit_tests".
It's a second try, previously, step "sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-23 04:41:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10407072/10009

[commit-bot: I haz the power, 2012-05-23 07:04:45]

Try job failure for 10407072-10009 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-05-23 13:20:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10407072/10009

[commit-bot: I haz the power, 2012-05-23 16:29:55]

Try job failure for 10407072-10009 (retry) on win_rel for step
"sync_unit_tests".
It's a second try, previously, step "sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-23 18:16:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/10407072/10009

[commit-bot: I haz the power, 2012-05-23 20:45:09]

Change committed as 138595