642503 - Generic blacklisting mechanism for bogus certs (NSS trust module)

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Daniel Veditz [:dveditz]]

+++ This bug was initially created as a clone of Bug #642395 +++

The NSS team agrees the bug 642395 approach is fine for a one-off emergency release but that it should be made a more generic feature and built into NSS itself rather than only in PSM. This bug is for moving that into NSS for all cert verification, not just SSL checks.

Bug 642395 uses only issuer+serial which works fine for certs mistakenly issued by a legit CA: they will not reuse serial numbers. A hacker-issued cert, however, could mimic any issuer+serial it wanted in some cases (e.g. social engineering attack "go ahead and set an exception for my self-signed cert"). Trying to block such a cert on issuer+serial could result in also killing some legitimate cert somewhere (which a clever hacker might pick for maximal collateral damage). Bob wanted to see if we could also include a cert hash in the mechanism. Downside, we might not always be able to get the hash depending on how we found out about the compromised cert.

Don't know if this can make NSS 3.12.10 at this point, but there's no 3.12.11 to set as the target milestone. We probably want to do this relatively soon.

Comment 1

[Gervase Markham [:gerv]]

I assume this bug can be made public after bug 642395 is?

I think that whatever we use to identify certs has to be calculable using only the public part as served by the server, and as kept by any issuing CA. Under what circumstances might we not be able to get a cert hash? What options do we have if we are trying to block a cert or certs we don't know the details of?

Gerv

Comment 2

[Daniel Veditz [:dveditz]]

(In reply to comment #1)
> I assume this bug can be made public after bug 642395 is?

Yes.

Comment 3

[Robert Relyea]

NSS Trust

For historical reasons, there are two different representations of NSS trust values.

	Part one, The original trust bits associated with the NSS Certificate:

The first is the original trust bits set up from day one, and which is still the one presented to the user, and used by the the certificate validation code when validating certificates. This trust record has three different elements:

sslFlags
emailFlags
objectSigningFlags

All the flags take the same set of bits:

#define CERTDB_VALID_PEER       (1<<0)
#define CERTDB_TRUSTED          (1<<1)
#define CERTDB_VALID_CA         (1<<3)
#define CERTDB_TRUSTED_CA       (1<<4) /* trusted for issuing server certs */

In addition sslFlags may have the following field. I'll discuss them separately.
#define CERTDB_TRUSTED_CLIENT_CA (1<<7) /* trusted for issuing client certs */
#define CERTDB_GOVT_APPROVED_CA (1<<9) /* can do strong crypto in export ver */

The following bits are not used in validation, and in general are generated on the fly.
Only CERTDB_USER is visible in any UI operation.
#define CERTDB_USER             (1<<6)
#define CERTDB_SEND_WARN        (1<<2)
#define CERTDB_NS_TRUSTED_CA    (1<<5)
#define CERTDB_INVISIBLE_CA     (1<<8) /* don't show in UI */


Let's take the validation in the most basic cases:


Processing the Leaf cert:

1) The trust does not exist:
   If no trust record exists, the cert must stand on it's own. We will continue processing, trying to find a chain that validates to a trusted CA.

2) The trust does exist, but none of the flags are on:
   This is handled the same as not trust record existing.

3) CERTDB_VALID_PEER and CERT_TRUSTED is on: 
   This certificate is explicitly trusted for the purpose. No additional cert chain checks will occur.

4) CERTDB_VALID_PEER is set but CERT_TRUSTED is not:
   This certificate is explicitly *NOT* trusted for the purpose. No addional cert chain checks will occur. (NOTE:Email currently ignores this case and treats it as case 1).

5) All other Leaf cert cases.
   Handled the same as case 1.

Processing a CA cert:

6) CERTDB_TRUSTED_CA is on:
   Processing ends and the original cert we were checking is accepted for it's purpose.

7) CERTDB_VALID_CA is on:
   Normal checks on to make sure this certificate is an actual CA is skipped. The certificate is neither explicitly trusted or distrusted. processing continues as in case 7.

8) All other cases:
   Processing continues looking for the next cert in the chain. It only ends when it reaches a self-signed certificate. If the current cert is self-signed, then the chain is rejected.

Currently there is not way to explicitly distrust and email cert leaf cert, an ocsp responder cert, or an intermedia CA cert. SSL certs, Code Signing Certs, and CA certs can be distrusted. The former by having the CERT_VALID_PEER bit on, the latter by not having the CERTDB_TRUSTED_CA bit on.

Processing of CERTDB_TRUSTED_CLIENT_CA and CERTDB_GOVT_APPROVED_CA

CERTDB_GOVT_APPROVED_CA is no longer processes. It allowed export grade crypto clients to set up to domestic grade crypto. NSS no longer ships in export grade, so all connections are make at full strength.

CERTDB_TRUSTED_CLIENT_CA is required in step 7 instead of CERTDB_VALID_CA when the ssl server is verifying the ssl client's certificate.

One final point. PSM treats these flags as 'checkboxes', I believe the check box turns on CERTDB_VALID_CA. When the check box is turned off, I believe all the bits are turned off. It is likely the checkboxes do different things to different certs.

	Part two, Trust as stored in the tokens and handled by the internal NSS cert processing

Originally the above structure was stored directly into the database. Even today something like it is stored in the old nss dbm format (which is still used by several applications). The problem is we can now have more than one source of trust for a certificate. We can accept trust from any number of PKCS #11 modules. The builtin module is just one module that provides a source of trust records for us, the user's database may also be a source of trust. Today, on linux, there can be several databases, included some databases that are supplied by the system administrator.

Each source of trust information can supply some trust values for a given certificate. Rolling a set of bits is complicated, so the underlying model is one of a set of purposes (NOTE, other purposes are defined, but none of them or processed by NSS):

#define CKA_TRUST_SERVER_AUTH*           (CKA_TRUST +  8)
#define CKA_TRUST_CLIENT_AUTH*           (CKA_TRUST +  9)
#define CKA_TRUST_CODE_SIGNING          (CKA_TRUST + 10)
#define CKA_TRUST_EMAIL_PROTECTION      (CKA_TRUST + 11)

