259031 - Patch: Add support for displaying certificate extensions

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Martin v. Löwis]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20040820 Debian/1.7.2-4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.2) Gecko/20040820 Debian/1.7.2-4

This patch adds support for various extensions occurring in certificates, which
are currently only displayed in their DER form. In particular, support is added for
- various PKIX extensions (basic constraints, alternative names, CRL
distribution points, AIA, extended key usage, key IDs, and certificate policies)
- Netscape extensions (all extensions that have an IA5String as their value)
- Microsoft extensions (certificate type, CA version, Principal name, Server GUID)
- Verisign extensions (Verisign user notices)

To support these changes, a number of changes have been made to NSS, and the
existing dumping code:
- OIDs can now be printed in the dotted form as well
- a number of OID symbolic numbers have been added
- a number of additional functions are now exported from NSS, for decoding
well-known datatypes


Reproducible: Always
Steps to Reproduce:
1. Open certificate manager
2. navigate to any of the builtin CAs with extensions in their certificates
3. try reading the extension value

Actual Results:  
It displays DER

Expected Results:  
It should display legible text whereever possible.

I hope I can attach the patch somehow.

Comment 1

[Martin v. Löwis]

Attachment: Proposed patch

Comment 2

[Martin v. Löwis]

I believe this patch fixes bug 230655, bug 127052, bug 161140.

Comment 3

[Andrew Schultz]

marking NEW (has patch)

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

A bug with a patch.  Hope some PSM reviewer can review it soon.

Comment 5

[Kai Engert [:KaiE:]]

Making bugs mentioned by Martin dependent on this one.

Comment 6

[Kai Engert [:KaiE:]]

Martin,

thanks a lot for your patch. You have coded a lot of details!

I do not have the time to play with your patch and test it, but if you are sure
it works, I think we should get it into Mozilla.

However, I would like you to do the following:
- please test it with various certificates, but given the complexity of your
patch, I'm sure you have already done so
- please ask an NSS expert, either Nelson Bolyard or Bob Relyea, to briefly
review your calls to NSS functions
- please make sure you don't introduce NSS resource leaks with your new code.
The best way to test is, after having executed your new code, to use the
"Tools/Switch Profile" menu command, which requires a re-init of NSS, which only
works if there are no NSS leaks.

I'm giving you r=kaie for changes made in directory mozilla/manager which means
you have "module owner approval" to check it in, once you have done the above.

However, your patch also includes changes to NSS. I assume those changes are not
yet contained in NSS.
If your Mozilla UI patch depends on the NSS library changes, please file a
separate bug on NSS, and attach a patch containing only your NSS changes, and
make your bug dependent on the other one. We will need to wait for the NSS
snapshot marked with NSS_CLIENT_TAG to contain your changed, before you can
check in this one.

And you will still need a usual Mozilla superreview.

I'm assigning the bug to you, in the hope you are able and willing to drive the
various actions required before checking in is possible.
If you're not, please let us know.

Thanks!

Comment 7

[Kai Engert [:KaiE:]]

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

r=kaie - provided that you do what I suggested in comment 6

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Any NSS changes need to be in a separate patch.  checkin access to NSS
is administered separately, and only an NSS developer can checkin to 
NSS, presently.  There are several issues with the proposed NSS changes 
in the patch.  Let me explain several.

1. This patch appears to me to be based on NSS 3.9.x which is now over a 
year old.  No more NSS 3.9.x releases are planned.  NSS 3.10 is the 
next planned release.  Some of the work shown in this patch is already
done in NSS 3.10.  

2. In NSS 3.10, there is a way for an application to dynamically extend
the OID table at run time.  This allows applications to add to the known
OIDs without every application burdening NSS with its OIDs.  So, the 
proper thing for mozilla to do is to use that API to add OIDs at run
time, rather than adding them to the NSS sources.  You can find an 
example of code that does this very thing at 
http://lxr.mozilla.org/security/source/security/nss/cmd/lib/moreoids.c

3. Obviously if the PSM changes depend on the proposed NSS changes, then
the PSM patch will have to wait until NSS 3.10 is released (or until 
moz/FF/TB/switch to the NSS 3.10 sources) before it can be checked in.

Comment 9

[Martin v. Löwis]

I'm willing to drive the changes necessary. AFAICT, it looks like this:
the patch needs changes to NSS 3.9, which are unlikely to be accepted anymore.
So I need to update my patch for NSS 3.10, which then means that integration of
the rest of the patch needs to be delayed until some? all? PSM applications have
switched to NSS 3.10.

I'm uncertain about the OID changes: are you suggesting that I should avoid
adding more OID constants to NSS, or that I should add them to moreoids.c, or
that adding them to the constant list is still fine? I likely need to change NSS
anyway, to support decoding more extension types.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Martin,  Thanks for your willingnews to contribute!

moreoids.c is code from a command line application that is part of the
NSS test/QA suite, but is not part of the NSS shared libraries.  
I'm suggesting that you use it as an example of what mozilla PSM code 
should do.  I am not suggesting that you add oids to NSS source code.  
I am suggesting that you add oids in PSM source code, using the method
demonstrated in moreoids.c 

I doubt that you need to add any code to NSS 3.10.  NSS does not need to 
contain templates and functions to parse every known cert extension.  
It only needs code to parse the extensions that are relevant to cert 
validity checking or other cert processing done by NSS itself.  

If applications want to process additional extensions, NSS provides the
functions by which they may do so themselves.  The ASN.1 DER and BER 
decoders are publicly callable, and work based on "templates" that 
describe the ASN.1 syntax, and the related structure.  Application are 
free to create their own templates.  The new templates need not be in NSS.  

If you're adding templates to parse *standard* extensions, I think that
would be welcome in NSS shared libs.  But the kinds of (mostly proprietary)
extensions you see in moreoids.c are example of extensions that belong in
applciations, not in NSS, IMO. 

The NSS development team maintains a CVS tag named "NSS_CLIENT_TAG" that 
the mozilla applications use to pull NSS for their trunk development. 
Presently it is on the NSS 3.9 branch.  But at some point, hopefully 
before too long, it will move to the NSS 3.10 sources.  At that point, 
the mozilla applications that pull NSS_CLIENT_TAG will all switch to 3.10.

