78012 - Finish up the Certificate Viewer for PSM 2

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Javier Delgadillo]

This is a tracking bug for the ongoing work being done for the certificate
viewer in PSM 2.0

Comment 1

[Javier Delgadillo]

Attachment: Current patch to get cert viewer up and running for PSM 2

Comment 2

[Bob Lord]

*** Bug 76284 has been marked as a duplicate of this bug. ***

Comment 3

[Javier Delgadillo]

Attachment: New patch which uses more of NSS

Comment 4

[Terry Hayes]

r=thayes

Comment 5

[David P. Drinan]

Adding [e-mail] to cc-list.

Scott,

Please super review this bug.

Thanks.

Comment 6

[Christopher Blizzard (:blizzard)]

+  char *serialNumber = CERT_Hexify(serialItem, 1);
+  if (serialNumber == nsnull)
+    return NS_ERROR_OUT_OF_MEMORY;
+
+  rv = printableItem->SetDisplayValue(NS_ConvertASCIItoUCS2(serialNumber).get());
+  nsMemory::Free(serialNumber);

You can use an nsXPIDLCString there.

+    j = oid->data[i];
+    val = (val << 7) | (j & 0x7f);
+    if (j & 0x80)
+      continue;
+    written = PR_snprintf(&buf[len], sizeof(buf)-len, "%lu ", val);

What the hell does that do?  Can we use some self describing macros, defines or
at least some comments?

+static nsresult
+ProcessRawBytes(SECItem *data, nsString &text)
+{
+  PRUint32 i;
+  char buffer[5];
+  for (i=0; i<data->len; i++) {
+    PR_snprintf(buffer, 5, "%02x ", data->data[i]);

once again, comments, please.

+  PRUnichar *value;
+  GetIssuerName(&value);
+
+  printableItem = new nsNSSASN1PrintableItem();
+  if (printableItem == nsnull)
+    return NS_ERROR_OUT_OF_MEMORY;
+
+  printableItem->SetDisplayValue(value);
+  nsMemory::Free(value);

Are you getting sick of my XPIDLString-fu yet?

+    data.data = mCert->issuerID.data;
+    data.len  = mCert->issuerID.len / 8;

Wow, is len in bits or something?  Can you comment that?

+  char *title;
+  GetWindowTitle(&title);
+  
+  mASN1Structure->SetDisplayName(NS_ConvertASCIItoUCS2(title).get());

Did title leak?  I can't find the release unless GetWindowTitle doesn't actually
allocate a new string or I'm just blind.

+/* long getLevel (in long index); */
+NS_IMETHODIMP 
+nsNSSASN1Outliner::GetLevel(PRInt32 index, PRInt32 *_retval)
+{
+  *_retval = GetLevelsTilIndex(index, mASN1Object);
+  if (*_retval == -2)
+    *_retval = -1;
+  return NS_OK;
+}

That looks like voodoo to me.  Comment?

+static int
+getInteger256(unsigned char *data, unsigned int nb)
+{
+    int val;
+
+    switch (nb) {
+      case 1:
+        val = data[0];
+        break;
+      case 2:
+        val = (data[0] << 8) | data[1];

Comments, please.  Same with getDERItemLength().

It seems like mData in class nsNSSASN1PrintableItem could be something other
than just a raw char * to avoid the memory management but it's your call on that
one.

Address the issues listed above and sr=blizzard.

Comment 7

[Javier Delgadillo]

+  char *serialNumber = CERT_Hexify(serialItem, 1);
+  if (serialNumber == nsnull)
+    return NS_ERROR_OUT_OF_MEMORY;
+
+  rv = 
printableItem->SetDisplayValue(NS_ConvertASCIItoUCS2(serialNumber).get());
+  nsMemory::Free(serialNumber);

>You can use an nsXPIDLCString there.

Done

+    j = oid->data[i];
+    val = (val << 7) | (j & 0x7f);
+    if (j & 0x80)
+      continue;
+    written = PR_snprintf(&buf[len], sizeof(buf)-len, "%lu ", val);

What the hell does that do?  Can we use some self describing macros, defines or
at least some comments?
>Comments added.  Used as the generic display for Object Identifier (OID) that
>we have not added support for displaying in a user readable format.

+static nsresult
+ProcessRawBytes(SECItem *data, nsString &text)
+{
+  PRUint32 i;
+  char buffer[5];
+  for (i=0; i<data->len; i++) {
+    PR_snprintf(buffer, 5, "%02x ", data->data[i]);

once again, comments, please.
>Comments added. Used to display a DER buffer that we don't support interpreting
>in a user readable format.

+  PRUnichar *value;
+  GetIssuerName(&value);
+
+  printableItem = new nsNSSASN1PrintableItem();
+  if (printableItem == nsnull)
+    return NS_ERROR_OUT_OF_MEMORY;
+
+  printableItem->SetDisplayValue(value);
+  nsMemory::Free(value);

Are you getting sick of my XPIDLString-fu yet?
>Now using nsXPIDLString here.

+    data.data = mCert->issuerID.data;
+    data.len  = mCert->issuerID.len / 8;

Wow, is len in bits or something?  Can you comment that?
>Is in fact converting from bit string length to byte length. Added comments
>to the 4 or so places I did this.
+  char *title;
+  GetWindowTitle(&title);
+  
+  mASN1Structure->SetDisplayName(NS_ConvertASCIItoUCS2(title).get());

Did title leak?  I can't find the release unless GetWindowTitle doesn't actually
allocate a new string or I'm just blind.
>Yes title leaked.  Now uses nsXPIDLCString

+/* long getLevel (in long index); */
+NS_IMETHODIMP 
+nsNSSASN1Outliner::GetLevel(PRInt32 index, PRInt32 *_retval)
+{
+  *_retval = GetLevelsTilIndex(index, mASN1Object);
+  if (*_retval == -2)
+    *_retval = -1;
+  return NS_OK;
+}

That looks like voodoo to me.  Comment?
>The if statement was actually no longer necessary.  It was removed.

+static int
+getInteger256(unsigned char *data, unsigned int nb)
+{
+    int val;
+
+    switch (nb) {
+      case 1:
+        val = data[0];
+        break;
+      case 2:
+        val = (data[0] << 8) | data[1];

Comments, please.  Same with getDERItemLength().

>Comments added to both.

It seems like mData in class nsNSSASN1PrintableItem could be something other
than just a raw char * to avoid the memory management but it's your call on that
one.
>The nature of DER is that I have to deal with unsinged char/char buffer arrays.
>So I'll keep this for now.

Comment 8

[Javier Delgadillo]

Fix checked in.

Comment 9

[John Unruh]

Verified.