*these two records map to sslFlags together in a complicated way which we will ignore as it has not bearing on the rest of the discussion.

each of which can take on a descrete value:

#define CKT_NSS_TRUSTED            (CKT_NSS + 1)
#define CKT_NSS_TRUSTED_DELEGATOR  (CKT_NSS + 2)
#define CKT_NSS_UNTRUSTED          (CKT_NSS + 3)
#define CKT_NSS_MUST_VERIFY        (CKT_NSS + 4)
#define CKT_NSS_TRUST_UNKNOWN      (CKT_NSS + 5) /* default */
#define CKT_NSS_VALID              (CKT_NSS + 10)
#define CKT_NSS_VALID_DELEGATOR    (CKT_NSS + 11)

modules are processed according to their priority, from lowest to highest. If the trust source has not trust object (or the attribute for the purpose we are interested in), or the record is CKT_NSS_TRUST_UNKNOWN, then the next highest trust source is queried. This proceeds until either a definitive trust value is found (one that is not CKT_NSS_TRUST_UNKNOWN).

When these values where created, they were envisioned to map to the bits above as follows:

CKT_NSS_TRUSTED -> CERTDB_TRUSTED
CKT_NSS_TRUSTED_DELEGATOR -> CERTDB_TRUSTED_CA
CKT_NSS_VALID -> CERTDB_VALID_PEER
CKT_NSS_VALID_DELEGATOR -> CERTDB_VALID_CA

CKT_NSS_TRUST_UNKNOWN would become CKT_NSS_MUST_VERIFY.

At this point there is a disconnect between how the supposed meaning of the CKT_ values and their actual mapping to the CERTDB_ values.

That disconnect was:

1) a present trust record with no bits on was assumed to be the same as CKT_NSS_UNTRUSTED. This conversion was assumed both ways, so if there was a trust record with no bits turned on, it would be stored in the PKCS #11 module as CKT_NSS_UNTRUSTED. The upshot is we cannot use the current CKT_NSS_UNTRUSTED for anything other then CKT_NSS_MUST_VERIFY.

2) the CERTDB_VALID_PEER bit was thought to mean that the peer is valid for the given operation, no matter what the cert data in the cert says. It would still need to be verified.

Here's how the low level maps to actual trust bits:

The trust values are mapped to CERTDB_ values as follows:

CKT_NSS_TRUSTED           -> CERTDB_TRUSTED|CERTDB_VALID_PEER
CKT_NSS_TRUSTED_DELEGATOR -> CERTDB_TRUSTED_CA|CERTDB_VALID_CA
CKT_NSS_VALID_DELEGATOR   -> CERTDB_VALID_CA
CKT_NSS_VALID             -> CERTDB_VALID_PEER

CKT_NSS_TRUST_UNKOWN and CKT_UNTRUSTED both map to zero bits. The coding of the CKT_NSS_MUST_VERIFY mapped it to CKT_NSS_MUST_VERIFY because the coder did not understand the difference.

The real semantic is now CKT_NSS_TRUST_UNKNOWN and CKT_NSS_MUST_VERIFY are treated as exactly the same.
CKT_NSS_UNTRUSTED is treated as it means CKT_NSS_MUST_VERIFY
For NSS leaf certs and root certs CKT_NSS_VAILD is really UNTRUSTED! For intermediate CA certs its a no-op.

When CERTDB_ trust values are mapped back out to low level trust values, the are mapped as follows:

If CERTDB_TRUSTED_CA is set (independent of any of the other bits), trust = CKT_NSS_TRUSTED_DELEGATOR
If CERTDB_TRUSTED is set (independent of any of the other bits excep those above) trust= CKT_TRUSTED
If CERTDB_VALID_CA is set (independent of any of the other bits excep those above) trust = CKT_VALID_DELEGATOR
If CERTDB_VALID_PEER is set (independent of any of the other bits excep those above) trust = CKT_VALID
If none of the above bits are set, trust = CKT_UNTRUSTED.


OK, so my proposal..... Since CKT_VALID already means UNTRUSTED for a certain set of certificates, I propose we expand that to include intermediate CA certs as well. Also since CKT_NSS_UNTRUSTED currently means MUST_VERIFY, I suggest we officially recognize that meaning.

The values CKT_NSS_UNTRUSTED, CKT_NSS_MUST_VERIFY, CKT_NSS_VALID and CERTDB_VALID_PEER well be deprecated with appropriate warning left in the header file. 

Internally CKT_NSS_UNTRUSTED will be renamed to CKT_NSS_NEED_VALIDATE
           CKT_NSS_VALID will be renamed to CKT_NSS_NOT_TRUSTED
           CERTDB_VALID_PEER will be renamed to CERTDB_TERMINAL_RECORD

The cert verification code will treat any cert which CERTDB_TERMINAL_RECORD set and either the expected CERTDB_TRUSTED or CERTDB_TRUSTED_CA not set, then the certificate will be treated as untrusted.
The above mapping code will put CERTB_VALID_PEER above CERTDB_VALID_CA.

This will allow us to clobber any cert we want explicitly. NOTE: this may trigger the need to have an explicit 'reject' button in PSM which will set the trust flags to CERTDB_TERMINAL_RECORD | 0.

BTW none of this is necessary to clobber the certs in the previous bug. I have a patch in bug 
642815 which does this.


bob

Comment 4

[Robert Relyea]

BTW, trust records can be separated from certificates. I ran into some bugs with leaf certs that have to be fixed. The currently require that the cert matches the hash in the record, but part of this proposal is to allow us to optionally clobber all certs with the same issuer/SN by setting the hash attributes to the string 'ANY'. In that case the cert would not have to match the hash. We would only do this for untrusted or must verify certs, of course.

Comment 5

[Robert Relyea]

> Normal checks on to make sure this certificate is an actual CA is skipped.

Better english: If this bit is on, normal CA checks on the certificate would be skipped.