Comment 11

[Martin v. Löwis]

I have checked NSS 3.10, and I think these changes would be needed:
- add CERT_DestroyUserNotice to nss.def
- add SEC_OID_ACCESS_DESCR_CA_ISSUERS as a OID constant,  (this is a standard
OID from PKIX 3)
I'm going to submit this as a patch to NSS, and update the PSM patch to use NSS
3.10.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #11)
> I have checked NSS 3.10, and I think these changes would be needed:
> - add CERT_DestroyUserNotice to nss.def
> - add SEC_OID_ACCESS_DESCR_CA_ISSUERS as a OID constant,  (this is a standard
> OID from PKIX 3)

Sounds reasonable to me.  Please file a separate NSS bug about those,
and mark this bug as deepending on that one.  Please cc me on that bug. Thanks.

Comment 13

[Martin v. Löwis]

Attachment: Patch version 2

I've updated the patch to rely on the two NSS features for which I've submitted
bug reports.

I have tested the patch with various real-world certificates that I could find.


To allows others to reproduce the tests, I could try to come up with a
synthetic certificate that demonstrates all the extensions (although one may
need a set of certificates to cover all CHOICEs). If it is desired that I
create such a thing, please also indicate whether I should try to do so through
NSS or openssh. Notice that neither package naturally supports all extensions
in newly-created certificates, so I would have to escape to plain DER bytes, in
which case there is no guarantee that the resulting test certificate would
really comply with the	standards.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 13, create the test certs with any tool you like,
as long as the results are properly encoded standards-comforming certs.
The DER-encoded (and perhaps Base64 encoded) certs themselves, not the
scripts/tools that create them, should be attached to the bug and 
eventually become part of the QA test suite.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Martin, both NSS features that you had requested were implemented in NSS 3.10
and are now part of the mozilla nightly builds.  So, please feel free to 
continue the work on this bug.  

Comment 16

[Martin v. Löwis]

Attachment: Patch v3 based on NSS 3.10

I have now updated the patch to NSS 3.10. Working on the demo certificate will
take more time.

Comment 17

[Robert Relyea]

Note on the attachment label: none of the changes made are to NSS, so it's a
patch to PSM, not NSS (NSS is the code that lives under mozilla/security/nss and
is shared between clients and servers).

This note should not be construed as some sort of rejection of the patch (I have
not reviewed it). The patch is made to exactly the code that it should be made
to (PSM, or the code under mozilla/security/manager).

Comment 18

[Martin v. Löwis]

Sorry for the misunderstanding. I meant to say it is a patch *based on* NSS
3.10, not a patch *to* NSS 3.10. I.e. the patch requires features only present
in NSS 3.10 - it is a patch *to* the PSM.

Comment 19

[Martin v. Löwis]

Attachment: Patch v4

Update to the patch, tested with test certificate. This adds a few more
certificate usages (as present in the test certificate), and deals more
gracefully with a failure to decode the user notice.

Comment 20

[Martin v. Löwis]

Attachment: Test certificate

This is a test certificate demonstrating the patch. It features all extensions
that OpenSSL SNAP-20050405 is capable of generating. The patch can display all
of them, except for the UserNotice in the policy. There is an obvious bug in
NSS 3.10's DecodeUserNotice (which assumes that the organization is IA5String,
but it does not need to be). However, in the test certificate, this bug is
worked-around, and it still fails to decode the certificate for some reason; I
was not able to figure out where the problem is. I believe the patch deals with
this case gracefully, by adding some alternative text and cleaning up what can
be cleaned.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Martin, if you haven't already, please file an NSS bug on the DecodeUserNotice
issues.  Thanks.

Comment 22

[Martin v. Löwis]

I hadn't done this before, but it now is bug #289823.

Comment 23

[Robert Relyea]

Comment on attachment 180313 [details] [diff] [review]
Patch v4

r+ relyea.

Comment 24

[Wan-Teh Chang]

Comment on attachment 180313 [details] [diff] [review]
Patch v4

For this amount of code, you should put your name in
the Contributors section in the license header in the
files you modified.

Bob Relyea is not a Mozilla super reviewer, so you need
to get a super review from someone else.  They are listed
on http://www.mozilla.org/hacking/reviewers.html.  If
you don't know any of them, I suggest [e-mail]
or [e-mail].

Comment 25

[Robert Relyea]

Comment on attachment 180313 [details] [diff] [review]
Patch v4

that should be r+ relyea, not sr+ relyea ;)

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

patch v4 containa a comment that says, in part:
+  /* XXX: currently, polcyxtn.c assumes that organization
+     is an IA5String, whereas it really ought to be a
+     choice of VisibleString, BMPString, and UTF8String.

RFC 3280 says 

   The DirectoryString type is defined as a choice of PrintableString,
   TeletexString, BMPString, UTF8String, and UniversalString.  The
   UTF8String encoding [RFC 2279] is the preferred encoding, and all
   certificates issued after December 31, 2003 MUST use the UTF8String
   encoding of DirectoryString (except as noted below).

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

One more point regarding the above comment.
NSS functions for parsing cert components convert all of the directory string
types to UTF8 internally, IINM.  The values returned by the various CERT_
function that return cert components all return strings that have been 
converted to UTF8, I believe.  

Comment 28

[Jean-Marc Desperrier]

It doesn't seem really optimal to have in ProcessExtKeyUsage a separate case for
each possible KeyUsage in the code.

Wouldn't it be better to generate the bundlekeys based on the OID ? That would
shorten the code, and only require to change the ressource to add new OIDs.
bundlekeys would be less explicit, but does it matter ?

AppendBMPtoUTF16 :
 /* XXX instead of converting to and from UTF-8, it would
     be sufficient to just swap bytes, or do nothing */

This part introduces a call to PORT_UCS2_UTF8Conversion from security/manager.
It never happens elsewhere. This is a hint of something wrong, I think.

