Closed
Bug 323903
Opened 19 years ago
Closed 19 years ago
Patch: further improvements on displaying certificate extensions
Categories
(Core Graveyard :: Security: UI, defect)
Core Graveyard
Security: UI
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mozbgz, Assigned: KaiE)
Details
(Keywords: fixed1.8.1, Whiteboard: [kerh-eha])
Attachments
(2 files)
(deleted),
patch
|
KaiE
:
review+
KaiE
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier:
As suggested by Kai in bug 259031, I'm filing a new bug for the patch I proposed there. It includes the following fixes/improvements:
1) basic constraints extension: make ProcessBasicConstraints() more robust in case CERT_DecodeBasicConstraintValue() fails (just display raw DER data), add support for recognizing CERT_UNLIMITED_PATH_CONSTRAINT, avoid uninitialized pathLenConstraint when isCA is false (i.e., it will correctly handle the certificate from https://www.verisign.com even if bug 323557 doesn't get fixed yet).
2) authority information access: add support for the OCSP responder OID; fix bug that information about the criticality gets overwritten inadvertently; remove unnecessary line breaks (which get added by ProcessGeneralName() already)
3) extended key usage: in addition to text description, also display the OID afterwards (in parentheses)
4) add support for a bunch of (mostly Microsoft) Extended Key Usage OIDs. The wording has been taken from the English version of a Windows XP operating system (they are coded into crypt32.dll, specifically).
5) add basic recognition of the PKIX Logotype extension as specified in RFC 3709 (contents are dumped in raw DER format only)
6) minor wording changes. EKU descriptions for Microsoft OIDs (CertDumpEKU_1_3_6_1_4_1_311_*) have again been taken from crypt32.dll, while the others (such as "Code Signing", "OCSP Signing", or "Netscape Server Gated Crypto") have been synchronized with the descriptions in OpenSSL (crypto/objects/objects.txt, specifically).
7) CRL distribution points: suppress an unneccesary blank line if more than one CDP is listed
I hope it's not too late to get this into Firefox & Thunderbird 2.0.
Martin, can we agree on this (revised) version of the patch?
Reproducible: Always
Steps to Reproduce:
(for the basic constraints bug: view the certificate details from https://www.verisign.com, e.g.)
(patch to be added with next comment)
Updated•19 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Updated•19 years ago
|
Whiteboard: [kerh-eha]
Assignee | ||
Updated•19 years ago
|
Attachment #208870 -
Flags: review?(martin)
Comment 2•19 years ago
|
||
The code changes all look good to me. I have still issues with the renaming, though:
1_3_6_1_5_5_7_3_3 is called "Signing of downloadable executable code" in RFC 3280;
1_3_6_1_5_5_7_3_9 is called "Signing OCSP responses" in the same place. The RFC naming sounds "more official" than OpenSSL's, to me.
Also, atleast for the Microsoft block, I would like to see a comment like
# Descriptions follow English version of XP
or some such (not sure whether there is a copyright issue in doing so;
that issue would exist regardless of whether this is explicitly spelled
out in the code).
(In reply to comment #2)
Thanks for the feedback, Martin.
> I have still issues with the renaming,
> though:
> 1_3_6_1_5_5_7_3_3 is called "Signing of downloadable executable code" in RFC
> 3280;
Here I took the liberty of using the wording from Win XP (and OpenSSL) as well - I think it will cause less confusion for users (especially on Windows), if we use the same wording. Also, if people look at it with OpenSSL and compare it with the display in Mozilla, it would be quite irritating... OpenSSL doesn't display OIDs, so it would be hard to figure out whether "Code Signing" and "Signing of downloadable executable code" refer to the same thing.
> 1_3_6_1_5_5_7_3_9 is called "Signing OCSP responses" in the same place. The RFC
> naming sounds "more official" than OpenSSL's, to me.
In the first version of the patch, I had it like this (exactly ;-) But then again, I think it's misleading if OpenSSL displays "OCSP Signing" and Mozilla says "Signing OCSP responses"...
I think RFC 3280 is not the best source for the exact wording in some cases (e.g., would you want to say "Binding the hash of an object to a time" instead of "Time Stamping"?), so I suggest we take OpenSSL (and even Win XP, in the case of 1_3_6_1_5_5_7_3_3) as a reference in this case.
> Also, atleast for the Microsoft block, I would like to see a comment like
>
> # Descriptions follow English version of XP
>
> or some such (not sure whether there is a copyright issue in doing so;
> that issue would exist regardless of whether this is explicitly spelled
> out in the code).
Maybe Kai can decide on that...? I don't think Microsoft can claim a trade mark or something like that on these strings - and after all, they should be glad to see that their OIDs are consistently named in other software, too.
Assignee | ||
Comment 4•19 years ago
|
||
> Maybe Kai can decide on that...? I don't think Microsoft can claim a trade mark
> or something like that on these strings - and after all, they should be glad to
> see that their OIDs are consistently named in other software, too.
I am not a lawyer, and can not comment, whether reusing strings from non-free products in our software is allowed.
In general I'd say, if we use the same strings as in other free products, we should be safe. So if all strings you are interested in can be copied from OpenSSL, I think that would be prefered?
Assignee | ||
Comment 5•19 years ago
|
||
Gerv, please see the previous 2 comments in this bug.
Do you know, are we allowed to use user interface strings from other non-free products in our software, without violating rights?
Comment 6•19 years ago
|
||
As a usability point, we should choose strings solely on the basis of what will explain the concept best to the user; we should feel under no obligation to use strings from any other product unless they are identical to or very close to what we consider the optimal string.
Having said that, I don't think there are issues using strings from another product as long as a) you didn't cut-and-paste them, and b) they factually describe the options they are talking about.
Gerv
Assignee | ||
Comment 7•19 years ago
|
||
Kaspar, Martin, based on Gerv's comments I encourage you to use your own judgement as to what strings are best, and not to add "from XP" comments to our code.
Comment 8•19 years ago
|
||
Ok, then I would vote for using the OpenSSL explanations whereever possible, and leave the rest at what they currently are.
(In reply to comment #8)
> Ok, then I would vote for using the OpenSSL explanations whereever possible,
> and leave the rest at what they currently are.
I think v2 of the patch is already at this stage (with some very minor differences). Here's a short recap of my changes or additions:
+CertDumpEKU_1_3_6_1_5_5_7_3_9=OCSP Signing
+CertDumpEKU_1_3_6_1_4_1_311_2_1_21=Microsoft Individual Code Signing
+CertDumpEKU_1_3_6_1_4_1_311_2_1_22=Microsoft Commercial Code Signing
+CertDumpEKU_1_3_6_1_4_1_311_10_3_3=Microsoft Server Gated Crypto
+CertDumpEKU_2_16_840_1_113730_4_1=Netscape Server Gated Crypto
+CertDumpOCSPResponder=OCSP
+CertDumpCAIssuers=CA Issuers
These are all identical to OpenSSL.
+CertDumpEKU_1_3_6_1_4_1_311_10_3_2=Microsoft Time Stamping
+CertDumpEKU_1_3_6_1_4_1_311_10_3_4_1=Microsoft File Recovery
+CertDumpEKU_1_3_6_1_4_1_311_10_3_5=Microsoft Windows Hardware Driver Verification
+CertDumpEKU_1_3_6_1_4_1_311_10_3_10=Microsoft Qualified Subordination
+CertDumpEKU_1_3_6_1_4_1_311_10_3_11=Microsoft Key Recovery
+CertDumpEKU_1_3_6_1_4_1_311_10_3_12=Microsoft Document Signing
+CertDumpEKU_1_3_6_1_4_1_311_10_3_13=Microsoft Lifetime Signing
+CertDumpEKU_1_3_6_1_4_1_311_21_6=Microsoft Key Recovery Agent
+CertDumpMSCerttype=Microsoft Certificate Template Name
All these are not available in OpenSSL, so they are taken from XP (and they're all below 1.3.6.1.4.1.311, the Microsoft OID).
Finally, there are three cases where OpenSSL and XP differ:
+CertDumpEKU_1_3_6_1_4_1_311_10_3_4=Microsoft Encrypting File System
In OpenSSL, this is defined as "Microsoft Encrypt*ed* File System"
+CertDumpEKU_1_3_6_1_4_1_311_20_2_2=Microsoft Smart Card Logon
In OpenSSL, this is "Microsoft Smartcardlogin"
+CertDumpMSNTPrincipal=Microsoft Principal Name
In OpenSSL, defined as "Microsoft Universal Principal Name" - which is clearly a mistake - it should be "User Principal Name", cf. http://msdn.microsoft.com/library/en-us/adschema/adschema/a_upnsuffixes.asp). [In your patch, Martin, it's currently "Microsoft NT User Principal Name" - I think it makes sense to drop "NT" since UPN is used in Active Directory as well, it's no longer an NT thing.]
So, to sum up: I think that for the last three cases, we should not follow OpenSSL, but apart from that it seems to me that we have resolved our disagreement. Martin, can we proceed with the patch as it is now?
Comment 10•19 years ago
|
||
Ok, the patch is fine with me, then.
r=martin@v.loewis.de
Assignee | ||
Updated•19 years ago
|
Attachment #208870 -
Flags: review?(martin) → review+
Assignee | ||
Comment 11•19 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•19 years ago
|
||
Comment on attachment 208870 [details] [diff] [review]
proposed patch, v2
Who could verify this works as expected now?
Attachment #208870 -
Flags: branch-1.8.1?(kengert)
Reporter | ||
Comment 13•19 years ago
|
||
(In reply to comment #12)
> Who could verify this works as expected now?
I've just tested this with the latest nightly build (20060131) - when examining the server certificate from https://www.verisign.com, it will now correctly display the basic constraints extension (no bogus data for pathlen) and recognize both the Logotype extension and OCSP in the authority information access extension.
In order to illustrate the other fixes/changes introduced by this patch I'm attaching an additional sample certificate. It has an EKU extension with all recognized OIDs plus AIA and CDP extensions with multiline values. Additionally, the basic constraints extension has { CA:false, pathlen:5 } - a pretty nonsensical combination which will now be displayed as raw DER data.
Attachment #210249 -
Attachment mime type: application/x-x509-server-cert → text/plain
Assignee | ||
Comment 14•19 years ago
|
||
Comment on attachment 208870 [details] [diff] [review]
proposed patch, v2
Kaspar, thanks for the explanation and the example cert.
Martin and Kaspar, thanks to you both for working on this feature.
Attachment #208870 -
Flags: branch-1.8.1?(kengert) → branch-1.8.1+
Assignee | ||
Updated•19 years ago
|
Keywords: fixed1.8.1
Comment 15•19 years ago
|
||
I also want to express my thanks to Martin and Kaspar for their contributions.
Such contributions are just what PSM needs, and I appreciate it.
If you gentlemen should want to make any more such contributions, I can
think of lots of places that could use the help, such as bug 107491.
Thanks again.
Comment 16•15 years ago
|
||
Another bug that requires similar enhancement to PSM's list of displayable
names for OIDs is bug 500333.
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•