BTW I forgot to meantion that there is a huge amount of confusion on the meaning of the CERTDB_VALID bits (even I was confused for a while). CERTDB_VALID_PEER and CERTDB_VALID_CA do not mean the related things. CERTDB_VALID_PEER means 'the CERTDB_TRUSTED flag is authortitive', CERTB_VALID_CA means 'the certificate is a valid ca, even if it doesn't have an extension that says so'.

I don't know if different meanings were original, or if one bit shifted meaning. My rename will make those differences clear.

bob

Comment 6

[Robert Relyea]

Adding Nelson and wtc to review the general proposal.

Additional information about the changes.

I intend to look at the CERTDB_TERMINAL_RECORD (currently called CERTDB_VALID_PEER) bit in the following instances that it currently isn't checked:

1) email - if CERTDB_TERMINAL_RECORD is set and CERTDB_TRUSTED is not set, then we won't trust the cert. (same code that is currently in signer and SSL processing).
2) ocsp -- currently ocsp accepts the cert record if any of the SSL, email, signer flags are set to trusted. I propose to add if none of them are set to trusted and any of them are set tu untrusted we will not trust this ocsp certificate.
3) CA chain processing. If the CERTDB_TERMINAL_RECORD is set and CERTDB_TRUSTED_CA is not set, then we will not trust that CA.

I think the current email semantic (1) is for historical reasons (in some very old version of NSS someone set the CERTDB_VALID_PEER on certificates thinking it meant the same for peers as CERTDB_VALID_CA meant for CAs). I think ocsp (2) is because it is a little more complicated to check for distrust over several certiciates (with the added thing that email certificates were marked incorrectly).

bob

Comment 7

[Wan-Teh Chang]

Bob:

1. Your comment 3 should be converted to an NSS tech note, which will
fix the NSS docs bug 319496.

2. Is this bug a duplicate of bug 470994?

3. Is this bug related to bug 254524?

Comment 8

[Kai Engert (:KaiE:)]

Would it be possible to mark certificates as revoked, causing NSS verification code to report error code "is revoked"?

I'm not sure if Bob's patch in bug 642815 has the same behavior as the approach described in this bug.

For the behaviour that we see if the error code is SEC_ERROR_UNTRUSTED_CERT see bug 642815 comment 11 to 15.

Comment 9

[Wan-Teh Chang]

I agree that if SEC_ERROR_UNTRUSTED_CERT means the user has marked the
certificate as not trusted, then the user should not be able to accept this
error.

I also found that libpkix seems to misinterpret SEC_ERROR_UNTRUSTED_CERT.
libpkix sets the SEC_ERROR_UNTRUSTED_CERT error if the
PKIX_PL_Cert_IsCertTrusted function fails (not to be confused with
setting the output argument *pTrusted to PKIX_FALSE):
http://mxr.mozilla.org/security/ident?i=PKIX_PL_Cert_IsCertTrusted
http://mxr.mozilla.org/security/search?string=CERTISCERTTRUSTEDFAILED

We should review that code carefully.  The fix could be as simple as
changing the error code in this PKIX_ERRORENTRY to 0:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h&rev=1.37&mark=199#199

Comment 10

[Robert Relyea]

SEC_ERROR_UNTRUSTED_CERT currently can mean both. At the validation code, we know if it is not trusted because we couldn't find a trust link or because it is explicitly marked untrusted.

Note that today, explicitly marking a cert untrusted is only something that happens SSL server certs and Code Signing certs. The proposal is to expand that to all other certs. This probably warrants another error code (SEC_ERROR_EXPLICITLY_UNTRUSTED) that PSM won't override. We could also use SEC_ERROR_REVOKED, which would supply all the same semantics, but may create a slightly confusing error message... though saying an untrusted cert is revoked is not completely confusing.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #5)

> CERTDB_VALID_PEER and CERTDB_VALID_CA do not mean the related things.
> CERTDB_VALID_PEER means 'the CERTDB_TRUSTED flag is authortitive',
> CERTB_VALID_CA means 'the certificate is a valid ca, even if it doesn't 
> have an extension that says so'.

Bob, I don't follow your statement about CERTDB_VALID_PEER. 
The (misnamed) "valid" flags have a different purpose than the "trusted" flags.

The valid flag means "Treat this cert as a Peer or CA for this type of usage,
even if the cert's extensions don't declare that".  It does not imply that 
the cert is "valid", e.g. doesn't override expiration, or invalid signature,
etc.  

The "valid" flag makes the difference between "cert not found" and "cert has untrusted issuer".  If a cert does not appear to be permitted to be used for
a particular use (e.g. CA) then it will not be found when searching for a cert
that IS permitted for that use.  Setting the valid flag ensures that the cert
will be found, and the error will be something else.

Comment 12

[Robert Relyea]

> The valid flag means "Treat this cert as a Peer or CA for this type of usage,
> even if the cert's extensions don't declare that".

Ah, yes the name has confused you as well as me. I thought that was the case, but it's not. If you set CERTDB_VALID_PEER and not set CERTDB_TRUSTED, NSS will reject that cert for that purpose (well if that purpose is SSL or code signing anyway).

CERTDB_VALID_CA *does* have the semantic you described and I have assumed all along. We do trust the cert as a valid CA, but still require that it chain eventually to a trusted certificate.

It appears way back when Jeff W designed CERTDB_VALID_PEER, what he really meant was the CERTDB_TRUSTED flag was authoritative. That is CERTDB_TRUSTED = 1 means the cert was trusted and CERTDB_TRUSTED = 0 means the cert was not trusted. In fact if you don't set CERTDB_VALID_PEER with CERTDB_TRUSTED the cert will not be trusted.

Comment 13

[Robert Relyea]

> (well if that purpose is SSL or code signing anyway).

I should point out my one worry is email. We currently do not reject email certs if the CERTDB_VALID_PEER is set but CERTDB_TRUSTED is not.

My proposal is based on the fact the currently, If you set CKT_NSS_VALID on any of the following certs:

SSL leaf, Object Signing Leaf, Any Root

It results in the cert being rejected (the expected semantic of CKT_NSS_UNTRUSTED).

I believe we should have not problem extending the semantic to ocsp certs and intermediate certs. email certs are the ones I still need to look at.

bob

