408547 - Cert Viewer: when pretty printing extensions, improve hex dump display and indenting

Status: RESOLVED WONTFIX

(Core :: Security: PSM, enhancement)

Description

[Kaspar]

Attachment: Proposed patch

Bug 259031 added support for displaying the content of many extensions. The current implementation has some shortcomings, though:

- for unknown/unrecognized extensions, it displays a hex dump of the raw data, but does not show its ASCII counterpart. For a number of extensions, being able to see the ASCII representation of the data is often helpful as a further indication about the extension's content.

- for extensions with nested structures (e.g. the certificatePolicies extension, or the subjectAltName extension with X.500 names), the current string rendering and its indenting is sometimes confusing, making it difficult to determine what elements are at what nesting level.

The attached patch addresses these issues. Specifically, it includes the following enhancements:

1) adds two new parameters to ProcessRawBytes():
. "showASCII" (default=true), which adds a visual representation at the end of each line (the standard hex+ASCII display)
. "indentlevel" (default=0), which specifies the indentation at the beginning of each line

2) changes the wantHeader default in ProcessRawBytes() to PR_FALSE, and explicitly set it to true for the following callers, only:
. ProcessSECAlgorithmID() ("Algorithm Parameters")
. ProcessSubjectPublicKeyInfo() ("Subject's Public Key")
. CreateASN1Struct() ("Certificate Signature Value")
[For these values, the size can help to determine key lengths etc. - it doesn't make sense to add the header for others, IMO. I assume this was the primary purpose when bug 328302 was addressed.]

3) disables showASCII for the following callers:
. ProcessSubjectKeyId() and ProcessAuthKeyId() ("Key ID", "Serial Number")
. ProcessSubjectPublicKeyInfo(), for RSA keys ("Modulus"/"Exponent")
. CreateASN1Struct() ("Certificate Signature Value")
[These are very unlikely to contain any human readable strings, and would only show random ASCII garbage.]

4) implements consistent indenting for the following elements:
. for GeneralNames. In particular, this improves the display of the certDirectoryName type ("X.500 name")
. for the certificatePolicies extension (also for its UserNotice subcomponent): the policy OIDs appear at level 0, cPSuri and userNotice at level 1, and their content at level 2.
. for subjectKeyIdentifier and authorityKeyIdentifier extensions: the "Key ID" and "Serial Number" values appear at level 1

5) for the CDP and AIA extensions: if decoding of the URI (a GeneralName) fails, display raw bytes for that specific entry only (instead of dumping the extension as a whole in hex).

The changes might appear bigger than they are, in fact. In most cases, they either replace text.Append(NS_LITERAL_STRING(...)) by the Indent() function, or set the appropriate parameters for the ProcessRawBytes() calls. I will attach a sample certificate which allows to see the effects of these changes rather quickly.

Kai, do you have time for a review, or should I ask somebody else (Bob R)? It would really be nice if we could land this for Fx 3.

Comment 1

[Kaspar]

Attachment: Sample certificate, includes extensions demonstrating the display changed introduced by this patch

Many changes proposed by this patch can be illustrated by showing the rendering of the subjectAltName or certificatePolicies extensions of this sample certificate:

--- subjectAltName extension, w/o patch -------
Not Critical
X.500 Name: DC = foo
DC = example
DC = net

1 3 6 1 5 5 7 8 5: Size: 17 Bytes / 136 Bits
0c 0f 66 6f 6f 2e 65 78 61 6d 70 6c 65 2e 6e 65 
74 
DNS Name: foo.example.net
1 3 6 1 5 5 7 8 7: Size: 19 Bytes / 152 Bits
16 11 5f 73 6d 74 70 2e 65 78 61 6d 70 6c 65 2e 
6e 65 74 
E-Mail Address: [e-mail]
IP Address: 127.0.0.1
-----------------------------------------------

--- subjectAltName extension, with patch ------
Not Critical
X.500 Name: 
  DC = foo
  DC = example
  DC = net
1.3.6.1.5.5.7.8.5: 
  0c 0f 66 6f 6f 2e 65 78    ..foo.ex
  61 6d 70 6c 65 2e 6e 65    ample.ne
  74                         t
