Closed Bug 230655 Opened 21 years ago Closed 16 years ago

Cert viewer does not show readable subject alt names

Categories

(Core Graveyard :: Security: UI, enhancement, P2)

Other Branch
x86
Windows 2000
enhancement

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: ken2006, Assigned: jgmyers)

References

()

Details

Attachments

(3 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040107
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040107

Not all details of a cert are reported, for example the
subject-alternative-name(s) in the example URL's cert are *.onnet.cc,
*.kensystem.om, & *.interactiveengines.com. However, none of these aliases can
be seen by the cert exam tool. 

When examining the cert served by https://www.kensystem.com, only the
*.interactiveengines.com CN can be seen, this may confuse some folks.

Reproducible: Always

Steps to Reproduce:
Actually, it DOES show all of them, but 
a) the extension OIDs are shown in decimal, not as meaningful strings, and 
b) the extension values are shown in hexadecimal, not ASCII or UTF8.
So, unless you can read the hex, and know the OIDs (as some of us do :-)
the display is pretty opaque.  

So, I am changing the summary of this bug report to be slightly more accurate.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Summary: Certificate viewer does not show alternate certificate subjects → Cert viewer does not show readable subject alt names
Severity: normal → enhancement
The OIDs were addressed by the fix to bug 97406.  Work on the values is blocked
by bug 232812, which has been awaiting review for a week.
Assignee: kaie → jgmyers
Depends on: 232812
I believe I need to use CERT_DecodeGeneralName() to implement this.  That
function is exported in nss.def, but it is declared in genname.h which NSS
doesn't export.  Nelson?
No longer depends on: 232812
It seems that we exported CERT_DecodeGeneralName for
PSM, but the PSM function that uses it has been commented
out.

In any case, if CERT_DecodeGeneralName is suitable for
being exported, we should move its declaration from
genname.h to cert.h.
Requires a newer version of NSS than is currently tagged.  Largely cribbed off
of nss/cmd/lib/secutil.c
Attachment #144285 - Flags: review?(ddrinan0264)
Attachment #144286 - Flags: review?(ddrinan0264)
Attachment #144287 - Flags: review?(ddrinan0264)
Attachment #144285 - Flags: review?(ddrinan0264) → review+
Attachment #144286 - Flags: review?(ddrinan0264) → review+
Attachment #144287 - Flags: review?(ddrinan0264) → review+
Attachment #144285 - Flags: superreview?(jag)
Attachment #144286 - Flags: superreview?(jag)
Attachment #144287 - Flags: superreview?(jag)
Comment on attachment 144286 [details] [diff] [review]
2 of 3: Make GetPIPNSSBundleString append instead of replace

Instead of doing this, perhaps you could pass in a parameter that specifies
whether to replace or append, and make it default to replace?
Comment on attachment 144287 [details] [diff] [review]
3 of 3: decode subject alt names and simple strings

In ProcessRawString and ProcessRawBytes, it looks like you can get into a
situation (data->len == 0 or data->data[data->len-1] == '\n') where you'll emit
|level| spaces without them being followed by anything.

How about:

static nsresult
ProcessRawString(SECItem *data, int level, nsAString &text)
{
  PRUint32 i;
  PRBool doIndent = PR_TRUE;
  ProcessIndent(level, text);
  for (i=0; i < data->len; i++) {
    if (doIndent) {
      ProcessIndent(level, text);
      doIndent = PR_FALSE;
    }
    char c = data->data[i];
    if (c == '\r' && i+1 < data->len && data->data[i+1] == '\n') continue;
    if (c == '\n') {
      text.Append(PRUnichar('\n'));
      doIndent = PR_TRUE;
      continue;
    }
    if (c < ' ' || c >= 0x7f) c = '.';
    text.Append(PRUnichar(c));
  }
  text.Append(PRUnichar('\n'));
  return NS_OK;
}    

Also, to aid l10n and RTL languages, instead of manually appending names etc.
to CertDumpGNameOther etc., put in a %S and use PIPBundleFormatStringFromName.

Looks good otherwise.
Attachment #144287 - Flags: superreview?(jag) → superreview-
Comment on attachment 144285 [details] [diff] [review]
1 of 3: Move cert details code to nsNSSCertHelper.cpp

Why did you move nsNSSCertificate::CreateTBSCertificateASN1Struct and
nsNSSCertificate::CreateASN1Struct?

Wouldn't it be better to expose the helper functions they need in
nsNSSCertHelper.h and keep the above functions in the nsNSSCertificate.cpp
file?
Comment on attachment 144285 [details] [diff] [review]
1 of 3: Move cert details code to nsNSSCertHelper.cpp

Moving the functions puts all of the code related to the cert details tab in
one file.  That allows us to avoid exposing functions across files and gives
the compiler better opportunities to make inter-function optimizations.

There is no advantage to having CreateTBSCertificateASN1Struct and
CreateASN1Struct in nsNSSCedrtificate.cpp
Comment on attachment 144285 [details] [diff] [review]
1 of 3: Move cert details code to nsNSSCertHelper.cpp

sr=jag

Could you address my questions/comments on the other two patches?
Attachment #144285 - Flags: superreview?(jag) → superreview+
Depends on: 259031
Blocks: 267515
Product: PSM → Core
Is it too late for any patch for this to make it into Deerpark betas? It SURE would be nice of we could fully inspect certificates in 1.5, especially subject-alt names.

Bug 281446 is a duplicate of this; please feel free to flag that, and/or communicate with any independent fixers there. Bug bug 259031 also appears related.
*** Bug 281446 has been marked as a duplicate of this bug. ***
Basically, this patch train got derailed by comment 8.  I never found the time to go and re-audit all of the code to implement an arbitrary design change.
Ken, it is too late for Firefox 1.5.  The next release
will be Firefox 2.0.
QA Contact: bmartin → ui
I think this got fixed a while ago.
When I use a trunk build and cert viewer to display the cert from the site mentioned in comment 0, cert viewer lists 4 subject alt names.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → WORKSFORME
Attachment #144286 - Flags: superreview?(jag)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.