Comment 14

[Robert Relyea]

OK, I've looked at our usage code. I only updates usage if both VALID and TRUSTED is set. So we currently don't have a way of saying 'VALID but verify'.

bob

Comment 15

[Wan-Teh Chang]

(In reply to comment #10)
> SEC_ERROR_UNTRUSTED_CERT currently can mean both. At the validation code, we
> know if it is not trusted because we couldn't find a trust link or because it
> is explicitly marked untrusted.

Bob, can you show me where NSS sets the SEC_ERROR_UNTRUSTED_CERT error
if it couldn't find a trust link?  I think NSS sets the
SEC_ERROR_UNKNOWN_ISSUER error in that case.

The four instances of setting the SEC_ERROR_UNTRUSTED_CERT error all
seem to be marking certificates explicitly untrusted:
http://mxr.mozilla.org/security/search?string=PORT_SetError(SEC_ERROR_UNTRUSTED_CERT)

Thank you.

Comment 16

[Robert Relyea]

Ah you are right. There are actually 3 errors:

SEC_ERROR_UNTRUSTED_CERT, which is currently returned in exactly those two cases where we explicitly distrust the cert.

SEC_ERROR_UNTRUSTED_ISSUER, which is set if we get to the end of the chain (self-signed root), and no certs in the chain are explicitly marked as trusted.

SEC_ERROR_UNKNOWN_ISSUER, which is set if we get cannot find the end of the chain (the last cert was not a self-signed root cert and we can't find the next cert in the chain).

In that case SEC_ERROR_UNTRUSTED_CERT could be treated as a hard error.

Comment 17

[Robert Relyea]

> 1. Your comment 3 should be converted to an NSS tech note, which will
> fix the NSS docs bug 319496.

Agreed, once we decide to walk down the path decribed by comment 3.

> 2. Is this bug a duplicate of bug 470994?

Probably..

> 3. Is this bug related to bug 254524?

Not really CKA_TRUSTED is a single bit whose meaning is a bit ambiguous. It could mean CKT_NSS_TRUSTED, CKT_NSS_TRUSTED_CA and it could apply to any or all of CKA_TRUST_SERVER_AUTH, CKA_TRUST_CLIENT_AUTH, CKA_TRUST_CODE_SIGNING, CKA_TRUST_EMAIL_PROTECTION, etc.

bob

Comment 18

[Jim]

What "cert hash" is being proposed to blacklist certificates?
It must not be a hash of the entire BER-encoded certificate.
That is, it must not be what browsers display as the certificate fingerprint.

If an attacker adjusts the bytes of a certificate outside the ToBeSigned part:
* It can still be valid BER, or at least pass the parser without error; and
* The signature still verifies; BUT
* Its fingerprint will be different (eg will not match a blacklisted fingerprint).

A fingerprint (hash of whole certificate) is fine for a whitelist (which is why it is OK for current uses such as checking that you are adding the correct new root), but not for a blacklist.

I suggest using the hash of the ToBeSigned part of the certificate in the blacklist. That cannot be changed without invalidating the signature. You have to calculate this hash to verify the signature anyway, though it might be awkward for the different parts of the code to share the one calculation.

Comment 19

[Wan-Teh Chang]

Attachment: Variant of the bad www.google.com certificate

Jim: thank you for your comment.  I made an extension
to your warning.  Even if an entire certificate is
DER-encoded, we can still generate a variant by either
adding or omitting the NULL in the signatureAlgorithm
after tbsCertificate, if an RSA signature algorithm is
used.

Attached is a variant of the bad www.google.com certificate.
I removed the NULL from the signatureAlgorithm after
tbsCertificate.

I was worried that blacklists that are based on the
entire certificates may not detect this variant.
Here are my test results.

This certificate cannot be imported into NSS because
CERT_NewTempCertificate fails with
SEC_ERROR_REUSED_ISSUER_AND_SERIAL due to the conflict
with the original bad www.google.com certificate in
libnssckbi.so:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/stanpcertdb.c&rev=1.86&mark=377,405#376

Windows CryptoAPI correctly rejects this certificate
with the CERT_TRUST_IS_REVOKED and CERT_TRUST_IS_EXPLICIT_DISTRUCT
errors.  (The original bad www.google.com certificate
is in the "Untrusted Publishers" certificate store.)

Comment 20

[Robert Relyea]

Attachment: Large rename patch - small semantic changes.

The following patch is quite large, but is really a big renaming patch.  There are very few semantic changes, which I'll identify below to aid in the review.

----------------------------------------------------------------------------
bulk rename...

This patch renames any occurrence of public # defines:
        CERTDB_VALID_PEER to CERTDB_TERMINAL_RECORD.
        CKT_NSS_UNTRUSTED to CKT_NSS_MUST_VERIFY_TRUST
        CKT_NSS_VALID to NSS_CKT_NOT_TRUSTED
        CKT_NSS_MUST_VERIFY to CKT_NSS_TRUST_UNKNOWN or to remove it if it is
now redundant.
        CKx_NETSCAPE_yyyy to CKx_NSS_yyyy as long as there is and CKx_NSS
define in those files what we otherwise will have touched.

-----------------------------------------------------------------------------
Changes to nssTrustLevel

This patch changes the private nssTrustLevel enum as follows:

1) nssTrustLevel_Valid is removed.
2) nssTrustLevel_MustVerify is added.

The  nssTrustLevel mapping to and from PKCS #11 has changed as as follows

Formerly:
CKT_NSS_UNTRUSTED (now CKT_NSS_MUST_VERIFY_TRUST) -> nssTrustLevel_Untrusted
CKT_NSS_MUST_VERIFY (now removed) -> nssTrustLevel_Unknown
CKT_NSS_VALID (now CKT_NSS_NOT_TRUSTED) -> nssTrustLevel_Valid

Now:
CKT_NSS_NOT_TRUSTED (was CKT_NSS_VALID) -> nssTrustLevel_Untrusted
CKT_NSS_MUST_VERIFY_TRUST (was CKT_NSS_UNTRUSTED) -> nssTrustLevel_MustVerify
default (anything unknown, including CKT_NSS_MUST_VERIFY) -> nssTrustLevel_Unknown

