Mark untrusted certificates as such in Linux UI.

Mark untrusted certificates as such in Linux UI.

Some certificates are included in the certificate database in order to
explicitly mark them as untrusted. In our current UI this isn't
indicated and there's at least one, clearly fraudulent, CA in the list
that looks like all the rest.

This change causes these explicitly untrusted CAs to be marked with a
red badge next to them.

BUG=79549
TEST=Open the certificates dialog on Linux/ChromeOS and look for untrusted CA certificates.


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

[agl, 2011-06-27 23:37:55]

wtc: please note that this causes several CAs to be marked as untrusted on my
Ubuntu box including some under Entrust and everything under "IPS".

I believe that the code is correct however and that these certificates really
don't have any trust flags in the NSS database.

[wtc, 2011-06-28 23:57:39]

mattm: please also review this CL.

agl: I will need to do some research to find out if the
CertDatabase::IsUntrusted() code is correct.

[wtc, 2011-06-29 00:30:45]

http://codereview.chromium.org/7272014/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7272014/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:3338: <message
name="IDS_CERT_MANAGER_UNTRUSTED" desc="This text is displayed next to untrusted
certificates in a red box. Untrusted certificates are sometimes added in order
to explicitly disable them. Thus, they will be listed but it's important that
the user note that serve a different purpose from the rest.">
Should
    that serve a different purpose
be
    they serve a different purpose
?

http://codereview.chromium.org/7272014/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:3339: untrusted
Should we capitalize "untrusted"?

http://codereview.chromium.org/7272014/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

http://codereview.chromium.org/7272014/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:36: static const
char kIsUntrusted[] = "untrusted";
kIsUntrusted => kUntrustedId

to follow the naming convention of the other char constants.

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

http://codereview.chromium.org/7272014/diff/1/net/base/cert_database_nss.cc#n...
net/base/cert_database_nss.cc:249: nsstrust.objectSigningFlags == 0;
I believe this is correct only for root certs.

(Note: the trusts for the IPS roots were removed in
https://bugzilla.mozilla.org/show_bug.cgi?id=530853.)

For non-root certs, I think we will need code similar to
the CERT_CheckLeafTrust function in Bob Relyea's patch
https://bug642503.bugzilla.mozilla.org/attachment.cgi?id=527127
in the NSS bug
https://bugzilla.mozilla.org/show_bug.cgi?id=642503

Specifically, we need to test if the
CERTDB_TERMINAL_RECORD bit (formerly named
CERTDB_VALID_PEER) is set.

nsstrust.sslFlags == 0 for a non-root cert actually means it
derives its trust from its CA.

Two important test cases for non-root certs are:
https://bugzilla.mozilla.org/show_bug.cgi?id=471715
https://bugzilla.mozilla.org/show_bug.cgi?id=642815

I will ask Bob Relyea (an NSS developer) about this.

[mattm, 2011-06-29 01:12:07]

http://codereview.chromium.org/7272014/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/certificate_tree.js (right):

http://codereview.chromium.org/7272014/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/certificate_tree.js:29: badge.textContent =
(new LocalStrings()).getString("badgeUntrusted");
localStrings.getString

http://codereview.chromium.org/7272014/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/certificate_tree.js:35:
badge.style.paddingRight = '1px'
Create a certificate_tree.css and put the style stuff there, then set the
element's class.  (Include the css from
chrome/browser/resources/options/options.html, next to the
certificate_manager.css one.)

http://codereview.chromium.org/7272014/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/certificate_tree.js:36:
treeItem.labelElement.appendChild(badge)
From a UI perspective I wonder if it would look better with the badge before the
cert name instead of after it?  Or maybe  a red strike-through?  Well, I don't
really want to bikeshed on the UI details, just with a long cert name the
appended badge might require horizontal scrolling to see.

http://codereview.chromium.org/7272014/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