DNS Name: foo.example.net
1.3.6.1.5.5.7.8.7: 
  16 11 5f 73 6d 74 70 2e    .._smtp.
  65 78 61 6d 70 6c 65 2e    example.
  6e 65 74                   net
E-Mail Address: [e-mail]
IP Address: 127.0.0.1
-----------------------------------------------


--- certificatePolicies extension, w/o patch -------
Not Critical
2.5.29.32.0:
  Certification Practice Statement pointer:
    http://repository.example.net/cps/
  User Notice: 
    This is an explicit text encoded as a BMPString.
  User Notice: Example, Inc. - #7, #8, #9
----------------------------------------------------

--- certificatePolicies extension, with patch ------
Not Critical
2.5.29.32.0:
  Certification Practice Statement pointer:
    http://repository.example.net/cps/
  User Notice:
    This is an explicit text encoded as a BMPString.
  User Notice:
    Example, Inc. - #7, #8, #9
----------------------------------------------------

Comment 2

[Kai Engert (:KaiE:)]

Comment on attachment 293372 [details] [diff] [review]
Proposed patch

Patch no longer applies.

I apologize that I never found the time to get to this review.

Comment 3

[Kaspar]

(In reply to Kai Engert (:kaie) from comment #2)
> I never found the time to get to this review.

Which is also the reason I stopped working on PSM patches years ago.

Comment 4

[Kaspar]

As there has been some recent activity on Cert Viewer improvements, thankfully (bug 622332, bug 932116, or bug 972702 e.g.), I wonder if this could be an opportunity to resurrect this patch. I'm going to attach an unbitrotten version next.

Comment 5

[Kaspar]

Attachment: Proposed patch, unbitrotten

The patch from attachment 293372 [details] [diff] [review], updated to trunk

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8432063 [details] [diff] [review]
Proposed patch, unbitrotten

Review of attachment 8432063 [details] [diff] [review]:
-----------------------------------------------------------------

There are a few style issues with this patch. Unfortunately, this file was written with a style different from what we use today, but we should be trying to move towards the new style as much as possible/reasonable. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style (Along those lines, as a general comment, use two spaces instead of a tab.)
That said, thanks for working on this patch, but I don't think firefox should implement an all-purpose certificate examiner that can handle anything. There are other, better tools for that.

::: security/manager/ssl/src/nsNSSCertHelper.cpp
@@ +620,5 @@
>    return rv;  
>  }
>  
> +static void
> +Indent(nsAString &text, PRUint32 level)

nit: nsAString&