The nssTrustLevel mapping to and from certdb trust have changed a follows:


Formerly:
nssTrustLevel_Untrusted -> 0 (no bits set)
nssTrustLevel_Valid -> CERTDB_VALID_PEER (now CERTDB_TERMINAL_RECORD)
nssTrustLevel_MustVerify Did not exist.

Now:
nssTrustLevel_MustVerify -> 0 (no bits set)
nssTrustLevel_Untrusted -> CERTDB_VALID_PEER (now CERTDB_TERMINAL_RECORD)

NOTE: the result of all of these mappings should result in no semantic changes unless they are described below....

--------------------------------------------------------------------------------------
Other Notes

certdata.c is generated from certdata.txt (make generate). Only the latter needs to be reviewed.

Since these defines are public, I've preserved the old defines in certdb/certdb.h and util/pkcs11n.h. In both cases I've added code so that on our main platforms (Win, Mac, and Linux), applications will now get warnings when they use the old defines. The old defines have exactly the same values as they used to, and should behave as they used to (which is counter to what their old name implies).

Legacydb

The LedgacyDB code had a flag called CERTDB_NOT_TRUSTED with was set whenever CKT_NSS_UTRUSTED was passed in. Besides the
 rename of CERTDB_VALID_PEER to CERTDB_TERMINAL_RECORD, I also rename CERTDB_NOT_TRUSTED to CERTDB_MUST_VERIFY, This shou
ld have any semantic change, since CERTDB_NOT_TRUSTED has been treated as CERTDB_MUST_VERIFY since day one.

--------------------------------------------------------------------------------------
Semantic differences

Where possible, I've indicated the high level semantic differences in terms of certutil trust flag outputs.

**** add builtins ***
Add builtins. There was a bug in Add builtins where CKT_NETSCAPE_TRUSTED_DELEGATOR was or'ed with CKT_NETSCAPE_TRUSTED. The problem is these are int values, not bit fields. The resulting value would have been CKT_NSS_UNTRUSTED now CKT_NSS_MUST_VERIFY_TRUST. In either case, exactly opposite of what was requested. We need to pick one value, so I chose CKT_NSS_TRUSTED_DELEGATOR. (PC,PC,PC -> ,, now C,C,C)

If no bits are set then the value is CKT_NSS_MUST_VERIFY_TRUST rather than CKT_NETSCAPE_TRUST_UNKNOWN.


**** certdata.txt ****
The MD5 Collisions forged Rogue CA:
Was CKT_NETSCAPE_UNTRUSTED which now mapps to CKT_NSS_MUST_VERIFY_TRUST was changed to CKT_NSS_NOT_TRUSTED (formerly CKT_NSS_VALID).

This isn't a major semantic change. A different value will be passed out of PKCS #11, and different bits will be set on the cert at the user level (p,p,p rather then ,, ). For an intermediate CA, however, this doesn't produce any difference in the chain processing. The reason for this change is 2 fold: 1) NOT_TRUSTED is the desired result, MUST_VERIFY_TRUST was just the best we could do on an intermediate, and 2) when we fix the code to explicitly not trust intermediates, this cert will have the desired semantics we were originally shooting for.

**** pk11merge.c ****

When merging the attributes from two separate databases, we want prefer 'hard trust' settings over 'soft trust' settings. Hard trust settings are CKT_TRUSTED, CKT_TRUSTED_DELEGATOR, and CKT_NOT_TRUSTED. The old code intended this semantic, except it had CKT_UNTRUSTED as the hard attribute, which is effectively CKT_NSS_MUST_VERIFY_TRUST. So before if the bits were:

    ,, and p,p,p  The merged result would be ,,

now the merge result is p,p,p. (which means untrusted).

**** pki3hack.c ****

When mapping certdb trust to nssTrustLevel, If both the CERT_VALID_CA bit and CERT_VALID_PEER (now CERT_TERMINAL_RECORD) was on, The nssTrustLevel would be nssTrustLevelValidDelegator Now it's nssTrustLevel_NotTrusted. This can only happen with cert trust generated from the application. No trust record read from the DB can have this combination of bits. From a certutil level.

    pc,pc,pc simplified to c,c,c now it simplifies to p,p,p

**** sftkdb.c ****

Same semantic change ad pk11merge.c

Comment 21

[Robert Relyea]

> Jim: thank you for your comment.

yes, thanks. That's just one more reason to get issuer/SN only trust working. When you want to attach positive trust (this cert is trusted), then you need to make sure you have the exact certs. The negative trust case, however, it's ok (and even desirable) to squash all certs with that issuer/SN.

WTC's test successfully failed because we have the full cert in the builtins database right now.

Comment 22

[Elio Maldonado]

Comment on attachment 524022 [details] [diff] [review]
Large rename patch - small semantic changes.