http://codereview.chromium.org/7272014/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:352:
localized_strings->SetString("badgeUntrusted",
maybe badgeCertUntrusted  (the localized strings namespace is shared among all
the options dialogs, so being more specific is good)

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

http://codereview.chromium.org/7272014/diff/1/net/base/cert_database_nss.cc#n...
net/base/cert_database_nss.cc:249: nsstrust.objectSigningFlags == 0;
Maybe we should only check the sslFlags?  If for some reason a cert is untrusted
for SSL but still trusted for other things we'd probably still want to display
the badge since we only use SSL trust, right?

[agl, 2011-06-29 14:31:11]

Have addressed points from review (thanks!). But I'm not going to commit until
we have a better idea of what 'untrusted' means in terms of NSS flags.

Cheers

http://codereview.chromium.org/7272014/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7272014/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:3338: <message
name="IDS_CERT_MANAGER_UNTRUSTED" desc="This text is displayed next to untrusted
certificates in a red box. Untrusted certificates are sometimes added in order
to explicitly disable them. Thus, they will be listed but it's important that
the user note that serve a different purpose from the rest.">
On 2011/06/29 00:30:45, wtc wrote:
> Should
>     that serve a different purpose
> be
>     they serve a different purpose
> ?

Done.

http://codereview.chromium.org/7272014/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:3339: untrusted
On 2011/06/29 00:30:45, wtc wrote:
> Should we capitalize "untrusted"?

Done.

http://codereview.chromium.org/7272014/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/certificate_tree.js (right):

http://codereview.chromium.org/7272014/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/certificate_tree.js:29: badge.textContent =
(new LocalStrings()).getString("badgeUntrusted");
On 2011/06/29 01:12:07, mattm wrote:
> localStrings.getString

Done.

http://codereview.chromium.org/7272014/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/certificate_tree.js:35:
badge.style.paddingRight = '1px'
On 2011/06/29 01:12:07, mattm wrote:
> Create a certificate_tree.css and put the style stuff there, then set the
> element's class.  (Include the css from
> chrome/browser/resources/options/options.html, next to the
> certificate_manager.css one.)

Done.

http://codereview.chromium.org/7272014/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/certificate_tree.js:36:
treeItem.labelElement.appendChild(badge)
On 2011/06/29 01:12:07, mattm wrote:
> From a UI perspective I wonder if it would look better with the badge before
the
> cert name instead of after it?  Or maybe  a red strike-through?  Well, I don't
> really want to bikeshed on the UI details, just with a long cert name the
> appended badge might require horizontal scrolling to see.

I agree that there's a danger that a long cert name could push the badge past
the right edge. I've moved the badge to the beginning.

http://codereview.chromium.org/7272014/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

http://codereview.chromium.org/7272014/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:36: static const
char kIsUntrusted[] = "untrusted";
On 2011/06/29 00:30:45, wtc wrote:
> kIsUntrusted => kUntrustedId
> 
> to follow the naming convention of the other char constants.

Done.

