Closed
Bug 348882
Opened 18 years ago
Closed 17 years ago
addbuiltin command ignores "c" trust arg (and probably others)
Categories
(NSS :: Tools, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.6
People
(Reporter: david.konrad.stutzman, Assigned: alvolkov.bgs)
Details
Attachments
(3 files, 2 obsolete files)
7.46 KB,
text/plain
|
Details | |
56.36 KB,
patch
|
rrelyea
:
review+
neil.williams
:
review+
|
Details | Diff | Splinter Review |
1.20 KB,
patch
|
KaiE
:
review+
wtc
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060815 BonEcho/2.0b1 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060815 BonEcho/2.0b1 I pass a trustarg of "c,c,c" to addbuiltin for a particular certificate and after the built-ins module is built and I check the contents with certutil I see the certificate has a trust arg of "p,p,p". I understand that the nssckbi module is usually used for "root" certificates only but it has value for other non-standard purposes as well. Reproducible: Always Steps to Reproduce: 1. addbuiltin -n "foo" -t "c,c,c" < foo.der >> certdata.txt 2. make generate 3. rebuild nss 4. copy the resulting libnssckbi.so to a location pointed to by the secmod.db in your profile 5. "certutil -L -d . -h all" to inspect the results Actual Results: Builtin Object Token:foo p,p,p Expected Results: Builtin Object Token:foo c,c,c
Reporter | ||
Comment 1•18 years ago
|
||
I copied the original addbuiltin.c to addbuiltin.c.orig then modified addbuiltin.c. I used the following command to generate the diff: diff -rcs addbuiltin.c.orig addbuiltin.c
Comment 2•18 years ago
|
||
Alexei, if you can confirm this bug, it's probably worthy of P2. We should ensure that ALL the trust flags are accurately set in the root cert module by the addbuiltins command, and are accurately passed from the root certs module to the rest of NSS, as seen by certutil -L.
Assignee: nobody → alexei.volkov.bugs
Severity: minor → normal
Version: unspecified → 3.11
Comment 3•18 years ago
|
||
Comment on attachment 234039 [details] [diff] [review] patch to make addbuiltin recognize "c" trustarg 12345678901234567890123456789012345678901234567890123456789012345678901234567890 This patch also shows where that unwanted "p" trust flag comes from, IINM. We should fix that too, while we're fixing this. > } else { > if (trust & CERTDB_TRUSTED_CA) { > return "CKT_NETSCAPE_TRUSTED_DELEGATOR"; >! } else { > return "CKT_NETSCAPE_VALID"; <<-- 'p' > } > }
Updated•18 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → 3.11.4
Assignee | ||
Comment 4•18 years ago
|
||
Attachment #234039 -
Attachment is obsolete: true
Attachment #240208 -
Flags: review?(nelson)
Comment 5•18 years ago
|
||
Alexei, to facilitate review, please attach to this bug: a) a text file containing the output of "certutil -L -d . -h all" made with an nssckbi built with your patch applied, and b) a text file with the diffs between that output and the output of the same command made without your patch applied.
Assignee | ||
Comment 6•18 years ago
|
||
Comment 7•18 years ago
|
||
Comment on attachment 240208 [details] [diff] [review] includes changes to certdata files OK, this looks good to me. I'm hereby asking Bob Relyea to SR it, since it's a change to the builtins list. David, please test this patch and tell us if it works for you.
Attachment #240208 -
Flags: superreview?(rrelyea)
Attachment #240208 -
Flags: review?(nelson)
Attachment #240208 -
Flags: review+
Comment 8•18 years ago
|
||
please try using CKT_NETSCAPE_VALID_DELEGATOR rather than CKT_NETSCAPE_UNTRUSTED. The latter means 'this is a valid CA' the latter means, 'this is untrusted, no matter what another (following) trust module says'. If that doesn't produce the desired results, then our parsing on CKT_NETSCAPE_UNTRUSTED may be incorrect in the stan code. bob
Comment 9•18 years ago
|
||
Bob, First, thanks for this comment. This is exactly why I wanted you to review this patch. But second, valid designator (Valid CA) is an override. It means "this CA is valid, even if it appears not to be". I agree that we don't want to say "this is explicitly invalid", but IMO neither do we want to say "this is valid, perform no validity checks." I think we want to just leave it "blank", with no validity flags, for CAs in which we have no trust for a particular usage. What's the best way to say that?
Reporter | ||
Comment 10•18 years ago
|
||
(In reply to comment #7) > David, please test this patch and tell us if it works for you. If you mean https://bugzilla.mozilla.org/attachment.cgi?id=240208, it won't help the problem I was having (as it's a patch for the default mozilla built-ins). I came across this behavior when I was building a custom libnssckbi.so. I was adding intermediate CAs and didn't want to set a full "C" trust arg, just a "c" which is valid CA (but not trusted) and the addbuiltin code was ignoring the "c" flag. I set a full "C" flag on the root CAs which works fine.
Reporter | ||
Comment 11•18 years ago
|
||
(In reply to comment #10) > If you mean https://bugzilla.mozilla.org/attachment.cgi?id=240208, it won't > help the problem I was having (as it's a patch for the default mozilla > built-ins). I see the patch for addbuiltin at the bottom. I'll test it out.
Comment 12•18 years ago
|
||
David, You filed this bug (bug 348882), and now there is a patch that purports to fix it. I'm asking you to verify this fix. Thanks.
Reporter | ||
Comment 13•18 years ago
|
||
Looks good. The output below is matching what I'm passing in with the addbuiltin command. Here's "certutil -L -d . -h all" output: Builtin Object Token:Root1 C,C,c Builtin Object Token:Root2 C,C,c Builtin Object Token:Intermediate CA c,c,c Builtin Object Token:user1 ,, Builtin Object Token:user2 ,, Sorry about my misunderstanding earlier. Thanks for taking care of this feature.
Comment 14•18 years ago
|
||
So Nelson, I thought the desire was to have the 'c' bit on, which means trust this as a valid CA, though not necessarily a trusted one. That would be the CKT_NETSCAPE_VALID_DELEGATOR bit. If you want blank ',,' rather than 'c,c,c' then the bit would be CKT_NETSCAPE_TRUST_UNKNOWN. bob
Comment 15•18 years ago
|
||
Bob, the 'c' bit is an OVERRIDE. It makes a CA cert "valid", even if it is not otherwise valid. It causes NSS to skip numerous checks on the CA cert's validity. IMO, we should not be habitually marking CA certs with this override, whether they need it or not. Likewise the 'p' bit is an override. We should not be setting overrides where they are not needed.
Comment 16•18 years ago
|
||
Nelson, I don't have a problem with that. Use CKT_NETSCAPE_TRUST_UNKNOWN then. I was just responsing to the original complaint which the bug was about.
> I pass a trustarg of "c,c,c" to addbuiltin for a particular certificate and
> after the built-ins module is built and I check the contents with certutil I
> see the certificate has a trust arg of "p,p,p".
Clearly, in this case, the desire was to specify the override and it morphed into a different override. If "c,c,c" was specified that result was ",," that would also be a bug.
bob
Comment 17•18 years ago
|
||
Comment on attachment 240208 [details] [diff] [review] includes changes to certdata files We should either have CKT_NETSCAPE_VALID_DELEGATOR or CKT_NETSCAPE_UNKNOWN.
Attachment #240208 -
Flags: superreview?(rrelyea) → superreview-
Assignee | ||
Updated•17 years ago
|
Target Milestone: 3.11.4 → 3.11.6
Assignee | ||
Comment 18•17 years ago
|
||
Attachment #240208 -
Attachment is obsolete: true
Attachment #254225 -
Flags: review?(rrelyea)
Assignee | ||
Updated•17 years ago
|
Attachment #254225 -
Attachment description: use CKT_NETSCAPE_TRUST_UNKNOWN CKT_NETSCAPE_UNTRUSTED → use CKT_NETSCAPE_TRUST_UNKNOWN instead of CKT_NETSCAPE_UNTRUSTED
Comment 19•17 years ago
|
||
Comment on attachment 254225 [details] [diff] [review] use CKT_NETSCAPE_TRUST_UNKNOWN instead of CKT_NETSCAPE_UNTRUSTED r+ good patch! bob
Attachment #254225 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 20•17 years ago
|
||
Comment on attachment 254225 [details] [diff] [review] use CKT_NETSCAPE_TRUST_UNKNOWN instead of CKT_NETSCAPE_UNTRUSTED one more review for the 3.11.6
Attachment #254225 -
Flags: superreview?(nelson)
Assignee | ||
Comment 21•17 years ago
|
||
Comment on attachment 254225 [details] [diff] [review] use CKT_NETSCAPE_TRUST_UNKNOWN instead of CKT_NETSCAPE_UNTRUSTED Neil, Nelson and Bob already reviewed the patch at least ones. Could you please give it last final review for 3.11.6. Thx
Attachment #254225 -
Flags: review?(neil.williams)
Updated•17 years ago
|
Attachment #254225 -
Flags: review?(neil.williams) → review+
Assignee | ||
Comment 22•17 years ago
|
||
Checking into 3.11 branch: Checking in cmd/addbuiltin/addbuiltin.c; /cvsroot/mozilla/security/nss/cmd/addbuiltin/addbuiltin.c,v <-- addbuiltin.c new revision: 1.13.24.1; previous revision: 1.13 /cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v <-- certdata.c new revision: 1.36.24.4; previous revision: 1.36.24.3 /cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v <-- certdata.txt new revision: 1.37.24.4; previous revision: 1.37.24.3 done
Assignee | ||
Comment 23•17 years ago
|
||
Attachment #255025 -
Flags: superreview?(wtchang)
Attachment #255025 -
Flags: review?(kengert)
Updated•17 years ago
|
Attachment #255025 -
Flags: superreview?(wtchang) → superreview+
Comment 24•17 years ago
|
||
Comment on attachment 255025 [details] [diff] [review] bumping nssckbi version r=kengert
Attachment #255025 -
Flags: superreview?(wtchang)
Attachment #255025 -
Flags: superreview+
Attachment #255025 -
Flags: review?(kengert)
Attachment #255025 -
Flags: review+
Updated•17 years ago
|
Attachment #255025 -
Flags: superreview?(wtchang) → superreview+
Assignee | ||
Comment 25•17 years ago
|
||
Checking into 3.11 branch: /cvsroot/mozilla/security/nss/lib/ckfw/builtins/nssckbi.h,v <-- nssckbi.h new revision: 1.14.2.3; previous revision: 1.14.2.2
Assignee | ||
Comment 26•17 years ago
|
||
Checking into trunk: /cvsroot/mozilla/security/nss/cmd/addbuiltin/addbuiltin.c,v <-- addbuiltin.c new revision: 1.14; previous revision: 1.13 /cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.c,v <-- certdata.c new revision: 1.41; previous revision: 1.40 /cvsroot/mozilla/security/nss/lib/ckfw/builtins/certdata.txt,v <-- certdata.txt new revision: 1.41; previous revision: 1.40
Assignee | ||
Updated•17 years ago
|
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Attachment #254225 -
Flags: superreview?(nelson)
You need to log in
before you can comment on or make changes to this bug.
Description
•