>Index: cmd/addbuiltin/addbuiltin.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/cmd/addbuiltin/addbuiltin.c,v
>@@ -159,13 +159,13 @@ ConvertCertificate(SECItem *sdder, char 
>     printf("CKA_TRUST_CODE_SIGNING CK_TRUST %s\n",
> 				 getTrustString(trust->objectSigningFlags));
> #ifdef notdef
>-    printf("CKA_TRUST_CLIENT_AUTH CK_TRUST CKT_NETSCAPE_TRUSTED\n");*/

Unmathed closing comment, couldn't find the opening '/*', 
It compiles because it's disabled by the #ifdef notdef

It looks good, otherwise.


>Index: cmd/lib/pk11table.c
>Index: cmd/lib/secutil.c
>Index: lib/certdb/certdb.c
The preceding three look good.


>Index: lib/certdb/certdb.h
>===================================================================
Can't attest to either Windows or the older GNU compiler.

> 
>Index: lib/certhigh/certvfy.c
Looks good.

Not so good with these two.
>Index: lib/ckfw/builtins/certdata.c
>Index: lib/ckfw/builtins/certdata.txt

The patch does not apply cleanly because of these two files. 
The 'builtins' where likely updated since you sent it.

The following are are fine until we reach grammatical nitpicks
and the end.
>Index: lib/dev/ckhelper.c
>Index: lib/dev/devt.h
>Index: lib/dev/devtoken.c
>Index: lib/pk11wrap/debug_module.c
>Index: lib/pk11wrap/pk11merge.c
>Index: lib/pk11wrap/pk11nobj.c
>Index: lib/pki/pki3hack.c
>Index: lib/softoken/sftkdb.c
>Index: lib/softoken/legacydb/lgattr.c
>Index: lib/softoken/legacydb/lgcreate.c
>Index: lib/softoken/legacydb/pcertt.h
>===================================================================
The above look good. By a somewhat mechanical inspection I didn't
find any inconsistencies.

>Index: lib/util/pkcs11n.h
=========================
Grammatical nitpicks
>+ * The new labels correctly reflects what the values effectively mean.
>+ */

s/reflects/reflect/

>+/* This magice gets the windows compiler to give us a deprecation
>+ * warning */
s/magice/magic/

>+#pragma deprecated(CKT_NSS_UNTRUSTED, CKT_NSS_MUST_VERIFY, CKT_NSS_VALID)
>+

Comment 23

[Elio Maldonado]

Comment on attachment 524022 [details] [diff] [review]
Large rename patch - small semantic changes.

Thank you for the clarifications on semantics you gave me. On the parts I could inspect and it's an +r from me, conditional that is. Please update the sections of the patch for the builtins and address the other minor issues pointed out.

Comment 24

[Robert Relyea]

Patch 1 checked into trunk.

Checking in cmd/addbuiltin/addbuiltin.c;
/cvsroot/mozilla/security/nss/cmd/addbuiltin/addbuiltin.c,v  <--  addbuiltin.c
new revision: 1.16; previous revision: 1.15
done
Checking in cmd/lib/pk11table.c;
/cvsroot/mozilla/security/nss/cmd/lib/pk11table.c,v  <--  pk11table.c
new revision: 1.13; previous revision: 1.12
done
Checking in cmd/lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.105; previous revision: 1.104
done
Checking in lib/certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.112; previous revision: 1.111
done
Checking in lib/certdb/certdb.h;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.h,v  <--  certdb.h
new revision: 1.23; previous revision: 1.22
done
Checking in lib/certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.72; previous revision: 1.71
done
Checking in lib/ckfw/builtins/certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v  <--  certdata.c
new revision: 1.77; previous revision: 1.76
done
Checking in lib/ckfw/builtins/certdata.txt;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v  <--  certdata.txt
new revision: 1.74; previous revision: 1.73
done
Checking in lib/dev/ckhelper.c;
/cvsroot/mozilla/security/nss/lib/dev/ckhelper.c,v  <--  ckhelper.c
new revision: 1.41; previous revision: 1.40
done
Checking in lib/dev/devt.h;
/cvsroot/mozilla/security/nss/lib/dev/devt.h,v  <--  devt.h
new revision: 1.25; previous revision: 1.24
done
Checking in lib/dev/devtoken.c;
/cvsroot/mozilla/security/nss/lib/dev/devtoken.c,v  <--  devtoken.c
new revision: 1.55; previous revision: 1.54
done
Checking in lib/pk11wrap/debug_module.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/debug_module.c,v  <--  debug_module.c
new revision: 1.18; previous revision: 1.17
done
Checking in lib/pk11wrap/pk11merge.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11merge.c,v  <--  pk11merge.c
new revision: 1.8; previous revision: 1.7
done
Checking in lib/pk11wrap/pk11nobj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11nobj.c,v  <--  pk11nobj.c
new revision: 1.13; previous revision: 1.12
done
Checking in lib/pki/pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.102; previous revision: 1.101
done
Checking in lib/softoken/sftkdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sftkdb.c,v  <--  sftkdb.c
new revision: 1.28; previous revision: 1.27
done
Checking in lib/softoken/legacydb/lgattr.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lgattr.c,v  <--  lgattr.c
new revision: 1.11; previous revision: 1.10
done
Checking in lib/softoken/legacydb/lgcreate.c;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/lgcreate.c,v  <--  lgcreate.c
new revision: 1.7; previous revision: 1.6
done
Checking in lib/softoken/legacydb/pcertt.h;
/cvsroot/mozilla/security/nss/lib/softoken/legacydb/pcertt.h,v  <--  pcertt.h
new revision: 1.4; previous revision: 1.3
done
Checking in lib/util/pkcs11n.h;
/cvsroot/mozilla/security/nss/lib/util/pkcs11n.h,v  <--  pkcs11n.h
new revision: 1.22; previous revision: 1.21
done

Comment 25

[Robert Relyea]

Attachment: Large rename patch - small semantic changes. - as checked in

This is the patch which incorporates Elio's comments... as checked in.

Comment 26

[Kai Engert (:KaiE:)]

What are the next steps?

Should this be marked FIXED?
Do you intent to land this on 3.12 branch?

Comment 27

[Kai Engert (:KaiE:)]

Please ignore comment 26.
I understand the above was just renaming, and the real work will follow.

Comment 28

[Robert Relyea]

Attachment: Patch 2: expicitly revoke email and intermediate certs, and also revoke certs in libpkix.

This patch does not allow explicit trusting of leaf certs in libpkix yet, though it starts on that implementation.

Comment 29

[Robert Relyea]

Patch 2 description:

certutil.c secutil.c -
  Catch some nss tools UI renaming missed in the previous patch.

cert.h certvfy.c
  Refactor the leaf checking code so that we have one copy instead of three.
  Add email and OCSP to the leaf checking code.
  Add explicit distrust for intermediates and root certs.

devtoken.c
  Pick up token trust records (like those stored in the database and in the builtins) even when the given certificate is a temp certificate. This is necessary if we want to revoke certs without puting the actual cert in the builtins module).

pkix_pl_pki.h pkix_pl_cert.c
   Add function to check leave cert validity.
   Reject explicitly untrusted intermediate and root certs.

pkix_build.c
   Reject untrusted leaf certs. NOTE: there is more work here to accept trusted leaf certs. We know at this point they are trusted, but we many need to do more tests, and we have to build up the correct final state here.