http://codereview.chromium.org/7272014/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:352:
localized_strings->SetString("badgeUntrusted",
On 2011/06/29 01:12:07, mattm wrote:
> maybe badgeCertUntrusted  (the localized strings namespace is shared among all
> the options dialogs, so being more specific is good)

Done.

[wtc, 2011-09-12 23:57:04]

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

http://codereview.chromium.org/7272014/diff/7001/net/base/cert_database_nss.c...
net/base/cert_database_nss.cc:251: nsstrust.objectSigningFlags == 0;
agl: sorry for the very late reply.

I got the answers from Bob Relyea within two weeks, but I
forgot post them here.

Here is the code we need to write.

1. Some code to define CERTDB_TERMINAL_RECORD for
NSS <= 3.12.x.

// In NSS 3.13, CERTDB_VALID_PEER was renamed CERTDB_TERMINAL_RECORD.  So
// we use the new name of the macro.
#if !defined(CERTDB_TERMINAL_RECORD)
#define CERTDB_TERMINAL_RECORD CERTDB_VALID_PEER
#endif

2. The test on lines 249-251 should be replaced with the
following:

  unsigned int flags = SEC_GET_TRUST_FLAGS(&nsstrust, trustSSL);
  if ((flags & CERTDB_TERMINAL_RECORD) &&
      ((flags & (CERTDB_TRUSTED_CA | CERTDB_TRUSTED)) == 0)) {
    // In a terminal trust record, three bits may be set: CERTDB_VALID_CA,
    // CERTDB_TRUSTED_CA, and CERTDB_TRUSTED.  The CERTDB_VALID_CA bit is
    // irrelevant to distrust, so we don't test that bit.
    return true;
  }
  // Repeat for email and code signing.
  flags = SEC_GET_TRUST_FLAGS(&nsstrust, trustEmail);
  if ((flags & CERTDB_TERMINAL_RECORD) &&
      ((flags & (CERTDB_TRUSTED_CA | CERTDB_TRUSTED)) == 0)) {
    return true;
  }
  flags = SEC_GET_TRUST_FLAGS(&nsstrust, trustObjectSigning);
  if ((flags & CERTDB_TERMINAL_RECORD) &&
      ((flags & (CERTDB_TRUSTED_CA | CERTDB_TRUSTED)) == 0)) {
    return true;
  }
  return false;

Two Notes
- This handles the explicit distrust case, which is most
  important to show in the UI.  A test similar to your code
  would be necessary to handle a root CA that was added
  without any trust (as opposed to with explicit distrust).
  I omitted that test here because we don't have any such
  root CA cert in NSS.
- Since Chrome doesn't do email or code signing, we can
  omit the trustEmail and trustObjectSigning code.  The
  original NSS code uses a for loop to test the three
  kinds of trust, but C++ doesn't allow us to increment
  an enum variable, so I had to unroll the for loop.

[agl, 2011-09-13 16:53:50]

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

http://codereview.chromium.org/7272014/diff/7001/net/base/cert_database_nss.c...
net/base/cert_database_nss.cc:251: nsstrust.objectSigningFlags == 0;
On 2011/09/12 23:57:04, wtc wrote:
> - This handles the explicit distrust case, which is most
>   important to show in the UI.  A test similar to your code
>   would be necessary to handle a root CA that was added
>   without any trust (as opposed to with explicit distrust).
>   I omitted that test here because we don't have any such
>   root CA cert in NSS.

At least on my Ubuntu system I do have a distrusted CA: MD5 Collisions Inc. Can
you provide the code for detecting such a CA. At that point, I think the patch
will be ready.

Thanks.

[wtc, 2011-09-14 22:00:53]

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

http://codereview.chromium.org/7272014/diff/7001/net/base/cert_database_nss.c...
net/base/cert_database_nss.cc:251: nsstrust.objectSigningFlags == 0;
On 2011/09/13 16:53:50, agl wrote:
>
> At least on my Ubuntu system I do have a distrusted CA: MD5 Collisions Inc.
Can
> you provide the code for detecting such a CA. At that point, I think the patch
> will be ready.

Yes.  Unfortunately the MD5 Collisions Inc. CA in the
NSS trusted CA file is *not* explicitly distrusted in
NSS 3.12.x.  It has "unknown" trust, which is mapped to
all 0 trust flags and means we must verify the cert as
usual.

I heard that certificate is a corrupted copy and is
meant to prevent the real MD5 Collisions Inc. CA cert
from being imported into NSS.  (NSS would reject it
because it has the same issuer and serial number as
the corrupted copy.)  The corrupted copy would fail
signature verification during certificate verification.
I need more time to verify the cert is indeed corrupted.

This is the code I have written so far (better than the
code I gave you yesterday):

// In NSS 3.13, CERTDB_VALID_PEER was renamed CERTDB_TERMINAL_RECORD.  So
// we use the new name of the macro.
#if !defined(CERTDB_TERMINAL_RECORD)
#define CERTDB_TERMINAL_RECORD CERTDB_VALID_PEER
#endif

...

  const unsigned int kTrusted = CERTDB_TRUSTED_CA | CERTDB_TRUSTED;

  // Handle explicitly distrusted certificates.
  unsigned int flags = SEC_GET_TRUST_FLAGS(&nsstrust, trustSSL);
  if ((flags & CERTDB_TERMINAL_RECORD) && (flags & kTrusted) == 0) {
    // In a terminal trust record, three bits may be set: CERTDB_VALID_CA,
    // CERTDB_TRUSTED_CA, and CERTDB_TRUSTED.  The CERTDB_VALID_CA bit is
    // irrelevant to distrust, so we don't test that bit.
    return true;
  }
  // Repeat for email and code signing.
  flags = SEC_GET_TRUST_FLAGS(&nsstrust, trustEmail);
  if ((flags & CERTDB_TERMINAL_RECORD) && (flags & kTrusted) == 0)
    return true;
  flags = SEC_GET_TRUST_FLAGS(&nsstrust, trustObjectSigning);
  if ((flags & CERTDB_TERMINAL_RECORD) && (flags & kTrusted) == 0)
    return true;

  // Self-signed certificates that don't have any trust bits set are
  // untrusted.  Other certificates that don't have any trust bits set
  // may still be trusted, if they chain up to a trust anchor.
  if (CERT_CompareName(&cert->os_cert_handle()->issuer,
                       &cert->os_cert_handle()->subject) == SECEqual) {
    return (nsstrust.sslFlags & kTrusted) == 0 &&
           (nsstrust.emailFlags & kTrusted) == 0 &&
           (nsstrust.objectSigningFlags & kTrusted) == 0;
  }
  return false;

You can simplify it further.  For example,
SEC_GET_TRUST_FLAGS(&nsstrust, trustSSL) is the fancy way
to say nsstrust.sslFlags.

I tested this patch with my code.  The only problem I see
is that it doesn't update the "Untrusted" badge as soon as
I change the trust bits in the Certificate Manager UI;
the new "Untrusted" status is only reflected after I restart
Chrome.  But you don't need to fix that bug in this CL.

[wtc, 2011-09-14 22:58:53]

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

http://codereview.chromium.org/7272014/diff/7001/net/base/cert_database_nss.c...
net/base/cert_database_nss.cc:251: nsstrust.objectSigningFlags == 0;
I examined the MD5 Collisions Inc. CA certficate in NSS.
It differs from the real MD5 Collisions Inc. CA certficate
in two ways.

1. The serial number changed from 65 (0x41) to 66 (0x42).
So what I said earlier about preventing the real MD5
Collision Inc. CA certificate from being imported into
NSS was wrong.  The real certificate can be imported,
but NSS won't use it because of the next change.

2. The last digit in the not-before and not-after UTC
times changed from '0' (0x30) to '1' (0x31).  Given two
certificates with the same subject, NSS favors the newer
certificate.

So, NSS will use the newer (corrupted) cert, and certificate
verification will fail during signature verification.

So, the code I gave you is the best we can do.

In NSS 3.12.x, the (corrupted) MD5 Collisions Inc. CA
cert won't have the "Untrusted" badge.

In the upcoming NSS 3.13, we explicitly distrust the
(corrupted) MD5 Collisions Inc. CA cert, so it will have
the "Untrusted" badge.

The reason we don't explicitly distrust the MD5 CA cert
in NSS 3.12.x is that NSS 3.12.x cannot distrust intermediate
CA certs.  This bug is fixed in NSS 3.13.

[agl, 2011-09-20 16:57:47]

wtc: could you take another look?

I've implemented the code as you suggested. On my desktop it still flags some
odd certificates (i.e. "UTN-USERFirst-Network Applications") but it appears that
they really may be untrusted by NSS based on this code.

[wtc, 2011-09-20 17:18:04]

On 2011/09/20 16:57:47, agl wrote:
> wtc: could you take another look?

I will review this CL later today.

> I've implemented the code as you suggested. On my desktop it still flags some
> odd certificates (i.e. "UTN-USERFirst-Network Applications") but it appears
that
> they really may be untrusted by NSS based on this code.

Yes.  For example, in NSS 3.12.8, "UTN-USERFirst-Network Applications"
has the following trust flags:
CKA_TRUST_SERVER_AUTH CK_TRUST CKT_NETSCAPE_TRUST_UNKNOWN
CKA_TRUST_EMAIL_PROTECTION CK_TRUST CKT_NETSCAPE_TRUST_UNKNOWN
CKA_TRUST_CODE_SIGNING CK_TRUST CKT_NETSCAPE_TRUST_UNKNOWN

That root was disabled this way in NSS bug 554334:
https://bugzilla.mozilla.org/show_bug.cgi?id=554334

So it is correct for your CL to flags the root as untrusted.

[wtc, 2011-09-21 17:05:33]

LGTM.  Please note the last comment (marked with "IMPORTANT")
below.

http://codereview.chromium.org/7272014/diff/13001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7272014/diff/13001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:3356: <message
name="IDS_CERT_MANAGER_UNTRUSTED" desc="This text is displayed next to untrusted
certificates in a red box. Untrusted certificates are sometimes added in order
to explicitly disable them. Thus, they will be listed but it's important that
the user note that they serve a different purpose from the rest.">
Nit: explicitly disable => explicitly distrust ?

http://codereview.chromium.org/7272014/diff/13001/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/7272014/diff/13001/net/base/cert_database.h#ne...
net/base/cert_database.h:163: // certificates are remembered for the specific
purpose of rejecting them.
Nit: remembered => stored in the database ?

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

http://codereview.chromium.org/7272014/diff/13001/net/base/cert_database_nss....
net/base/cert_database_nss.cc:253: // handle explicitly distrusted certificates.
Nit: capitalize "handle".

http://codereview.chromium.org/7272014/diff/13001/net/base/cert_database_nss....
net/base/cert_database_nss.cc:261: }
IMPORTANT: Did you omit the checking of distrust for email and
code signing intentionally?  If so, you should add a comment,
and change the boolean expression on lines 268-270 to match
(which would be just (nsstrust.sslFlags & kTrusted) == 0.

[agl, 2011-09-21 17:53:53]

http://codereview.chromium.org/7272014/diff/13001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7272014/diff/13001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:3356: <message
name="IDS_CERT_MANAGER_UNTRUSTED" desc="This text is displayed next to untrusted
certificates in a red box. Untrusted certificates are sometimes added in order
to explicitly disable them. Thus, they will be listed but it's important that
the user note that they serve a different purpose from the rest.">
On 2011/09/21 17:05:33, wtc wrote:
> Nit: explicitly disable => explicitly distrust ?

Done.

http://codereview.chromium.org/7272014/diff/13001/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/7272014/diff/13001/net/base/cert_database.h#ne...
net/base/cert_database.h:163: // certificates are remembered for the specific
purpose of rejecting them.
On 2011/09/21 17:05:33, wtc wrote:
> Nit: remembered => stored in the database ?

Done.

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

http://codereview.chromium.org/7272014/diff/13001/net/base/cert_database_nss....
net/base/cert_database_nss.cc:261: }
On 2011/09/21 17:05:33, wtc wrote:
> IMPORTANT: Did you omit the checking of distrust for email and
> code signing intentionally?  If so, you should add a comment,
> and change the boolean expression on lines 268-270 to match
> (which would be just (nsstrust.sslFlags & kTrusted) == 0.

I did deliberately omit the tests for email and code signing. Just because a
terminal record isn't trusted for email signing doesn't mean that it's
distrusted for SSL, which is what we're interested in here.

But nor do I really want to adjust the code below to match because, in that
case, self-signed certs which are only valid for email will be marked as
untrusted.

Please see if you like the latest revision of this code any better.

[wtc, 2011-09-21 19:01:13]

Review comment on Patch Set 4:

Everything is good except for the following:

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

http://codereview.chromium.org/7272014/diff/17002/net/base/cert_database_nss....
net/base/cert_database_nss.cc:265: return has_no_trust_flags;
The boolean expression has_no_trust_flags is wrong for
explicit distrust.

For explicit distrust, we need to use || instead of &&.
That is, if a cert is only explicitly distrusted for SSL,
we still want to flag it as untrusted, agreed?

[wtc, 2011-09-21 21:49:21]

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

http://codereview.chromium.org/7272014/diff/17002/net/base/cert_database_nss....
net/base/cert_database_nss.cc:261: if (flags & CERTDB_TERMINAL_RECORD) {
Also the CERTDB_TERMINAL_RECORD bit needs to be tested
in nsstrust.sslFlags, nsstrust.emailFlags, and
nsstrust.objectSigningFlags separately.  (There could be
three terminal trust records.)

You can just revert to the CertDatabase::IsUntrusted logic
in Patch Set 3.

[agl, 2011-09-21 22:23:13]

On Wed, Sep 21, 2011 at 5:49 PM,  <wtc@chromium.org> wrote:
>
http://codereview.chromium.org/7272014/diff/17002/net/base/cert_database_nss....
> net/base/cert_database_nss.cc:261: if (flags & CERTDB_TERMINAL_RECORD) {
> Also the CERTDB_TERMINAL_RECORD bit needs to be tested
> in nsstrust.sslFlags, nsstrust.emailFlags, and
> nsstrust.objectSigningFlags separately.  (There could be
> three terminal trust records.)

I'm afraid that I clearly have no understanding of these trust flags.

If we do ||, not &&, then roots which are not valid for email or
object signing would be marked as untrusted, even if they are
perfectly good for SSL. Half of the Verisign roots as untrusted on
this basis because they are only valid for email or "status responder"
(OCSP?)

If we test only sslFlags == 0, we still mark half of the Verisign
roots as untrusted.

Does the TERMINAL_RECORD flag mark a trust root? If so, then I agree
that a cert which is marked as a terminal record for any type is one
that we want to test for trust flags. I've updated the CL on this
basis and checked that the set of certificates marked as untrusted is
still sane.


Cheers

AGL

[wtc, 2011-09-21 22:40:42]

I'm sorry this is complicated.  I explain it below.

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

http://codereview.chromium.org/7272014/diff/18005/net/base/cert_database_nss....
net/base/cert_database_nss.cc:246: CERTCertTrust nsstrust;
The CERTCertTrust structure contains three trust records:
sslFlags, emailFlags, and objectSigningFlags.  The three
trust records are independent of each other.

If the CERTDB_TERMINAL_RECORD bit in a trust record is set,
then that trust record is a terminal record.  A terminal
record is used for explicit trust and distrust of an
end-entity or intermediate CA cert.

In a terminal record, if neither CERTDB_TRUSTED_CA nor
CERTDB_TRUSTED is set, then the terminal record means
explicit distrust.  On the other hand, if the terminal
record has either CERTDB_TRUSTED_CA or CERTDB_TRUSTED bit
set, then the terminal record means explicit trust.

For a root CA, the trust record does not have
the CERTDB_TERMINAL_RECORD bit set.

So the logic I suggested before was:

1. Examine the three trust records of the cert in turn.
If any of them is a terminal record for explicit distrust,
then flag the cert as untrusted.

(A variant of this logic is to examine just the SSL trust
record because Chrome doesn't care about email and code
signing.)

Since the three trust records are independent, the
check of the CERTDB_TERMINAL_RECORD bit and the kTrusted
bits must be done separately for each of the three trust
records.  They cannot be combined as you did below.

2. For a self-signed root cert, flag it as untrusted only
if we do not trust it for any purpose (i.e., if all three
trust records have no trust bits).

[agl, 2011-09-22 16:41:51]

On Wed, Sep 21, 2011 at 6:40 PM,  <wtc@chromium.org> wrote:
> I'm sorry this is complicated.  I explain it below.

Thanks. PTAL. Hopefully I got it right this time!


Cheers

AGL

[wtc, 2011-09-22 17:15:21]

LGTM on Patch Set 6.  Just one nit below.

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

http://codereview.chromium.org/7272014/diff/20001/net/base/cert_database_nss....
net/base/cert_database_nss.cc:269: * the CERTDB_TERMINAL_RECORD bit set. */
Please use C++ style comments.  This file is a Chromium C++
file.

[commit-bot: I haz the power, 2011-09-22 21:59:34]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/7272014/26001

[commit-bot: I haz the power, 2011-09-22 23:54:39]

Change committed as 102402