@@ +623,5 @@
> +static void
> +Indent(nsAString &text, PRUint32 level)
> +{
> +  // each level means indenting by two spaces
> +  for (PRUint32 i = 0; i < level; i++)

nit: braces around the body of any conditional, loop, etc.

for (...) {
  ...
}

@@ +631,5 @@
>  #define SEPARATOR "\n"
>  
>  static nsresult
>  ProcessRawBytes(nsINSSComponent *nssComponent, SECItem *data, 
> +                nsAString &text, bool wantHeader = true,

The style we're trying to adhere to is to have the pointer/reference indicator in types be next to the type name. So, this should be "nsINSSComponent* nssComponent", etc.. Added/modified lines should definitely use this style. It would be great if lines near what you're modifying were updated as well. (Also, there's trailing whitespace on the previous line here - please fix that as well.)

@@ +684,5 @@
>      PR_snprintf(buffer, 5, "%02x ", data->data[i]);
>      AppendASCIItoUTF16(buffer, text);
> +    if (showASCII) {
> +      // the number of bytes we've output so far
> +      PRUint32 written = i+1;

nit: spaces around operators: "i + 1"

@@ +689,5 @@
> +      if (written % 8 == 0 || written == data->len) {
> +        // the number of bytes on the current line
> +        PRUint32 llen = (written % 8) ? (written % 8) : 8;
> +        PRUint32 j;
> +        for (j = 0; j <= 8-llen; j++)

nit: braces

@@ +690,5 @@
> +        // the number of bytes on the current line
> +        PRUint32 llen = (written % 8) ? (written % 8) : 8;
> +        PRUint32 j;
> +        for (j = 0; j <= 8-llen; j++)
> +          text.AppendLiteral("   ");

use Indent?

@@ +693,5 @@
> +        for (j = 0; j <= 8-llen; j++)
> +          text.AppendLiteral("   ");
> +        for (j = written-llen; j < written; j++) {
> +          const char c = data->data[j];
> +          if (c > 31 && c < 127)

nit: braces

@@ +694,5 @@
> +          text.AppendLiteral("   ");
> +        for (j = written-llen; j < written; j++) {
> +          const char c = data->data[j];
> +          if (c > 31 && c < 127)
> +            text.Append(NS_ConvertUTF8toUTF16(&c, 1));

AppendASCII

@@ +695,5 @@
> +        for (j = written-llen; j < written; j++) {
> +          const char c = data->data[j];
> +          if (c > 31 && c < 127)
> +            text.Append(NS_ConvertUTF8toUTF16(&c, 1));
> +          else

nit: braces

@@ +962,5 @@
>    return NS_OK;
>  }
>  
>  static nsresult
> +ProcessName(CERTName *name, nsINSSComponent *nssComponent, char16_t **value,

nit: "CERTName* name", etc.

@@ +1466,5 @@
>  	     of the Qualifier choice. */
>  	  rv = ProcessIA5String(&policyQualifier->qualifierValue,
>  				text, nssComponent);
>  	  if (NS_FAILED(rv))
> +	    ProcessRawBytes(nssComponent, &policyQualifier->qualifierValue,

A couple of issues here:
1. ProcessIA5String could fail for reasons other than an unknown encoding (e.g. out-of-memory error)
2. rv will still be a failing nsresult here, so when this function returns, it'll indicate failure

@@ -1407,5 @@
>  	     of the Qualifier choice. */
>  	  rv = ProcessIA5String(&policyQualifier->qualifierValue,
>  				text, nssComponent);
>  	  if (NS_FAILED(rv))
> -	    goto finish;

Looks like with this removal, the label finish is no longer used in this function - it should be removed.

Comment 7

[Kaspar]

Attachment: Proposed patch v2

Thank you for the very prompt review, David. I tried to address your comments/concerns with this updated version, which I hope you might still consider worth getting into the tree.

> That said, thanks for working on this patch, but I don't think firefox
> should implement an all-purpose certificate examiner that can handle
> anything. There are other, better tools for that.

That's true, and the patch does not really add support for parsing even more extensions (like bug 259031 did at the time). What this patch does is a) enhance the display of hex data (being able to see its ASCII representation can be helpful) and b) improve the rendering of supported extensions by applying more consistent indenting.

For b), compare the current display of the AKID extension of the "AddTrust External CA Root":

--- authorityKeyIdentifier extension, w/o patch ---
Not Critical
Size: 20 Bytes / 160 Bits
ad bd 98 7a 34 b4 26 f7 fa c4 26 54 ef 03 bd e0 
24 cb 54 1a 
Issuer: X.500 Name: CN = AddTrust External CA Root
OU = AddTrust External TTP Network
O = AddTrust AB
C = SE

Serial Number: 1
---------------------------------------------------

This looks rather messy, and makes it fairly hard to grasp the real structure of this extension. With the patch, it is properly indented and rendered as follows:

--- authorityKeyIdentifier extension, with patch ---
Not Critical
Key ID:
  ad bd 98 7a 34 b4 26 f7 fa c4 26 54 ef 03 bd e0 
  24 cb 54 1a 
Issuer:
  X.500 Name: 
    CN = AddTrust External CA Root
    OU = AddTrust External TTP Network
    O = AddTrust AB
    C = SE
  Serial Number: 1
---------------------------------------------------

> @@ +690,5 @@
> > +        // the number of bytes on the current line
> > +        PRUint32 llen = (written % 8) ? (written % 8) : 8;
> > +        PRUint32 j;
> > +        for (j = 0; j <= 8-llen; j++)
> > +          text.AppendLiteral("   ");
> 
> use Indent?

It's actually three spaces (deliberately), so Indent can't be used. (Furthermore, it's not really indenting which is happening here [we're not at the beginning of a new line], but more a sort of padding.)

> @@ +1466,5 @@
> >  	     of the Qualifier choice. */
> >  	  rv = ProcessIA5String(&policyQualifier->qualifierValue,
> >  				text, nssComponent);
> >  	  if (NS_FAILED(rv))
> > +	    ProcessRawBytes(nssComponent, &policyQualifier->qualifierValue,
> 
> A couple of issues here:
> 1. ProcessIA5String could fail for reasons other than an unknown encoding
> (e.g. out-of-memory error)
> 2. rv will still be a failing nsresult here, so when this function returns,
> it'll indicate failure

My idea was that if we fail to properly decode the string (SEC_ASN1DecodeItem failing in ProcessIA5String), we simply render it in hex, *and* continue with the next policyQualifier (that's a real problem of the current code, IMO - it will abort further processing completely). AFAICT, NS_FAILED(rv) should properly detect the case of a failed SEC_ASN1DecodeItem, or am I overlooking something?

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8440856 [details] [diff] [review]
Proposed patch v2

Review of attachment 8440856 [details] [diff] [review]:
-----------------------------------------------------------------

Just letting you know: I have a lot on my plate right now, and I might take a while to address this patch.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8440856 [details] [diff] [review]
Proposed patch v2

Review of attachment 8440856 [details] [diff] [review]:
-----------------------------------------------------------------

I'm concerned about the default values of ProcessRawBytes. It seems that in many cases wantHeader is being changed from true to false. Are you intending that this change include not displaying the header in these cases? Also, showASCII is now true in most cases. Let's be explicit about what each call to ProcessRawBytes wants, which means not having default arguments.
Stylistically, this looks good. I would appreciate it if the hexdump functionality were factored out into a separate function, however.
Additionally, I would be remiss in my duties as PSM peer if I didn't require tests for this patch, so r- for now.

::: security/manager/ssl/src/nsNSSCertHelper.cpp
@@ +587,5 @@
>    return rv;  
>  }
>  
> +static void
> +Indent(nsAString& text, PRUint32 level)

This should probably be called "PreIndentLine" or something that indicates that it doesn't indent what's currently there, but what will be there.

@@ +654,5 @@
>      AppendASCIItoUTF16(buffer, text);
> +    if (showASCII) {
> +      // the number of bytes we've output so far
> +      PRUint32 written = i + 1;
> +      if (written % 8 == 0 || written == data->len) {

This should be factored out into a general hexdump implementation (do we not already have one?) I think it would make it easier to be convinced that we won't, for instance, do an out of bounds read on the input data.

@@ +1027,5 @@
>  		      d[3], d[2], d[1], d[0], d[5], d[4], d[7], d[6],
>  		      d[8], d[9], d[10], d[11], d[12], d[13], d[14], d[15]);
>  	  value.AssignASCII(buf);
>  	} else {
> +          ProcessRawBytes(nssComponent, &current->name.OthName.name, value,

Looks like we're going from wantHeader = true (by default) to wantHeader = false. Why?

@@ +1034,5 @@
>      } else {
>        rv = GetDefaultOIDFormat(&current->name.OthName.oid, nssComponent, key, ' ');
>        if (NS_FAILED(rv))
>  	goto finish;
> +      ProcessRawBytes(nssComponent, &current->name.OthName.name, value,

Same question - why no header now?

@@ +1049,5 @@
>      value.AssignASCII((char*)current->name.other.data, current->name.other.len);
>      break;
>    case certX400Address:
>      nssComponent->GetPIPNSSBundleString("CertDumpX400Address", key);
> +    ProcessRawBytes(nssComponent, &current->name.other, value, false,

Same question.

@@ +1061,5 @@
>        goto finish;
>      break;
>    case certEDIPartyName:
>      nssComponent->GetPIPNSSBundleString("CertDumpEDIPartyName", key);
> +    ProcessRawBytes(nssComponent, &current->name.other, value, false,

Same question.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

It doesn't look like there's either a lot of demand for this or much interest in completing it. In any case, augmenting the existing C++ code that implements the certificate viewer is not the direction we should be going in. Any effort at this point should be towards a reimplementation in JS.