certificate.c
   Accept trust records which do not have hash values if that trust record is used to revoke a certificate. This allows us to kill any cert with a given issuer/SN without knowing the actual certificate.

cert.sh init.sh
   remove setting the 'p' bits for our email test certs. This now means they are invalid (it always meant that for SSL and ObjectSigning certs).
   Add tests to distrust a leaf cert and an intermediate explicitly.

Comment 30

[Wan-Teh Chang]

Attachment: Patch 2 part 1: expicitly revoke email and intermediate certs

Bob checked in this part of patch 2 on 2011/07/12
after I reviewed it.

mozilla/security/nss/cmd/certutil/certutil.c, revision 1.159
mozilla/security/nss/cmd/lib/secutil.c, revision 1.107
mozilla/security/nss/lib/certhigh/certvfy.c, revision 1.73
mozilla/security/nss/lib/dev/devtoken.c, revision 1.56
mozilla/security/nss/lib/pki/certificate.c, revision 1.68
mozilla/security/nss/tests/cert/cert.sh, revision 1.58

Comment 31

[Wan-Teh Chang]

Attachment: Patch 2 part 2: revoke certs in libpkix and add test cases

The rest of Bob's patch 2, which still needs to be
reviewed and checked in.

Comment 32

[Wan-Teh Chang]

Attachment: Fix nits in patch 2 part 1

Bob: please review.

This patch not only fixes nits in your patch 2 part 1 but also
contains important questions for you, marked with "XXX".

The change to mozilla/security/nss/lib/dev/ckhelper.c is necessary
to handle a trust object with a zero-length CKA_CERT_SHA1_HASH
attribute, or a trust object without a CKA_CERT_SHA1_HASH attribute.

Note: while testing a trust object without a CKA_CERT_SHA1_HASH
attribute, I rediscovered the fact that we don't really need to
support it because NSS won't import a certificate with a duplicate
issuer and serial number.

Comment 33

[Wan-Teh Chang]

Attachment: Fix nits in patch 2 part 1 (v2) [checked in]

Bob,

I updated this patch based on our discussion this morning.
Please review, in particular the CERTDB_TRUSTED|CERTDB_TRUSTED_CA
changes.  I hope I made them correctly.

The description of this patch in comment 32 still applies.

Comment 34

[Wan-Teh Chang]

Attachment: Proof of concept: allow adding a trust object without an associated cert to nssckbi

This patch allows me to add a trust object to nssckbi
without an associated certificate.  If the trust object
has no SHA1 hash attribute, I verified that it matches
two different encodings of the intended certificate.

If I don't add the certificate to nssckbi, when I import
the certificate as a temp cert, |context| is non-NULL
whereas |instance| is NULL, so we call
nssCryptoContext_FindTrustForCertificate.  But the
trust object in nssckbi can only be looked up with a
nssTrustDomain_FindTrustForCertificate call.  So I add
that call if nssCryptoContext_FindTrustForCertificate
returns NULL.

Unfortunately the NSSCertificate object |c| doesn't
have the |issuer| and |serial| members set up at that
point.  Perhaps those two members will be set up later;
I don't know.  So I added a hack to set them up before
calling nssTrustDomain_FindTrustForCertificate.

Comment 35

[Robert Relyea]

Comment on attachment 560040 [details] [diff] [review]
Fix nits in patch 2 part 1 (v2) [checked in]

r+ rrelyea

Comment 36

[Wan-Teh Chang]

Attachment: Patch 2 part 2: revoke certs in libpkix and add test cases (v2) [checked in]

I edited Bob's patch.  Most of my changes were whitespace changes
(avoid TABs because TABs are not commonly used in libpkix code).

I will attach a file that shows the diffs between Bob's original
patch and this patch, ignoring whitespace.

Comment 37

[Wan-Teh Chang]

Attachment: Diffs between Bob's and my versions of patch 2 part 2, ignoring whitespace

Bob, please review my changes to your patch.  Thanks.