I believe everywhere else Mozilla interfaces with NSS at a level where it
receives already decoded UTF-8 string. In this patch, it goes much lower. The
only problem with that is that it incorporates inside Mozilla code that could be
useful for other NSS users. 

Maybe part of that code could be part of NSS in a manner similar to the CRMF
code, that is not included in NSS3 or another shared library, but in a separate
static library that anyone who really needs that part can incorporate.

Comment 29

[Martin v. Löwis]

(In reply to comment #26)
> RFC 3280 says 
> 
>    The DirectoryString type

However, the organization is not a DirectoryString, it is a DisplayText

DisplayText ::= CHOICE {
        ia5String        IA5String      (SIZE (1..200)),
        visibleString    VisibleString  (SIZE (1..200)),
        bmpString        BMPString      (SIZE (1..200)),
        utf8String       UTF8String     (SIZE (1..200)) }

The alternative of IA5String is new in RFC 3280 (was not in RFC 2459);
apparently in response to the common misunderstanding what precisely
goes into the organization field.

Comment 30

[Martin v. Löwis]

(In reply to comment #28)
> AppendBMPtoUTF16 :
>  /* XXX instead of converting to and from UTF-8, it would
>      be sufficient to just swap bytes, or do nothing */
> 
> This part introduces a call to PORT_UCS2_UTF8Conversion from security/manager.
> It never happens elsewhere. This is a hint of something wrong, I think.

I think it is not surprising that this not used elsewhere: BMPString is
an uncommon type. It is only needed in two places in the patch, in one
alternative of DisplayText (which typically will contain IA5String, so
the code is not executed), and in Microsoft certificate extensions.

So I think reuse is unlikely, and the conversion via UTF-8 is used so
infrequently that the slight performance hit will not matter much in
practice.

Comment 31

[Martin v. Löwis]

(In reply to comment #28)
> Wouldn't it be better to generate the bundlekeys based on the OID ? That would
> shorten the code, and only require to change the ressource to add new OIDs.
> bundlekeys would be less explicit, but does it matter ?

I don't know how to do that: what procedure should be used to generate them,
and how would that integrate with the rest of the properties file? I have
no idea how mozilla localization works in the first place - the workload for
translators would not change at all, would it?

Comment 32

[Jean-Marc Desperrier]

Comment #31 :
In fact, it's the code :
    switch (oidTag) {
      case SEC_OID_EXT_KEY_USAGE_SERVER_AUTH: 
        bundlekey = "CertDumpServerAuth";
        break;
      case XXX : 
      [... repeat for n identifier ]
    default:
      if (oidTag ==  SEC_OID(MS_KEY_USAGE_CODE_IND)) {
	bundlekey = "CertDumpMSCodeInd";
	break;[...]
      if (oidTag ==  SEC_OID(XXX)) 
      [... repeat for n identifier ]
    if (bundlekey) {
      rv = nssComponent->GetPIPNSSBundleString(bundlekey, local);
    }
which I find really ugly. 
It's a lot of code to convert OID values to a bundlekey which *only* use is to
get the real description from the properties file. And the more value we
support, the more it grows. Adding a new value means both modifying that and
pipnss.properties. 

I think it would like it better to replace it all with something like (beware,
the string usage might be bad) :
      nsAutoString bundleref;
      rv = GetDefaultOIDFormat(oid, bundleref, true);
      if (NS_FAILED(rv))
        return rv;

      nsAutoString bundlekey = NS_LITERAL_STRING("CertDumpEKU_") + bundleref;
      rv = nssComponent->GetPIPNSSBundleString(bundlekey, local);
      if (NS_FAILED(rv)) {
        rv = GetDefaultOIDFormat(oid, local, false);
      }

Just by renaming the entries adequately, we'd get rid of around 40 lines of
code, and we'd be able to support future values without writing code.
pipnss.properties would grow, but it's compressed. A similar thing could be done
at some other places in the code, maybe with less gain.

Comment 33

[Jean-Marc Desperrier]

(In reply to comment #30)
BMPString is rarely actually used, but there are other cases in X509 where it's
a possible case in a choice, so there ought to be some more case of use.
My main point is that PSM usually interfaces with NSS at a level where it can
get already decoded utf-8 string. The reason it doesn't happen here is that we
are implementing the knowledge about how to decode extensions inside PSM instead
of inside NSS. This is a bad choice because those extensions are general usage
and other product than PSM will certainly be interested in them, *but* it was
already done this way before your patch.

It would look cleaner to me to interface at the UTF-8 level, CERT_DecodeAVAValue
could be adequate, except that it accepts several types in addition to BMP.

Else change the comment to document that PORT_UCS2_UTF8Conversion takes UCS2LE
as input, whereas the usual Mozilla type for UTF16 has a platform dependant
byte-order, so can not be used to handle that input directly and we need the
intermediate UTF-8 conversion.

Comment 34

[Martin v. Löwis]

(In reply to comment #33)
> This is a bad choice because those extensions are general usage
> and other product than PSM will certainly be interested in 
> them, *but* it was already done this way before your patch.

It seems there is disagreement wrt. this view. I originally
proposed to add more extensions to NSS, but in comment #10,
I was specifically advised not to, and do everything in PSM
unless NSS has already support for it.

> Else change the comment to document that PORT_UCS2_UTF8Conversion takes UCS2LE
> as input, whereas the usual Mozilla type for UTF16 has a platform dependant
> byte-order, so can not be used to handle that input directly and we need the
> intermediate UTF-8 conversion.

But that is not the point of the XXX comment. The comment complains
about the lack for a UCS2LE conversion function in Mozilla, so that
the only way to do the conversion (short of manually swapping bytes)
is to go via UTF-8. So if a BMPString-to-nsAString conversion function
is ever introduced, the code should be changed to use it.

Notice that CERT_DecodeAVAValue also does PORT_UCS2_UTF8Conversion,
so by calling this function, I would merely hide the fact that I'm
going back and forth to UTF-8, the "problem" would persist.

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 33 :
> [...] change the comment to document that PORT_UCS2_UTF8Conversion takes 
> UCS2LE as input, 

Actually, it takes Big Endian UCS2 as input.  Whenever UCS2 appears in a cert,
it is always big endian, regardless of the local CPU's preferred byte order.

Comment 36

[Jean-Marc Desperrier]

> Notice that CERT_DecodeAVAValue also does PORT_UCS2_UTF8Conversion,
> so by calling this function, I would merely hide the fact that I'm
> going back and forth to UTF-8, the "problem" would persist.

Maybe it's not necessary to discuss it much further, but the problem for me is
not the conversion, we're already doing it everywhere PSM interfaces with NSS. 
It's the fact the reason for the conversion is not perfectly clear if you don't
document that BMPString is UCS2BE. Everywhere else the two conventions never mix
in the same product, because only NSS calls PORT_UCS2* with UCS2BE, and
mozilla/PSM always uses it's own functions with platform dependent byte order.

> It seems there is disagreement wrt. this view. I originally
> proposed to add more extensions to NSS, but in comment #10,
> I was specifically advised not to, and do everything in PSM
> unless NSS has already support for it.

The disagreement is not as direct as it seems. I agree with Nelson there's no
reason to include this code in the NSS shared libraries. I thought about putting
it in a static library, like the CRMF code. This way it wouldn't grow NSS for
those who don't use it. But the final decision is Nelson's or Relyea/WTC's.

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 180313 [details] [diff] [review]
Patch v4

Dan, please review.  Thanks!

Comment 38

[Martin v. Löwis]

Attachment: Patch v5

This revision (v5) implements the remaining suggestions: I added myself to the
contributor's list, and I changed the rendering of extended usages to generate
bundle identifiers (of the form CertDumpEKU_1_3_6_1_5_5_7_3_1), so that new key
usages can be added without changing the code.

Comment 39

[Robert Relyea]

This would be really nice to get into aviary1.5. (though it does and some i18n
strings).

bob

Comment 40

[Mike Schroepfer]

Missed l10n window for 1.8 branch.   Also not a regression since 1.0.x so should
deploy on trunk and not on 1.8 branch.

Comment 41

[Kai Engert [:KaiE:]]

I just compared patch revisions 4 and 5.
I am fine with the changes between those versions but have the following
comments, I'm just asking you check again the code is what you are intending.


In the following code, in the earlier patch reviewed by Bob, you had a true
value in the call to GetDefaultOIDFormat, and in all the other places, you
changed such arguments to '.'. So I wonder: are you sure you want a ' ' argument
here?

+		      "{%.2x%.2x%.2x%.2x-%.2x%.2x-%.2x%.2x-%.2x%.2x-%.2x%.2x%.2x%.2x%.2x%.2x}",
+		      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(&current->name.OthName.name, value);
+	}
+    } else {
+      rv = GetDefaultOIDFormat(&current->name.OthName.oid, key, ' ');
       if (NS_FAILED(rv))
-        return rv;
+	goto finish;
+      ProcessRawBytes(&current->name.OthName.name, value);
+    }
+    break;
+  }
+  case certRFC822Name:
+    nssComponent->GetPIPNSSBundleString("CertDumpRFC822Name", key);
+    value.AssignASCII((char*)current->name.other.data, current->name.other.len);
+    break;
+  case certDNSName:




I assume that separator has a value of either space or a dot character.
In the following code, are you really intending to introduce a space? (because
in your patch reviewed by Bob Relyea, you either added a dot, or no character at
all.

@@ -186,7 +225,8 @@
     val = (val << 7) | (j & 0x7f);
     if (j & 0x80)
       continue;
-    written = PR_snprintf(&buf[len], sizeof(buf)-len, "%lu ", val);
+    written = PR_snprintf(&buf[len], sizeof(buf)-len, "%c%lu", 
+			  separator, val);


     if (written < 0)
       return NS_ERROR_FAILURE;
 

Comment 42

[Kai Engert [:KaiE:]]

Of course, I'd be glad to see Dan review that code, too, but:
- you already have r=kaie on patch v1
- you already have r=rrelyea on patch v4

After you have addressed my previous comment regarding differences between
revisions 4 and 5, you have r=kaie

If you think somebody should review the new OID details in patch v5, you could
ask Bob to diff patch v4 against v5 and confirm these additional changes are ok.

After that, all you need is a superreview.

Comment 43

[Martin v. Löwis]

(In reply to comment #41)
> In the following code, in the earlier patch reviewed by Bob, you had a true
> value in the call to GetDefaultOIDFormat, and in all the other places, you
> changed such arguments to '.'. So I wonder: are you sure you want a ' ' 
> argument here?

It's a minor display issue: the "true ASN.1 way" uses spaces; I'm uncertain
where the dotted notation originates from. It doesn't matter much either way.
In the ASN.1 syntax, the identifiers are also written in curly braces. So it
could be changed back to a dot, or the curly braces could be added, or it could
be left as-is: the user will be able to read it in any notation.

> I assume that separator has a value of either space or a dot character.
> In the following code, are you really intending to introduce a space? (because
> in your patch reviewed by Bob Relyea, you either added a dot, or no 
> character at all.

In v4, I either added the dot before, or the space after each arc of the OID.
This goes back to the original code, which always added a space after each arc.
In v5, I consistently add the separator before each arc, which works nicely
because the first two arcs are handled separately, anyway.

This entire change was necessary to support a third separator, _, which is only
used in ProcessExtKeyUsage (to denote resource names there).

Comment 44

[Nelson Bolyard (seldom reads bugmail)]

The dotted decimal notation for OIDs comes from RFCs 1485, 1779 and 2253.
But I think we should not let the choice of space or dot stand in the way 
of this patch, if that is all that remains to be resolved.

Comment 45

[Wan-Teh Chang]

Comment on attachment 192140 [details] [diff] [review]
Patch v5

Kai, Simon, could you review this very useful patch that
improves our certificate display?  Thanks.

Comment 46

[Kai Engert [:KaiE:]]

Comment on attachment 192140 [details] [diff] [review]
Patch v5

Thanks for the explanation, sounds good to me. As I said in comment 42:

r=kaie

Comment 47

[Mike Shaver (:shaver emeritus)]

Comment on attachment 192140 [details] [diff] [review]
Patch v5

sr=shaver, though it would nbe good to get bugs on file for the new XXX comments (the different string possibilities, the excessive UTF8 transcoding, etc.)

Comment 48

[Simon Fraser [no longer active]]

Comment on attachment 192140 [details] [diff] [review]
Patch v5


>Index: ssl/src/nsNSSCertHelper.cpp
>===================================================================

>+static SECOidData more_oids[] = {

Make this data const (as well as any existing static data tables) if you can.

Comment 49

[Kai Engert [:KaiE:]]

> >+static SECOidData more_oids[] = {
> Make this data const if you can.

Unfortunately we can not, because there is a dynamic registration process that will set a struct member to a value determined at runtime.

Comment 50

[Kai Engert [:KaiE:]]

Patch checked in.

Martin, thanks a lot for your contribution.

Comment 51

[Kai Engert [:KaiE:]]

Reopening, because this patch does not build on Windows.
It fails trying to compile IP v6 code.

Build Error Summary

c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(845) : error C2065: 'INET6_ADDRSTRLEN' : undeclared identifier
c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(845) : error C2057: expected constant expression
c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(845) : error C2466: cannot allocate an array of constant size 0
c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(845) : error C2133: 'buf' : unknown size
c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(848) : error C2065: 'inet_ntop' : undeclared identifier
c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(848) : error C2065: 'AF_INET' : undeclared identifier
c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(848) : error C2070: illegal sizeof operand
c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(850) : error C2065: 'AF_INET6' : undeclared identifier
c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp(850) : error C2070: illegal sizeof operand

Comment 52

[Kai Engert [:KaiE:]]

I do not have a Windows development system at the moment.

Who can help to adjust the patch to work on Windows?

Comment 53

[Ryan VanderMeulen [:RyanVM]]

I've got a Windows build system (MSVC2003). I can test out any future patches if you want.

Comment 54

[Wan-Teh Chang]

Comment on attachment 192140 [details] [diff] [review]
Patch v5

Kai,

Please try the following code:

+  case certIPAddress:
+    {
+      char buf[128];
       PRNetAddr addr;
+      nssComponent->GetPIPNSSBundleString("CertDumpIPAddress", key);
+      if (current->name.other.len == 4) {
        addr.inet.family = PR_AF_INET;
        memcpy(&addr.inet.ip, current->name.other.data, 4);
        PR_NetAddrToString(&addr, buf, sizeof(buf));
+      } else if (current->name.other.len == 16) {
        addr.ipv6.family = PR_AF_INET6;
        memcpy(&addr.ipv6.ip, current->name.other.data, 16);
        PR_NetAddrToString(&addr, buf, sizeof(buf));
+      } else
+	PL_strncpyz(buf, "Invalid IP address", sizeof(buf));
+      value.AssignASCII(buf);
+      break;
+    }

You can replace 128 by the value of INET6_ADDRSTRLEN.

Note that inet_ntop doesn't exist on Windows.

The "Invalid IP address" is an internationalization bug.
You may want to replace it by something like "***" or
"???" (if the latter, watch out for "trigraph" problems).

Comment 55

[Martin v. Löwis]

As for the "Invalid IP Address" case: It shouldn't happen
in a "correct" certificate; if it does happen, we probably
can just display the raw bytes:

   case certIPAddress:
     {
       char buf[128];
       PRNetAddr addr;
       nssComponent->GetPIPNSSBundleString("CertDumpIPAddress", key);
       if ((current->name.other.len == 4) || 
           (current->name.other.len == 16))
        addr.inet.family = (current->name.other.len == 4) ? 
                            PR_AF_INET : PR_AF_INET6;
        memcpy(&addr.inet.ip, current->name.other.data, 
               current->name.other.len);
        PR_NetAddrToString(&addr, buf, sizeof(buf));
        value.AssignASCII(buf);
       } else
         /* invalid IP address */
         ProcessRawBytes(&current->name.other, value);
       break;
     }

Comment 56

[Wan-Teh Chang]

Martin: good idea.  Displaying the raw bytes is more
useful than a (localized) error message in this case.

Kai, if you use Martin's code snippet, please change
addr.inet.family to addr.raw.family, and use
&addr.ipv6.ip for the IPv6 case because inet.ip and
ipv6.ip have different offsets in the PRNetAddr union.

Comment 57

[Kai Engert [:KaiE:]]

This is the combination of Wan-Teh's code with Martin's proposal for the "invalid IP" case, with my added definition of the fallback INET6_ADDSTRLEN definition.

Does that look good?


+#ifndef INET6_ADDRSTRLEN
+#define INET6_ADDRSTRLEN 46
+#endif

...

+  case certIPAddress:
+    {
+      char buf[INET6_ADDRSTRLEN];
+      PRNetAddr addr;
+      nssComponent->GetPIPNSSBundleString("CertDumpIPAddress", key);
+      if (current->name.other.len == 4) {
+        addr.inet.family = PR_AF_INET;
+        memcpy(&addr.inet.ip, current->name.other.data, 4);
+        PR_NetAddrToString(&addr, buf, sizeof(buf));
+      } else if (current->name.other.len == 16) {
+        addr.ipv6.family = PR_AF_INET6;
+        memcpy(&addr.ipv6.ip, current->name.other.data, 16);
+        PR_NetAddrToString(&addr, buf, sizeof(buf));
+        value.AssignASCII(buf);
+      } else {
+        /* invalid IP address */
+        ProcessRawBytes(&current->name.other, value);
+      }
+      break;
+    }

Comment 58

[Wan-Teh Chang]

Kai, value.AssignASCII(buf) also needs to be called
in the IPv4 (PR_AF_INET) case.

Comment 59

[Kai Engert [:KaiE:]]

Attachment: Patch v5 b

Here is an updated version of patch v5, it has the following differences:

added:

+#ifndef INET6_ADDRSTRLEN
+#define INET6_ADDRSTRLEN 46
+#endif

certIPAddress section is now:

+  case certIPAddress:
+    {
+      char buf[INET6_ADDRSTRLEN];
+      PRNetAddr addr;
+      nssComponent->GetPIPNSSBundleString("CertDumpIPAddress", key);
+      if (current->name.other.len == 4) {
+        addr.inet.family = PR_AF_INET;
+        memcpy(&addr.inet.ip, current->name.other.data, 4);
+        PR_NetAddrToString(&addr, buf, sizeof(buf));
+      } else if (current->name.other.len == 16) {
+        addr.ipv6.family = PR_AF_INET6;
+        memcpy(&addr.ipv6.ip, current->name.other.data, 16);
+        PR_NetAddrToString(&addr, buf, sizeof(buf));
+        value.AssignASCII(buf);
+      } else {
+        /* invalid IP address */
+        ProcessRawBytes(&current->name.other, value);
+      }
+      break;
+    }

Comment 60

[Kai Engert [:KaiE:]]

Ryan, thanks a lot for trying to compile this patch on Windows!
Unfortunately, the build still failed.

Patch v5 b has been created 10 days ago.
It took me until today to look at the reported error in detail:

nsBrowserApp.obj : warning LNK4217: locally defined symbol _XRE_main imported in function _main
pipnss.lib(nsNSSCertHelper.obj) : error LNK2019: unresolved external symbol _SEC_BMPStringTemplate referenced in function "unsigned int __cdecl ProcessBMPString(struct SECItemStr *,class nsAString_internal &,class nsINSSComponent *)" (?ProcessBMPString@@YAIPAUSECItemStr@@AAVnsAString_internal@@PAVnsINSSComponent@@@Z)
pipnss.lib(nsNSSCertHelper.obj) : error LNK2019: unresolved external symbol _SEC_IA5StringTemplate referenced in function "unsigned int __cdecl ProcessIA5String(struct SECItemStr *,class nsAString_internal &,class nsINSSComponent *)" (?ProcessIA5String@@YAIPAUSECItemStr@@AAVnsAString_internal@@PAVnsINSSComponent@@@Z)
pipnss.lib(nsNSSCertHelper.obj) : error LNK2019: unresolved external symbol _SEC_OctetStringTemplate referenced in function "unsigned int __cdecl ProcessSubjectKeyId(struct SECItemStr *,class nsAString_internal &,class nsINSSComponent *)" (?ProcessSubjectKeyId@@YAIPAUSECItemStr@@AAVnsAString_internal@@PAVnsINSSComponent@@@Z)
firefox.exe : fatal error LNK1120: 3 unresolved externals

It seems these data symbols are not exported on Windows, the nss.def file says:

;+# Data objects
;+# Don't export these DATA symbols on Windows because they don't work right.
;;SEC_BMPStringTemplate DATA ;
;;SEC_IA5StringTemplate DATA ;
;;SEC_OctetStringTemplate DATA ;

Comment 61

[Nelson Bolyard (seldom reads bugmail)]

When exporting symbols from a shared library for use in another shared lib
or in a program, Windows works the same as unix for function names, but not
for data.  On unix, the exported symbol has the value of the address of the 
named item, but in Windows, the exported symbol has the value of the address
of a pointer to the named item.

NSS has devised a set of macros to solve this issue for ASN1 templates, so 
that a single piece of source code can be written that works on both Unix 
and Windows.  When attempting to access an ASN1 template as a data symbol 
from a (another) shared lib, code should use the NSS macros SEC_ASN1_MKSUB, 
SEC_ASN1_SUB, SEC_ASN1_GET, and in templates should also use SEC_ASN1_XTRN.  
That code will then work properly on both Windows and Unix.  The macros 
expand on Unix to the usual symbols, and on Windows they expand to pointer 
to functions in the remote shared lib that returns the desired address.

There are MANY examples of the use of these macros in 
http://lxr.mozilla.org/security/source/security/nss/lib/smime/cmsasn1.c
Search that file for SEC_PointerToOctetStringTemplate to see the proper
use of those macros.  You will also find use of some of those macros for
the 3 templates named in the preceeding comment.

Essentially,, the rules are:

a) in a template, instead of referring to the remote template directly,
use the macro SEC_ASN1_SUB to reference it, and then add SEC_ASN1_XTRN to
the "kind" member of the template.  See an example of this at 
http://lxr.mozilla.org/security/source/security/nss/lib/smime/cmsasn1.c#116
Then for each symbol used with SEC_ASN1_SUB, you need a single declaration 
of an accessor function, using the macro SEC_ASN1_MKSUB, as shown at 
http://lxr.mozilla.org/security/source/security/nss/lib/smime/cmsasn1.c#60

b) in a function, instead of referring directly to the remote template, code
should use the macro SEC_ASN1_GET, as shown at 
http://lxr.mozilla.org/security/source/security/nss/lib/smime/cmsasn1.c#562

The documentation for those macros is in this page:
http://www.mozilla.org/projects/security/pki/nss/release_notes_32.html#What's

Comment 62

[Wan-Teh Chang]

Comment on attachment 205942 [details] [diff] [review]
Patch v5 b

>+  case certIPAddress:
>+    {
>+      char buf[INET6_ADDRSTRLEN];
>+      PRNetAddr addr;
>+      nssComponent->GetPIPNSSBundleString("CertDumpIPAddress", key);
>+      if (current->name.other.len == 4) {
>+        addr.inet.family = PR_AF_INET;
>+        memcpy(&addr.inet.ip, current->name.other.data, 4);
>+        PR_NetAddrToString(&addr, buf, sizeof(buf));
>+      } else if (current->name.other.len == 16) {
>+        addr.ipv6.family = PR_AF_INET6;
>+        memcpy(&addr.ipv6.ip, current->name.other.data, 16);
>+        PR_NetAddrToString(&addr, buf, sizeof(buf));
>+        value.AssignASCII(buf);
>+      } else {
>+        /* invalid IP address */
>+        ProcessRawBytes(&current->name.other, value);
>+      }
>+      break;
>+    }

You also need the "value.AssignASCII(buf);" statement in
the PR_AF_INET case.

Comment 63

[Kai Engert [:KaiE:]]

Attachment: Patch v6

Nelson, thanks a lot for your explanation. This new patch now uses macro SEC_ASN1_GET.


> You also need the "value.AssignASCII(buf);" statement in
> the PR_AF_INET case.

Thanks Wan-Teh, sorry that you had to say that twice.

Comment 64

[Wan-Teh Chang]

Comment on attachment 206407 [details] [diff] [review]
Patch v6

r=wtc.  As a matter of style, please use
current->name.other.len instead of 4 and 16
in the following two lines:

>+        memcpy(&addr.inet.ip, current->name.other.data, 4);

>+        memcpy(&addr.ipv6.ip, current->name.other.data, 16);

Comment 65

[Kai Engert [:KaiE:]]

Attachment: Patch v7

This patch has Wan-Teh's requested change.
carrying forward r=wtc

Mike, could you please review again?

Here are the differences between the patch you had already reviewed and the current one:

- added:
+#ifndef INET6_ADDRSTRLEN
+#define INET6_ADDRSTRLEN 46
+#endif

- wrapped three variables with an accessor function call: SEC_ASN1_GET

- changed one code section to this:
+  case certIPAddress:
+    {
+      char buf[INET6_ADDRSTRLEN];
+      PRNetAddr addr;
+      nssComponent->GetPIPNSSBundleString("CertDumpIPAddress", key);
+      if (current->name.other.len == 4) {
+        addr.inet.family = PR_AF_INET;
+        memcpy(&addr.inet.ip, current->name.other.data, current->name.other.len);
+        PR_NetAddrToString(&addr, buf, sizeof(buf));
+        value.AssignASCII(buf);
+      } else if (current->name.other.len == 16) {
+        addr.ipv6.family = PR_AF_INET6;
+        memcpy(&addr.ipv6.ip, current->name.other.data, current->name.other.len);
+        PR_NetAddrToString(&addr, buf, sizeof(buf));
+        value.AssignASCII(buf);
+      } else {
+        /* invalid IP address */
+        ProcessRawBytes(&current->name.other, value);
+      }
+      break;
+    }

Comment 66

[Kai Engert [:KaiE:]]

Comment on attachment 206410 [details] [diff] [review]
Patch v7

Bob, can you please review the new snippets in this patch (see previous comment).

Comment 67

[Robert Relyea]

Comment on attachment 206410 [details] [diff] [review]
Patch v7

r+ on the diff from v6 to v7

Comment 68

[Kai Engert [:KaiE:]]

checked in to trunk, second attempt

Comment 69

[Kaspar]

Attachment: Proposed patch to the patch

A very useful patch - I've been badly missing this feature in Firefox/Thunderbird for quite some time, so I really appreciate it has now been committed to trunk.

Can I propose a patch to the patch (attached to this comment)? It includes the following fixes/improvements:

1) basic constraints extension: if the CA flag is false (i.e. in an end-entity certificate), then there can't be a pathLenConstraint value. However, ProcessBasicConstraints() currently doesn't take this into account and will display bogus data for the pathLenConstraint (value.pathLenConstraint never gets initialized properly - look at the certificate from https://www.verisign.com to see what I mean - it'll say something like "Maximum number of intermediate CAs: 36514320"). Additionally, I've added support for the case where the path length is unlimited.

2) authority information access: support for the OCSP responder OID added; fixed bug that information about the criticality gets overwritten inadvertently; removes 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) adds support for a bunch of (Microsoft) Extended Key Usage OIDs (mostly from http://support.microsoft.com/default.aspx?scid=kb;en-us;287547)

5) adds basic recognition of the PKIX Logotype extension (RFC 3709) - no, it won't parse the contents of this rather quirky extension... but at least you know what it refers to when you encounter it in certificates from Verisign (take https://www.amazon.com as an example)

6) minor wording changes ("Server Gated Crypto" instead of "Strong Crypto Approved" etc.)

Thanks for considering these suggested changes (I hope my code is not too ugly/buggy).

Comment 70

[Martin v. Löwis]

I'm not entirely sure what the procedures are, but, as the original submitter of the patch, I would hope that the status of that patch stays at "FIXED" - it took long enough to get into that state.

So I would suggest the right procedure is to submit this patch as a separate, new item (I'm sure Mozilla folks will correct me if I'm wrong here).

Conceptually, I see three kinds of changes in the patch, each of which should be discussed separately (and which might get accepted/rejected separately):
- bug fixes/corrections
- additions
- message changes

With additions (like the logo extensions), I have no issues: I expected more and more additions to be contributed over time.

With the message changes, I think great care is necessary; gratuitous renamings should be avoided. Keep in mind that these are i18n changes: every message you change will have to be translated, so there should be a really good reason to change it. One reason would be that you found the "official" name of some extension somewhere; clearly, things should be named by their official name. As for the Microsoft extensions, I think I took most of the names from some MS KB article, so if you found something more official elsewhere, I would like to know. For example, why is "Microsoft Time Stamping" better than "Microsoft Time Stamp Signing"?

As for the BasicConstraints fix: this should be done (regardless of the rest of the patch). I think it still isn't completely right, though, partly due to a problem in NSS: It is *conforming* to have cA set to FALSE, yet pathLenConstraint present. 4.2.1.10 just says that pathLenConstraint won't be meaningful in that case, but does not forbid that (through a MUST NOT specification). Unfortunately, NSS doesn't provide a CERT_PATH_CONSTRAINT_ABSENT value, instead, it will report an error in such a case.

Comment 71

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 208551 [details] [diff] [review]
Proposed patch to the patch

This is not a full review, but just some comments.
1. I like all the changes you've proposed to make here.
2. I wouldn't worry too much about the L10n implications of the
   changes to existing strings.  It's OK if there's a minor 
   change to the English string and no corresponding changes
   to the other languages immediately.
3. Adding new strings that must be localized *is* an issue, and
   may affect whether this patch can go into this next release
   or must wait until a later one.  If the L10n efforts on the 
   prior patch have not yet begun, then I'd say we want to get
   these latest changes in ASAP.

As for whether a new bug is needed, I think that mostly depends
on whether the new patch is going to get into the same release
as the previous one.  If so, then I think one bug can serve
both, but if not then the new patch needs a new bug.  It wouldn't
hurt for the new patch to get a new bug of its own anyway.

Kai, please advise us about this.  Are you willing to champion 
this patch into PSM rather immediately?  Is it too late for this 
release?

Also, In response to Martin's observation about NSS's parsing of
BasicConstraints extensions, I have filed bug 323557 and attached
a patch thereto.  Martin, I invite your review comments on that
patch, and please attach a sample cert to it.  Thanks.

Comment 72

[Nelson Bolyard (seldom reads bugmail)]

One other thought about all the new OIDs now recognized by cert manager.
Many of these new OIDs are not actually used by mozilla/FF/TB/SM/NSS/PSM.
Having the OID names decoded in cert manager may set the user's expectation
that the extensions (or extended usages) will be recognized/honored, when
they actually will not.  

So, I might suggest adding some indicator to the OID name strings for 
OIDs not actually used by mozilla, indicating that fact.  Perhaps an 
asterisk "*" at the end of each such string.  

Comment 73

[Kaspar]

> As for the Microsoft extensions, I think I took most of the names from some
> MS KB article, so if you found something more official elsewhere, I would
> like to know. For example, why is "Microsoft Time Stamping" better than
> "Microsoft Time Stamp Signing"?

I wouldn't say it's "better", but where available I took the wording from an English version of the operating system itself (i.e., the Windows GUI will display "Microsoft Time Stamping"). It's possible that I didn't catch all of them, but I can look into it again if that helps (e.g., 1.3.6.1.4.1.311.10.3.4.1 would just be "File Recovery" then, not "Encrypting Filesystem Recovery Agent").

> As for the BasicConstraints fix: this should be done (regardless of the rest
> of the patch). I think it still isn't completely right, though, partly due to
> a problem in NSS: It is *conforming* to have cA set to FALSE, yet
> pathLenConstraint present. 4.2.1.10 just says that pathLenConstraint won't be
> meaningful in that case, but does not forbid that (through a MUST NOT
> specification). Unfortunately, NSS doesn't provide a
> CERT_PATH_CONSTRAINT_ABSENT value, instead, it will report an error in such a
> case.

Correct, that's why I added this to a later version of the patch, in order to fail more gracefully when CERT_DecodeBasicConstraintValue fails (the "Details" tab will remain blank otherwise):

@@ -558,8 +569,8 @@

   rv = CERT_DecodeBasicConstraintValue (&value, extData);
   if (rv != SECSuccess) {
-    NS_ASSERTION(0,"Could not decode basic constraints");
-    return NS_ERROR_FAILURE;
+    ProcessRawBytes(extData, text);
+    return NS_OK;
   }
   if (value.isCA)
     rv2 = nssComponent->GetPIPNSSBundleString("CertDumpIsCA", local);


Depending on how bug 323557 is fixed, this might no longer be necessary, though.

Comment 74

[Martin v. Löwis]

(In reply to comment #72)
Is it really likely that users will have that expectation? For most of the extensions, there isn't any meaningful action the browser could take (e.g. Netscape certificate comment). Should it display the extension as supported or unsupported?

Also, for some extensions (in particular "extended key usage") it might be that the browser supports some values, but not others. Again, for each key usage, it might be that there is no meaningful action the browser could take.

For other extensions, whether or not it is used is a configuration setting, e.g. OCSP. Or it could depend on the specific tool (browser vs. mail reader) whether the extension is used.

So I would expect such an asterisk is more confusing to the user than helpful.

Comment 75

[Martin v. Löwis]

(In reply to comment #73)
> I wouldn't say it's "better", but where available I took the wording from an
> English version of the operating system itself (i.e., the Windows GUI will
> display "Microsoft Time Stamping").

Ah, that would be better indeed (hoping that Microsoft doesn't reword it that often between operating system releases). I can accept this, atleast for the Microsoft OIDs. I would appreciate if some place in the code pointed out where the strings come from (noting that I failed to do so myself, so collecting this information incrementally would be fine).

Comment 76

[Kai Engert [:KaiE:]]

I appreciate your work on this.

Usually, once some issue is done, and did not introduce regressions, I prefer to keep a bug fixed, and stop working on the bug. This simplifies tracking. Usually all ongoing work should be done in a new, separate bug.

My opinion is: If you have reached an agreement already, that the above additional patch is perfect to be checked in already, I'm fine to review it and land it shortly.

However, if we expect additional revisions of the patch and some more discussion, I'd prefer to have a new, separate bug.

Note that having a new, separate bug does not mean a slowdown. As this is a backend only change, we will be able to check this in quickly, after you have reached agreements. Note we no longer need a superreview for backend only changes to PSM, that do not touch UI, but a module owner/peer review is sufficient.

Comment 77

[Wan-Teh Chang]

Comment on attachment 206410 [details] [diff] [review]
Patch v7

Requesting approval1.8.1 for Firefox and Thunderbird 2.0.

This patch greatly improves the display of certificate
extensions.  It has been on the Mozilla trunk since 2005-12-25.

Comment 78

[Kaspar]

(In reply to comment #76)
> However, if we expect additional revisions of the patch and some more
> discussion, I'd prefer to have a new, separate bug.

Ok, I think this is what's going to happen, so I've filed bug 323903 and
hope that we can soon reach an agreement (Martin, please have a look at it).

On a side note (with regard to comment 71): there's one big misnomer(TM) in the whole Mozilla tree... CA stands for "Certification Authority", not "Certificate Authority" - but no, I didn't want to change that ;-) (PEM [RFC 1421, February 1993] already had it like this, maybe it was a glitch in the early Netscape days...?)

I'm marking this version of the patch as obsolete.

Comment 79

[Kaspar]

Comment on attachment 208551 [details] [diff] [review]
Proposed patch to the patch

for v2 of this patch, see bug 323903

Comment 80

[Kaspar]

(In reply to comment #2)
> I believe this patch fixes bug 230655, bug 127052, bug 161140.

I agree - but the first two are still open as of today. Would it be correct to resolve these as duplicates of this one here, or what's the proper procedure?