Trust bits for "AddTrust Public CA Root" are missing in builtin roots module

RESOLVED FIXED in 3.12

Status

NSS
Libraries
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Kaspar Brand, Assigned: kaie)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
Created attachment 268060 [details] [diff] [review]
Fix trust bits for AddTrust Public CA Root

The AddTrust Public CA Root (nickname "AddTrust Public Services Root", added to the builtin roots in 2001) doesn't have any trust bits. Listing the four AddTrust roots in a freshly loaded nssckbi module with certutil currently gives:

> Builtin Object Token:AddTrust Low-Value Services Root        C,C,p
> Builtin Object Token:AddTrust External Root                  C,C,C
> Builtin Object Token:AddTrust Public Services Root           ,,
> Builtin Object Token:AddTrust Qualified Certificates Root    C,C,C

This is actually a regression caused by

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/security/nss/lib/ckfw/builtins&command=DIFF&root=/cvsroot&file=certdata.txt&rev1=1.13&rev2=1.14#80

At that time, CKA_ISSUER and CKA_SERIAL_NUMBER were added to the trust settings in certdata.txt, but the CKA_ISSUER for the "AddTrust Public Services Root" was wrong, unfortunately - it's the same as for the "AddTrust External Root".

The effect of this is that NSS fails to retrieve the correct trust bits for the "AddTrust Public Services Root" cert, since they are looked up by combining CKA_ISSUER and CKA_SERIAL_NUMBER (cf. http://lxr.mozilla.org/mozilla/ident?i=nssToken_FindTrustForCertificate).

The attached patch fixes CKA_ISSUER for the AddTrust Public CA Root. Note that I left nssckbi.h untouched - I'm not sure if a new minor version is really needed (the comment only says "[...] AND whenever we change the list of trusted certificates").
Attachment #268060 - Flags: review?(nelson)
Who are AddTrust? Why haven't they noticed this problem themselves?

Their website http://www.addtrust.com/ is (C) 2002, as is their latest item of news. They still have a number of outstanding certificates on the public web (according to figures I have) but it's falling fast. They show all the signs of a company going out of business.

I'm not convinced we should fix this, at least until and unless AddTrust request it.

Gerv
(Reporter)

Comment 2

11 years ago
(In reply to comment #1)
> Who are AddTrust? Why haven't they noticed this problem themselves?

See e.g. news://news.mozilla.org:23/mailman.177.1175671835.6011.dev-tech-crypto@lists.mozilla.org (or http://www.mail-archive.com/dev-tech-crypto@lists.mozilla.org/msg01532.html, if you prefer an HTTP URI).

This is a bug about fixing an inconsistency in the Builtin Roots module, not about policy decisions. That's why I filed it against NSS, not the CA Certificates component.

> They still have a number of outstanding certificates on the public web
> (according to figures I have) but it's falling fast. They show all the signs
> of a company going out of business.

By that argument, you would have to remove the trust bits for (at least) these roots as well:

Builtin Object Token:GTE CyberTrust Root CA                  CG,C,C
Builtin Object Token:GTE CyberTrust Global Root              CG,C,C
(GTE is gone, the root is with Verizon Business, currently)

Builtin Object Token:Equifax Secure CA                       C,C,C
Builtin Object Token:Equifax Secure Global eBusiness CA      C,C,C
Builtin Object Token:Equifax Secure eBusiness CA 1           C,C,C
Builtin Object Token:Equifax Secure eBusiness CA 2           C,C,C
(Equifax is no longer in the PKI business, the roots are owned by Geotrust/Verisign)

Builtin Object Token:ValiCert Class 1 VA                     C,C,C
Builtin Object Token:ValiCert Class 2 VA                     C,C,C
Builtin Object Token:RSA Root Certificate 1                  C,C,C
(ValiCert is gone, its roots are now owned by either Go Daddy or RSA)

Builtin Object Token:Baltimore CyberTrust Root               C,C,p
Builtin Object Token:beTRUSTed Root CA                       C,C,C
Builtin Object Token:beTRUSTed Root CA-Baltimore Implementation C,C,C
Builtin Object Token:beTRUSTed Root CA - Entrust Implementation C,C,C
Builtin Object Token:beTRUSTed Root CA - RSA Implementation  C,C,C
(Baltimore and Betrusted are gone, they ended up in Cybertrust, which is now owned by Verizon Business)

Builtin Object Token:UTN-USER First-Network Applications     C,C,C
Builtin Object Token:UTN DATACorp SGC Root CA                C,p,p
Builtin Object Token:UTN USERFirst Email Root CA             p,C,p
Builtin Object Token:UTN USERFirst Hardware Root CA          C,p,p
Builtin Object Token:UTN USERFirst Object Root CA            p,p,C
(UTN roots all owned by Comodo nowadays)

This doesn't mean I would object if you go through the whole list and re-evaluate the existing roots according to the new policy, of course - but if that's going to happen, then it should really be a separate endeavor.

In the meantime, fixing the broken CKO_NETSCAPE_TRUST settings for the AddTrust root in the builtins seems reasonable (note that otherwise, this would continue to be the *only* root included in NSS which has no trust bits at all - not really what this module is supposed to provide, I guess).
Assuming Kaspar's analysis is correct, this bug was introduced in rev 1.14,
and this cert has had no trust bits since year 2001!  If no-one has noticed
until now, I think we can safely conclude that fixing this is not urgent.

BTW, yes, changing the version number for NSSCKBI would be necessary to 
make this change.  But that is no worry.  The version doesn't have to be
changed for every change to the module, but rather for every release of 
the module that contains a change.  Because of the good work that Gerv is
doing in clearing the backlog of CA cert requests, new certs are being added
to the list rather frequently now, so one needn'y worry about this particular
change necessitating any otherwise-unnecessary version number changes.

I see another issue in the output cited above.  See all those lower case "p"
characters?  They're all wrong.  I think there is another bug filed about
that problem however.  
Assignee: nobody → kengert
Status: UNCONFIRMED → NEW
Ever confirmed: true
The checkins for bug 348882 removed the CKT_NETSCAPE_VALID flags from 
certdata.txt, which SHOULD have eliminated the "p" flags, but didn't. 
So, apparently we have another bug somewhere. :-(
Comment on attachment 268060 [details] [diff] [review]
Fix trust bits for AddTrust Public CA Root

r=nelson.  Asking Kai for a second review.

This bug demonstrates a serious weakness with the present format of the certdata files:
It's almost impossible to verify the contents by human inspection.  

Here is one way that could be improved.  We could preceed each large octal block of subject or issuer name with a line containing the DN in string form, and programmatically verify that the octal code is indeed the DER encoding of the string.  That way, a human could verify the string contents (which would be readable) and rely on the tool to verify that the octal matches the string.
Attachment #268060 - Flags: superreview?(kengert)
Attachment #268060 - Flags: review?(nelson)
Attachment #268060 - Flags: review+
Kaspar: my point was that the current owners are going out of business, not that the company whose name is on the root is going out of business. While it's unfortunate that roots have changed hands, there's not much we can do about that situation now.

I also don't agree with the idea that unless we can fix the entire problem, we can't fix any of it. If this root is unused (and I don't think anyone could describe it otherwise) then we should remove it. May it be the first of many.

I agree that this is a bug in the store, and so the final call belongs to the owner of the module containing the store (which isn't me). But if their decision is to remove the root, I would entirely support that from a policy point of view. 

Gerv
(Reporter)

Comment 7

11 years ago
(In reply to comment #6)
> Kaspar: my point was that the current owners are going out of business,

Did you actually look at my posting to dev-tech-crypto I cited above? [http://www.mail-archive.com/dev-tech-crypto@lists.mozilla.org/msg01532.html] The AddTrust roots are now owned by Comodo, and I have no indication that they are going out of business anytime soon (or do you know better?).

> While it's
> unfortunate that roots have changed hands, there's not much we can do about
> that situation now.

Yes, welcome to the world of commercial trust anchors... that's how things work. (See also Lucky Green's posting to the cryptography mailing list I referenced in my posting in April - "IP: SSL Certificate "Monopoly" Bears Financial Fruit", July 2002). I never said we should attempt to change that - doing so would just be a complete quixotism.

> I agree that this is a bug in the store, and so the final call belongs to the
> owner of the module containing the store (which isn't me).

It's comparable to a fix like the one for Bug 289979, which was committed a couple of days ago. Nelson has given r+ on my patch in the meantime, so I hope it will get in when certdata.{txt,c} are touched again. (I agree it isn't urgent and doesn't need to be fixed by committing separately.)
(Assignee)

Comment 8

11 years ago
Since the patch itself is not really human readable, I am not sure how to review this patch.

Proposal:

Please tell me what trust flags you are going to add, I can't find a statement about that in this bug.

I could export the cert, re-add it with the new trust flags, and compare the results with what you produced?

(Reporter)

Comment 9

11 years ago
(In reply to comment #8)
> Since the patch itself is not really human readable, I am not sure how to
> review this patch.
> 
> Proposal:
> 
> Please tell me what trust flags you are going to add, I can't find a statement
> about that in this bug.

Ok, I understand it's difficult to review that one... let me try to explain what it does in a few words. Executing

  addbuiltin -n "AddTrust Public Services Root" -t C,C,C -i addtrustpsr.der

will create the following CKO_NETSCAPE_TRUST object for certdata.txt:

  # Trust for Certificate "AddTrust Public Services Root"
  CKA_CLASS CK_OBJECT_CLASS CKO_NETSCAPE_TRUST
  CKA_TOKEN CK_BBOOL CK_TRUE
  CKA_PRIVATE CK_BBOOL CK_FALSE
  CKA_MODIFIABLE CK_BBOOL CK_FALSE
  CKA_LABEL UTF8 "AddTrust Public Services Root"
  CKA_CERT_SHA1_HASH MULTILINE_OCTAL
  \052\266\050\110\136\170\373\363\255\236\171\020\335\153\337\231
  \162\054\226\345
  END
  CKA_CERT_MD5_HASH MULTILINE_OCTAL
  \301\142\076\043\305\202\163\234\003\131\113\053\351\167\111\177
  END
  CKA_ISSUER MULTILINE_OCTAL
  \060\144\061\013\060\011\006\003\125\004\006\023\002\123\105\061
  \024\060\022\006\003\125\004\012\023\013\101\144\144\124\162\165
  \163\164\040\101\102\061\035\060\033\006\003\125\004\013\023\024
  \101\144\144\124\162\165\163\164\040\124\124\120\040\116\145\164
  \167\157\162\153\061\040\060\036\006\003\125\004\003\023\027\101
  \144\144\124\162\165\163\164\040\120\165\142\154\151\143\040\103
  \101\040\122\157\157\164
  END
  CKA_SERIAL_NUMBER MULTILINE_OCTAL
  \002\001\001
  END
  CKA_TRUST_SERVER_AUTH CK_TRUST CKT_NETSCAPE_TRUSTED_DELEGATOR
  CKA_TRUST_EMAIL_PROTECTION CK_TRUST CKT_NETSCAPE_TRUSTED_DELEGATOR
  CKA_TRUST_CODE_SIGNING CK_TRUST CKT_NETSCAPE_TRUSTED_DELEGATOR
  CKA_TRUST_STEP_UP_APPROVED CK_BBOOL CK_FALSE

If you do a diff of this against the version as currently stored in certdata.txt, you will find that the CKA_ISSUER information is not the same - in certdata.txt v1.42, it's currently

  CKA_ISSUER MULTILINE_OCTAL
  \060\157\061\013\060\011\006\003\125\004\006\023\002\123\105\061
  \024\060\022\006\003\125\004\012\023\013\101\144\144\124\162\165
  \163\164\040\101\102\061\046\060\044\006\003\125\004\013\023\035
  \101\144\144\124\162\165\163\164\040\105\170\164\145\162\156\141
  \154\040\124\124\120\040\116\145\164\167\157\162\153\061\042\060
  \040\006\003\125\004\003\023\031\101\144\144\124\162\165\163\164
  \040\105\170\164\145\162\156\141\154\040\103\101\040\122\157\157
  \164
  END

This is actually the issuer DN of the "AddTrust External Root", which can be found by scrolling back a few lines and comparing it with the CKA_ISSUER there (it's the CKO_NETSCAPE_TRUST object immediately preceding the "AddTrust Public Services Root").

Now, all my patch does is replace the CKA_ISSUER field by the correct one. Therefore, strictly speaking, we're not adding any trust flags, we're just making sure that NSS will actually *find* them ;-)

Finally, FWIW, here's an octal dump of the "new" CKA_ISSUER my patch is going to add, showing printable characters where available - the last two lines are of particular relevance, as they show that it's the issuer DN of the "AddTrust Public CA Root".

0000000  060 144 061 013 060 011 006 003 125 004 006 023 002 123 105 061
           0   d   1 013   0  \t 006 003   U 004 006 023 002   S   E   1
0000020  024 060 022 006 003 125 004 012 023 013 101 144 144 124 162 165
         024   0 022 006 003   U 004  \n 023 013   A   d   d   T   r   u
0000040  163 164 040 101 102 061 035 060 033 006 003 125 004 013 023 024
           s   t       A   B   1 035   0 033 006 003   U 004 013 023 024
0000060  101 144 144 124 162 165 163 164 040 124 124 120 040 116 145 164
           A   d   d   T   r   u   s   t       T   T   P       N   e   t
0000100  167 157 162 153 061 040 060 036 006 003 125 004 003 023 027 101
           w   o   r   k   1       0 036 006 003   U 004 003 023 027   A
0000120  144 144 124 162 165 163 164 040 120 165 142 154 151 143 040 103
           d   d   T   r   u   s   t       P   u   b   l   i   c       C
0000140  101 040 122 157 157 164
           A       R   o   o   t
0000146

Hope this helps when reviewing the patch. Let me know if you need any additional information.
(Assignee)

Comment 10

11 years ago
Comment on attachment 268060 [details] [diff] [review]
Fix trust bits for AddTrust Public CA Root

Kaspar, thanks a lot for your very helpful explanation.

I agree with Kaspar's analysis in this bug.

I exported CA from NSS, I re-added the cert using addtrust, I moved the new data over the old incorrect data, and I ended up with the same patch that Kaspar attached.

r=kengert
Attachment #268060 - Flags: superreview?(kengert) → superreview+
(Assignee)

Comment 11

11 years ago
I checked in the patches for bug 384118 and bug 383722 to both trunk and 3.11 branch. Marking fixed.

trunk:

Checking in ckfw/builtins/certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v  <--  certdata.c
new revision: 1.43; previous revision: 1.42
done
Checking in ckfw/builtins/certdata.txt;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v  <--  certdata.txt
new revision: 1.43; previous revision: 1.42
done


3.11 branch:

Checking in ckfw/builtins/certdata.c;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v  <--  certdata.c
new revision: 1.36.24.6; previous revision: 1.36.24.5
done
Checking in ckfw/builtins/certdata.txt;
/cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v  <--  certdata.txt
new revision: 1.37.24.6; previous revision: 1.37.24.5
done
(Reporter)

Comment 12

11 years ago
(In reply to comment #11)
> Marking fixed.

Wasn't the case yet - so resolving it definitely now.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12
You need to log in before you can comment on or make changes to this bug.