Re: the following code that you and I discussed this morning:

         /* no key usage information and store is not trusted */
         if (plContext == NULL || cert->store == NULL) {
                 *pTrusted = PKIX_FALSE;
                 goto cleanup;
         }
 
         if (cert->store) {
                 PKIX_CHECK(PKIX_CertStore_GetTrustCallback
                         (cert->store, &trustCallback, plContext),
                         PKIX_CERTSTOREGETTRUSTCALLBACKFAILED);

Rather than adding an XXX comment to revisit this later,
I decided to assume the code is correct and change the
comment to match, and remove the unnecessary if (cert->store)
test.

Comment 38

[Wan-Teh Chang]

Attachment: (Better) Diffs between Bob's and my versions of patch 2 part 2, ignoring whitespace

I regenerated the diffs with function names and more context.

Comment 39

[Wan-Teh Chang]

Comment on attachment 560040 [details] [diff] [review]
Fix nits in patch 2 part 1 (v2) [checked in]

Patch checked in on the NSS trunk (NSS 3.13).

Checking in cmd/certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.160; previous revision: 1.159
done
Checking in lib/certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.75; previous revision: 1.74
done
Checking in lib/dev/ckhelper.c;
/cvsroot/mozilla/security/nss/lib/dev/ckhelper.c,v  <--  ckhelper.c
new revision: 1.42; previous revision: 1.41
done

Comment 40

[Wan-Teh Chang]

Attachment: Fix typo "was was" in Large rename patch

Patch checked in on the NSS trunk (NSS 3.13).

Checking in pkcs11n.h;
/cvsroot/mozilla/security/nss/lib/util/pkcs11n.h,v  <--  pkcs11n.h
new revision: 1.23; previous revision: 1.22
done

Comment 41

[Robert Relyea]

Comment on attachment 560092 [details] [diff] [review]
Fix typo "was was" in Large rename patch

r+ rrelyea

Comment 42

[Robert Relyea]

Comment on attachment 560077 [details] [diff] [review]
(Better) Diffs between Bob's and my versions of patch 2 part 2, ignoring whitespace

r+

I presume the check-ed version will fix the white space from indenting the block inside the now removed if (cert->store) {

Thanks for not actually making the change in the patch for review;).

bob

Comment 43

[Wan-Teh Chang]

Comment on attachment 560074 [details] [diff] [review]
Patch 2 part 2: revoke certs in libpkix and add test cases (v2) [checked in]

Patch checked in on the NSS trunk (NSS 3.13).

Checking in lib/certdb/certi.h;
/cvsroot/mozilla/security/nss/lib/certdb/certi.h,v  <--  certi.h
new revision: 1.36; previous revision: 1.35
done
Checking in lib/libpkix/include/pkix_pl_pki.h;
/cvsroot/mozilla/security/nss/lib/libpkix/include/pkix_pl_pki.h,v  <--  pkix_pl_pki.h
new revision: 1.14; previous revision: 1.13
done
Checking in lib/libpkix/pkix/top/pkix_build.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/top/pkix_build.c,v  <--  pkix_build.c
new revision: 1.61; previous revision: 1.60
done
Checking in lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c,v  <--  pkix_pl_cert.c
new revision: 1.27; previous revision: 1.26
done
Checking in tests/cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.59; previous revision: 1.58
done
Checking in tests/common/init.sh;
/cvsroot/mozilla/security/nss/tests/common/init.sh,v  <--  init.sh
new revision: 1.73; previous revision: 1.72
done

Comment 44

[Wan-Teh Chang]

Comment on attachment 560074 [details] [diff] [review]
Patch 2 part 2: revoke certs in libpkix and add test cases (v2) [checked in]

Bob,

I have two questions about your changes to tests/cert/cert.sh.

>-        html_failed "${CU_ACTION} ($RET) " 
>+        html_failed "${CU_ACTION} ($RET=$EXPECTED) " 
>         cert_log "ERROR: ${CU_ACTION} failed $RET"

What does the shell expression ($RET=$EXPECTED) do?
Is it to print the values of both $RET and $EXPECTED?
If so, it seems better to print ($RET!=$EXPECTED) instead,
and we should also update the cert_log statement.  Agreed?

>     CU_ACTION="Import Root CA for $CERTNAME"
>     certu -A -n "TestCA" -t "TC,TC,TC" -f "${R_PWFILE}" -d "${PROFILEDIR}" \
>-          -i "${R_CADIR}/root.cert" 2>&1
>+          -i "${R_CADIR}/TestCA.ca.cert" 2>&1

There are two other changes like the above.  Why do you change root.cert to
TestCA.ca.cert and ecroot.cert to TestCA-ec.ca.cert?

Comment 45

[Kai Engert (:KaiE:)]

Bob, Wan-Teh, does this bug fix bug 647364, too? (If yes, could you please mark 647364 as a duplicate of this bug?)

All patches in this bug have been checked in - can this be marked fixed? If not what's missing?

(We are trying to understand the status of this bug, as it's indirectly blocking bug 651426. Thanks for clarifying.)

Comment 46

[Robert Relyea]

> does this bug fix bug 647364, too?

Short answer, No.. see bug 647364 for more detail.

bob

Comment 47

[Gervase Markham [:gerv]]

A different question: with this patch, do we now have the ability to add certificates (roots, intermediates or leaf) to NSS's store and mark them as "never trust a chain including this cert"? (This is the capability that we would have liked for the DigiNotar and Comodo incidents.)

If not, what else do we need to get that?

Gerv

Comment 48

[Wan-Teh Chang]

(In reply to Kai Engert (:kaie) from comment #45)
>
> All patches in this bug have been checked in - can this be marked fixed? If
> not what's missing?

The only remaining work is to add the various (original,
uncorrupted) DigiNotar certs to certdata.txt for use by
libpkix.

(In reply to Gervase Markham [:gerv] from comment #47)
> A different question: with this patch, do we now have the ability to add
> certificates (roots, intermediates or leaf) to NSS's store and mark them as
> "never trust a chain including this cert"?

Yes.  However we don't plan to merge the patches to the
NSS_3_12_BRANCH (in spite of the current target milestone
of 3.12.10), so NSS 3.12.x still uses the "knockout cert"
approach.

Comment 49

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Wan-Teh Chang from comment #48)
> The only remaining work is to add the various (original,
> uncorrupted) DigiNotar certs to certdata.txt for use by
> libpkix.

Why does this need to be done? Why aren't the current "knockout" certs not good enough?

If this needs to be done, I think we should do so in a new bug so we can close this one as Resolved Fixed/NSS/Libraries/3.12.10. Then, the new bug that changes the DigiNotar knockout certs, when fixed, will be Resolved Fixed/NSS/CA Certificates/1.89. And, if we need to make that change, then the next Firefox 3.6.x update should be sure to require version 1.89 or later of the root module.

Comment 50

[Wan-Teh Chang]

Marked the bug FIXED (in NSS 3.13) because NSS 3.13 has been released.
The patches in this bug were not checked in on the NSS_3_12_BRANCH.

The reason the "knockout" certs don't work for libpkix is that
libpkix can still build a certification path using the original
certs in the presence of the "knockout" certs.

Comment 51

[David E. Ross]

What is the tracking bug for version 3.13?  Is there some other bug report for that version?  I cannot find any bug report with either a target or version equal to 3.13.

Comment 52

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

We don't do tracking bugs for NSS releases. You can see the closed bugs with target milestone 3.13 here:
https://bugzilla.mozilla.org/buglist.cgi?list_id=1647926&classification=Components&query_format=advanced&bug_status=RESOLVED&bug_status=VERIFIED&bug_status=CLOSED&target_milestone=3.13&product=NSS

Also, 3.13 had a regression during NSS_init in it that was fixed in 3.13.1, so you probably want 3.13.1.

Comment 53

[Kai Engert (:KaiE:)]

Please see bug 727204.

I confirm that the patch added with Wan-Teh's comment 34 is required to make the active distrust work (active by issuer DN and serial number).

Based on that I would argue that this bug is not yet fixed.

Should this bug be reopened, or should the remaining work be tracked in bug 727204 ?