234856 - Cert viewer titlebar and hierarchy panel displaying utf8 as iso-8859-1

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[John G. Myers]

View a certificate with non-ascii characters in the subject name.  The titlebar
of the certificate viewer will incorrectly display the non-ascii characters.

An example of a problemenatic cert is
http://bugzilla.mozilla.org/attachment.cgi?id=109214&action=view

Comment 1

[John G. Myers]

I could use some help on this bug.  For one, the cert viewer titlebar isn't
displayed on MacOS and I don't have any other platform I can set up a build
environment on.  For another thing, this appears to be js/DOM related and I find
those exceedingly difficult to debug.

In security/manager/pki/resources/content/viewCertDetails.js, the function
setWindowName() gets the name from one of two places.  I suspect it is getting
it from the second-listed place, nsNSSCertificate::GetWindowTitle(char *
*aWindowTitle).  That routine is returning the string in UTF-8 and
setWindowName() then uses it to set the "title" attribute on the window.

Why the "title" attribute is being interpreted as ISO-8859-1 and how I can get
it to interpret it as UTF-8, I have no idea.

Comment 2

[John G. Myers]

Ok, the problem is that nsIX590Cert.idl defines windowTitle as "string" and
xpconnect treats "string" objects as being in ISO-8859-1.

Comment 3

[Jean-Marc Desperrier]

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

Comment 4

[Simon Montagu :smontagu]

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

Comment 5

[Simon Montagu :smontagu]

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

Comment 6

[Simon Fraser [no longer active]]

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

Comment 7

[Simon Fraser [no longer active]]

Comment #2 identifies the problem. The best fix for this is to change the idl to
make windowTitle a AUTF8String attribute, but nsIX509Cert is frozen. What to do?

Comment 8

[Simon Montagu :smontagu]

Simon, see also the blocker bug 235230.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: cert example

This is the cert from bug 185167, here in a form that should be easily 
downloaded into the browser for examination.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Well, there's another new (?) bug.  That cert cannot be imported at all,
not by downloading with the appropriate MIME content type, and not by 
downloading to a disk file and then locally importing it from that file.
Sigh.

Comment 12

[Kaspar]

(In reply to comment #11)
> and not by downloading to a disk file and then locally importing it from
> that file. Sigh.

Nelson, on what tab (in Cert Manager) did you attempt to load the cert? Try "Web Sites", that should do the trick (the "Other People's" tab makes additional checks, which prevent it from being imported in this case).

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Kaspar, those other checks  in the browser were added to prevent AUTOMATIC 
importing of invalid certs into cert DBs.  They should not prevent users 
from knowingly and consciously adding other certs from local files.  
That they do so is a bug.

I added the cert with certutil.

Comment 14

[Kaspar]

(In reply to comment #13)
> They should not prevent users 
> from knowingly and consciously adding other certs from local files.  
> That they do so is a bug.

It's probably considered a "feature" of Cert Manager that it doesn't allow to import untrusted/expired certificates on the "Other People's" tab - nsNSSCertificateDB::ImportEmailCertificate explicitly calls CERT_VerifyCert before it adds them to the database.

In any case, my suggestion in comment 12 was simply meant as a quick way to get the cert into the database... because in the context of this bug, it doesn't matter whether the cert appears under the "Other People's" or the "Web Sites" tab.

Comment 16

[Zack Weinberg (:zwol)]

This appears to be fixed by the patch I'm about to post for bug 643041.  Suggestions for how to test it automatically would be appreciated.

Comment 17

[(no longer active)]

(In reply to comment #16)
> This appears to be fixed by the patch I'm about to post for bug 643041. 
> Suggestions for how to test it automatically would be appreciated.

Hmm, try to get the length of those strings with some real UTF-8 data inside them.  Here's a word in UTF-8 that you can just paste in your test:

آزمایش

the length should be 6 if the string is interpreted as UTF-8, and 12 otherwise.

Comment 18

[Zack Weinberg (:zwol)]

I need a little more help than that.  The word has to be packaged in a certificate that can somehow be imported, converted to an nsIX509Cert handle, and then its .windowTitle attribute retrieved.  I'm certain all this can be done from a chrome mochitest (possibly with the certificate embedded as a data blob) but I have no idea where to begin.

Comment 19

[(no longer active)]

(In reply to comment #18)
> I need a little more help than that.  The word has to be packaged in a
> certificate that can somehow be imported, converted to an nsIX509Cert handle,
> and then its .windowTitle attribute retrieved.  I'm certain all this can be
> done from a chrome mochitest (possibly with the certificate embedded as a data
> blob) but I have no idea where to begin.

I don't know how you would wrap this word up in a certificate, but once you get that part working, you can probably open this window from your test <http://mxr.mozilla.org/mozilla-central/source/security/manager/pki/resources/content/viewCertDetails.xul> and just read the title.  Take a look at the files inside that directory, that's where the certificate manager UI is implemented.

Comment 20

[John G. Myers]

There is a test certificate attached to this bug.

Comment 21

[Zack Weinberg (:zwol)]

Attachment: patch adding automated test

This test has been manually confirmed to fail when run against current trunk, and to pass when run against current trunk + the patches in bug 643041.  If approved, I would land it together with those patches (which have already passed review).

Comment 22

[Zack Weinberg (:zwol)]

Attachment: better automated test

It occurred to me as I was going to sleep last night that I could embed the base64-encoded certificate in the test driver and avoid all that mucking about with XMLHttpRequest.  Also moved the test to an existing test directory.

Comment 23

[Kai Engert (:KaiE:)]

Comment on attachment 521194 [details] [diff] [review]
better automated test

r=kaie thanks

Comment 24

[(no longer active)]

FWIW, I also read the test, and it looks great to me.

Comment 29

[Zack Weinberg (:zwol)]

Attachment: updated better automated test

Since others have fixed bug 235230 and bug 643041, I've dusted off the test case that I posted to this bug, three years ago (!), and committed it.  Here is the committed version - changes relative to the previous are: remove mention of nsIX509Cert3; chrome.ini instead of Makefile.in; sha256Fingerprint instead of md5Fingerprint; subjectName and issuerName have seen unrelated accuracy improvements; all subtests now expected to pass.

For maximum clarity, the *bug* originally reported here was fixed in bug 235230; however, this test case depends on bug 643041 as well.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e8b6862acb2

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/0e8b6862acb2