453227 - installation of PEM-encoded certificate without trailing newline fails

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Igor Velkov]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3pre) Gecko/2008090106 GranParadiso/3.0.3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3pre) Gecko/2008090106 GranParadiso/3.0.3pre

If I create client certificate from https://www.startssl.com/?app=12 it not apperas in personal certificate manager, and seems lost.

Result is added account that can be accessed only with client certificate, without certificate.

Reproducible: Always

Steps to Reproduce:
1. Go to https://www.startssl.com/?app=12 and register for an account
2. check high grade
3. got security code via mail, enter it into wizard
4. wait while key generation process
5. confirm certificate install button
6. got "Congratulations!
     * Your first client certificate is installed in your browser. This is a bootstrapping certificate for authentication purpose.
    * Backup this certificate to an external media"
screen

Go to tools -> options -> advanced -> encryption -> view certificates ->your certificates
Actual Results:  
you not see certificate for entered into form email signed by StartCom. If you don't have any certs before start this test - you will see empty cert list.

Expected Results:  
cert signed by StartCom 

works ok with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

Similar bug actual for current seamonkey-trunk Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080901002016 SeaMonkey/2.0a1pre ID:20080901002016

Comment 1

[Eddy Nigg (StartCom)]

It's most likely the wrong component. Kai, can you move that to the right place? Perhaps it's either NSS or PSM.

Comment 2

[Eddy Nigg (StartCom)]

Confirmed using 3.0.3pre from tonight (on Linux). That's a critical bug, since client certificates can't be installed anymore!

Comment 3

[Eddy Nigg (StartCom)]

Adding to CC according to latest checkings which might have affected this regression.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Eddy, does this affect all certs for all users, or only a few certs?
If only a few, please attach the PEM files for one or more affected certs
to this bug.

I'm resetting the priority, since it was P5, which is the lowest, and 
I'm sure that's not right.

Comment 5

[Eddy Nigg (StartCom)]

Thanks, I meant highest priority (but it's not documented).

It fails with all client certificates which are a response of a private key generated in the browser. The private key generation functions correctly and the certificate is issued correctly too. It happens with the current nightly every time. The bug was detected by Igor Velkov and reported by him.

Same functions and responses work with current release and backwards - I think back until Netscape 4.72 and perhaps older than that...

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

The convention is that the reporter sets the "severity" 
(critical, major, normal, trivial, enhancement) and the 
assignee sets the priority.   But assignees are generally
willing to leave the priority alone when it is set low. :)
So, I'll make the priority unset, and the assignee can set it.

Kai, it would be useful if you could determine where the failure
occurs.  If the failure is in an NSS function called by PSM, it 
would be useful to know which one.

Comment 7

[Eddy Nigg (StartCom)]

Understood!

Kai, for testing purpose feel free to use startssl.com (just don't use any important email addresses which you really might want to use in the future).

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Eddy, 
Please attach to this bug one PEM file for one cert with which the problem
is seen.
Also, please tell us here (in a comment) what MIME content-type is used
for the failing cert downloads.

Comment 9

[Eddy Nigg (StartCom)]

Attachment: Example PEM encoded S/MIME certificate

This was the certificate I received during my test run with the nightly. All certificates are affected however.

Comment 10

[Eddy Nigg (StartCom)]

The content type is application/x-x509-user-cert. The certificate is provided as pkcs7 including chained CA certificates.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

NSS has no problem parsing or displaying the cert previously attached.

Please attach a copy of the PKCS#7 file that is downloaded to this bug.

Comment 12

[Eddy Nigg (StartCom)]

Attachment: Example PKCS7 encoded S/MIME certificate with CA chain

This is the certificate as it would have been sent and which installs in current Firefox and earlier. Note the -----BEGIN CERTIFICATE----- header and footer which isn't -----BEGIN PKCS7----- for Firefox.

Comment 13

[Kai Engert (:KaiE:)]

I signed up, but I used fake data, now it says it's waiting for manual review...

Comment 14

[Eddy Nigg (StartCom)]

Which shows our system is working as expected ;-)

Please sign up using your real details and by adhering to our CA policy. Thanks for your understanding!

Comment 15

[Kai Engert (:KaiE:)]

Using the latest nightly Firefox 3.0.3pre builds and a fresh profile I was successfully able to enroll for and pick up a certificate using the service at https://digitalid.verisign.com/client/class1Netscape.htm
using fake data.

Comment 16

[Kai Engert (:KaiE:)]

Are you able to identify the latest version that still worked?

Does Error Console give any information when you experience the failure?

Comment 17

[Kai Engert (:KaiE:)]

(In reply to comment #14)
> Please sign up using your real details and by adhering to our CA policy. Thanks
> for your understanding!

I'd prefer if you could set up some test account.
Thanks.

Comment 18

[Eddy Nigg (StartCom)]

Which doesn't help really...do you know in which format the certificate is delivered? There error console doesn't reveal anything, I've checked that first actually. If it's of any help, I can simply crawl back through the nightly's until I find the last one which works...well, perhaps I should start that now.

Comment 19

[Kai Engert (:KaiE:)]

Thanks for your offer to help testing nightlies.

The last NSS version upgrade happened on August 14. Maybe it would be good to start by testing builds from August 13 and August 15.


Maybe you could use the livehttpheaders extension and snoop the data exchange between mozilla and your server.

It would be helpful to see what content and which content types get exchanged.

Comment 20

[Eddy Nigg (StartCom)]

I use livehttpheaders, however in our case most likely not helpful since at the StartSSL CA application isn't easy to intercept such data. I'm doing testing right now and will report back soonish...

Comment 21

[Eddy Nigg (StartCom)]

The last working version is Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.2pre) Gecko/2008081804 GranParadiso/3.0.2pre, the version from the 19th fails. Hope this helps to find it. Thanks!

Comment 22

[Kai Engert (:KaiE:)]

After a bit of teasing in both directions a test account has been set up.

I tried to reproduce the bug and I can confirm the report from comment 0:
- web page gives a success message
- cert viewer shows no personal cert

Now that web page told me it was some kind of one kind thing, and I can't repeat the request again???

I tried to go to "retrieve cert", and this time it told me I don't have the private key.

I decided to quit Firefox and start again.

I went to www.startssl.com and now I got a SSL handshake error:
ssl_error_handshake_failure_alert

Comment 23

[Kai Engert (:KaiE:)]

I can always reproduce the handshake error using this procedure:

- create a fresh profile (or erase key3.db cert8.db secmod.db)
- start firefox
- go to www.startssl.com
- click the big LOGIN button

error

Comment 24

[Eddy Nigg (StartCom)]

Kai, that's not related, it's because you don't have the client certificate installed (obviously), see also https://www.startssl.com/?app=25#10

This is NOT the bug. The bug is, that the client certificate isn't installed, please choose a nightly version that works (e.g. pre 18th of August) and register again with a different email address. See that it works. Thereafter you can always login using the installed client cert and perform other tests with the same profile but new browser versions.

Comment 25

[Kai Engert (:KaiE:)]

I was able to find a path through the UI that allowed me to re-download, and we were able to identify the place where it fails.

It fails in CERT_DecodeCertPackage.
This was changed with bug 414003.
I reverted that change and it fixed the download bug.

Comment 26

[Kai Engert (:KaiE:)]

Attachment: revert test patch

After I applied this change on top of Firefox / NSS, the test case works.

Comment 27

[Kai Engert (:KaiE:)]

Attachment: contents of certbuf/certlen

Nelson asked me to investigate the exact data that is being passed to function DecodeCertPackage.

I added this code snippet to the beginning of the function:

FILE *fp;
fp = fopen("/tmp/certbuf.txt", "wb");
fwrite(certbuf, certlen, 1, fp);
fclose(fp);

It produced the attached file.

Comment 28

[Kai Engert (:KaiE:)]

file size of certbuf.txt is 8069 bytes.

No line ending after the final line of data.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Attachment: proposed patch (long term)

While reviewing this code (again), I found a BUNCH of problems, most of 
which were there from the beginning.  I think this patch fixes them all
and fixes the one problem that is the cause of this bug report.

I will also attach a much shorter patch that might be more acceptable as a 
short term emergency fix.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Attachment: proposed patch (short term emergency fix)

This patch makes the code more closely match its behavior in FF 3.0

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Additional comments:

The crucial discovery is the lack of a \n at the end of the last line of the 
cert download, the line that contains -----END CERTIFICATE-----  

I was unable to reproduce the failure with the second attachment above
(attachment 336914 [details]) 
because that attachment has a final \n at the end of the last line.  
I was able to reproduce the failure with Kai's attachment 336959 [details] 
which lacked the trailing \n.  

Clearly we need to add a file like Kai's attachment to our tests.

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Oh, the workaround for this is trivial.  Simply add one more character onto
the end of that download, after the -----END CERTIFICATE-----
That character can be a \n or a blank or any other non-NULL character.
It's probably a good idea to have a trailing \n in any case.

Comment 33

[Kai Engert (:KaiE:)]

Comment on attachment 336966 [details] [diff] [review]
proposed patch (short term emergency fix)

r=kaie

I understand why this patch is correct.

I have tested that this patch fixes the bug.

Comment 34

[Kai Engert (:KaiE:)]

Comment on attachment 336964 [details] [diff] [review]
proposed patch (long term)

I won't review this more complicated patch today. My head is in flames already. I advice against changing the code more until we have some kind of testing for this code.

I'd vote to take only the emergency fix for now.

Comment 35

[Wan-Teh Chang]

Comment on attachment 336966 [details] [diff] [review]
proposed patch (short term emergency fix)

r=wtc.

>+	    cl -= NS_CERT_HEADER_LEN + 1;
>+	    cp += NS_CERT_HEADER_LEN + 1;

Would be nice to add a comment to explain what the + 1
is for.  I think it skips over an expected eol after
the cert header.

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

I think this means that we go back to the tag that FF3 is using now 
(whatever it is) and make a "mini branch" off of that tag, and apply 
the patch Kai reviewed above to it.

Christophe would ordinarily do that (create the mini branch) but I think 
he's on vacation this week.  If you will tell me the tag presently being
used by FF on the 1.9.0 branch, I will make the mini branch from there.

Comment 37

[Kai Engert (:KaiE:)]

I've produced a test case that works independently of CA websites.
All it requires is copying key3.db and cert8.db to a profile directory and visiting a URL.

With a buggy firefox, nothing happens, and cert manager will show no personal cert.

With an old working firefox, or with a new fixed firefox, one will be prompted to enter a master password and the import will succeed.

Please contact me if you'd like to obtain the test files.
I'll add them as a private data to this bug, but I'd like to keep them private, as they involve an official cert to a test email address owned by me.

Comment 38

[Kai Engert (:KaiE:)]

The tag currently being used by the Firefox 3.0.x branch is NSS_3_12_1_RC1.
This tag would work as the base tag for a 3.12.1 mini-branch.

Comment 39

[Wan-Teh Chang]

Comment on attachment 336966 [details] [diff] [review]
proposed patch (short term emergency fix)

>-	while ( cl > NS_CERT_TRAILER_LEN ) {
>+	while ( cl >= NS_CERT_TRAILER_LEN ) {

I think this change alone is enough to fix this bug.
The other + 1 change isn't necessary, because certbegin
doesn't need to point to the exact beginning of the cert,
and certend doesn't need to point at the byte after the
exact end of the cert.

Comment 40

[Kai Engert (:KaiE:)]

(In reply to comment #39)
> (From update of attachment 336966 [details] [diff] [review])
> >-	while ( cl > NS_CERT_TRAILER_LEN ) {
> >+	while ( cl >= NS_CERT_TRAILER_LEN ) {
> 
> I think this change alone is enough to fix this bug.
> The other + 1 change isn't necessary, because certbegin
> doesn't need to point to the exact beginning of the cert,
> and certend doesn't need to point at the byte after the
> exact end of the cert.

Yes, I agree. This was the first patch I had tested, and I confirm it worked.

Comment 41

[Wan-Teh Chang]

Comment on attachment 336966 [details] [diff] [review]
proposed patch (short term emergency fix)

I suggest that we omit the +1 change then.
The line ending may be \r\n, in which case
the + 1 would only skip over the \r, leaving
cp pointing to the \n rather than the very
first byte of the cert.

Comment 42

[Kai Engert (:KaiE:)]

Comment on attachment 336966 [details] [diff] [review]
proposed patch (short term emergency fix)

Requesting approval for including this fix for a bad regression into Firefox 3.0.2

We want to deliver an updated NSS tag into Firefox 3.0.x that has this patch as its single change.

Comment 45

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 336964 [details] [diff] [review]
proposed patch (long term)

wtc please review for trunk

Comment 46

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #42)
> (From update of attachment 336966 [details] [diff] [review])
> Requesting approval for including this fix for a bad regression into Firefox
> 3.0.2

To be clear, the regression is that certificates delivered in ASCII without a trailing \n are no longer successfully imported?

Is a trailing \n required by some specification? Do we have any idea if this is a common case? Might Eddy be able to help us out by modifying the way he delivers these certs?

Just looking for possible mitigations ...

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #46)

> To be clear, the regression is that certificates delivered in ASCII without a
> trailing \n are no longer successfully imported?

certs delivered with no character following the final ---- are no longer 
successfully imported.  It doesn't have to be \n.  But the code requires that
there exist one more byte past the final trailing -----.

> Is a trailing \n required by some specification? 

No.  It's an off-by-one error.

> Do we have any idea if this is a common case? 

We have received no reports from any CA other than Eddy's, but this 
probably only means that Eddy is helping us by testing nightlies where
the other CAs may not be doing so.  

So, no, we don't really know if this is common or not. 
Personally, I doubt that it is, but we really don't know.

> Might Eddy be able to help us out by modifying the way he
> delivers these certs? 

If he is willing to add another character onto the end of his cert 
downloads, that will avoid this problem and will avoid any last 
minute changes to the code for FF 303

Comment 48

[Kai Engert (:KaiE:)]

I think this might not be limited to installing personal certs, but might affect various kinds of cert downloads.

Everything that gets routed through PSM's function getCertsFromPackage could be affected. One more example is installing a CA cert by downloading it from a web site.

I'd vote to respin.

Comment 49

[Kai Engert (:KaiE:)]

(In reply to comment #46)
> To be clear, the regression is that certificates delivered in ASCII without a
> trailing \n are no longer successfully imported?

This is about the PEM encoding format for x.509 certificates, that uses a trailing line with:
-----END CERTIFICATE-----

Comment 50

[Kai Engert (:KaiE:)]

I have proven that this affects other download types, too.
Test case:
  http://kuix.de/misc/test453227/ca.php

With FF 3.0.1 and earlier, you'll get an offering to install a new CA root cert.

With the current 3.0.2 pre, you get silence.

Comment 51

[Wan-Teh Chang]

Comment on attachment 336964 [details] [diff] [review]
proposed patch (long term)

We should declare 'found' as PRBool and use PR_TRUE and PR_FALSE
instead of 1 and 0.

But the 'found' change is not necessary.  Our base64 decoder
automatically skips line endings.

> 	/* skip to next eol */
>-	do {
>+	while ( cl && ( *cp != '\n')) {
> 	    cp++;
> 	    cl--;
>-	} while ( ( *cp != '\n') && cl );
>+	} 
> 
> 	/* skip all blank lines */
>-	while ( ( *cp == '\n') && cl ) {
>+	while ( cl && ( *cp == '\n')) {
> 	    cp++;
> 	    cl--;
> 	}

These changes are good.  We should test 'cl' before dereferencing
'cp'.

Nit: add a space before the closing parentheses in the four while
loop tests to maintain the original style.

If the input file may have DOS \r\n line endings, the while loop
to "skip all blank lines" doesn't work as intended.  (This problem
exists in the original code.)

Comment 52

[Eddy Nigg (StartCom)]

(In reply to comment #47)
> > Might Eddy be able to help us out by modifying the way he
> > delivers these certs? 
> 
> If he is willing to add another character onto the end of his cert 
> downloads, that will avoid this problem and will avoid any last 
> minute changes to the code for FF 303

Wouldn't be a problem from our side, however I want to make sure that it doesn't break older FF versions if possible. Otherwise we'd have to hack around the various versions...

Comment 53

[Carsten Book [:Tomcat]]

i will create a litmus testcase for this.

Comment 54

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 336966 [details] [diff] [review]
proposed patch (short term emergency fix)

a=beltzner

AIUI, we can't just apply this patch to our own tag for this, we now need a patch that updates NSS to a version that includes it, right?

Comment 55

[[e-mail] (:bhearsum)]

(In reply to comment #38)
> The tag currently being used by the Firefox 3.0.x branch is NSS_3_12_1_RC1.
> This tag would work as the base tag for a 3.12.1 mini-branch.

(In reply to comment #54)
> (From update of attachment 336966 [details] [diff] [review])
> a=beltzner
> 
> AIUI, we can't just apply this patch to our own tag for this, we now need a
> patch that updates NSS to a version that includes it, right?



We pull NSS from the "release tag" (eg, FIREFOX_3_0_2_RELEASE) for releases (see client.mk: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=client.mk&branch=GECKO190_20080827_RELBRANCH&root=/cvsroot&subdir=mozilla&command=DIFF_FRAMESET&rev1=1.383&rev2=1.383.2.1). This tag lives on the release branch, (eg, GECKO190_20080827_RELBRANCH). NSS is part of this release branch.

AFAICT we can land this fix on it and kick-off the release automation.

If you guys want to make an NSS release tag for your own sake, that's cool too, of course, but for the purposes of Firefox 3.0.2 it only needs to land on the release branch (GECKO190_20080827_RELBRANCH)

Does that make sense to you, Kaie/Nelson?

Comment 56

[Wan-Teh Chang]

It's best to wait for an NSS release tag because there are one or
two version strings that need to be updated.

You actually don't need to change NSPR_CO_TAG and NSS_CO_TAG in
client.mk in your release branch or release tag.  Their original
values (e.g., NSPR_4_7_1_RTM and NSS_3_12_1_RC1) are more useful
because they contain the version numbers.

Comment 57

[[e-mail] (:bhearsum)]

(In reply to comment #56)
> It's best to wait for an NSS release tag because there are one or
> two version strings that need to be updated.
> 
> You actually don't need to change NSPR_CO_TAG and NSS_CO_TAG in
> client.mk in your release branch or release tag.  Their original
> values (e.g., NSPR_4_7_1_RTM and NSS_3_12_1_RC1) are more useful
> because they contain the version numbers.

Only updating the NSS release tag will *not* get it into the release. It *must* land on the release branch, too. client.mk on the 3.0.2 release branch pulls NSS from the 'FIREFOX_3_0_2_RELEASE' tag.

Does that make sense?

Comment 58

[Wan-Teh Chang]

Sorry about the confusion.  I meant that you don't need to change
the value of NSPR_CO_TAG from NSPR_x_y_z_RTM to FIREFOX_a_b_c_RELEASE.
You do need to change NSPR_CO_TAG to NSPR_x1_y1_z1_RTM to pick up a
new NSPR release.  Similarly for NSS_CO_TAG.

Comment 59

[[e-mail] (:bhearsum)]

(In reply to comment #58)
> Sorry about the confusion.  I meant that you don't need to change
> the value of NSPR_CO_TAG from NSPR_x_y_z_RTM to FIREFOX_a_b_c_RELEASE.
> You do need to change NSPR_CO_TAG to NSPR_x1_y1_z1_RTM to pick up a
> new NSPR release.  Similarly for NSS_CO_TAG.

We *do* need to change it, actually. This is how our releases have worked for...a long time. When we respin this is what our automation will do:
* it will check-out GECKO190_20080827_RELBRANCH and update it to tip (which contains a client.mk w/ NSS_CO_TAG = FIREFOX_3_0_2_RELEASE)
* it will move the FIREFOX_3_0_2_RELEASE tag to the tip of GECKO190_20080827_RELBRANCH

And then our builds pull from FIREFOX_3_0_2_RELEASE.

Does that make clear how the NSS/NSPR isn't relevant for the release? We still need it for cvs trunk, of course, but for the purposes of the release we just need this patch on the release branch.

Comment 60

[Wan-Teh Chang]

You still need to wait for an NSS release tag that has the version
strings updated properly.  The version strings embedded in the NSS
shared libraries are important to us.  I'm sorry about the delay.

Comment 61

[[e-mail] (:bhearsum)]

(In reply to comment #60)
> You still need to wait for an NSS release tag that has the version
> strings updated properly.  The version strings embedded in the NSS
> shared libraries are important to us.  I'm sorry about the delay.

Are you saying that there is an additional patch that needs to land first? For the purposes of Firefox 3.0.2, the *only* branch that matters now is GECKO190_20080827_RELBRANCH. Things landing on CVS trunk will *not* make it into the release.

I totally understand if we need to wait for some sort of version bump patch, but I want to make sure we're not delaying unnecessarily.

Comment 62

[Wan-Teh Chang]

Christophe, could you generate a new NSS tag?  It needs to
contain Nelson's "proposed patch (short term emergency fix)"
(attachment 336966 [details] [diff] [review]), which hasn't been checked in when I
wrote this comment.

I don't know if NSS 3.12.1 has been released or not, and
whether we need to update the softoken's version string
even though the softoken hasn't changed since NSS_3_12_1_RC1.
Otherwise I would create this new NSS tag myself.

Comment 63

[Nelson Bolyard (seldom reads bugmail)]

Ben, the revision shown by the link in comment 55 appears to break a long 
standing agreement between NSS and Mozilla.  The NSS team does not maintain 
any branches except its own, and supports only its own release tags.  So FF 
should pull NSS from NSS's tags, not make its own copy on its own branch, 
if it wants and expects support from the NSS team.  Any additional branch 
you make becomes a downstream copy and the NSS team doesn't maintain 
downstream copies.  So, I urge you to go back to using NSS tags.  

We will make a new tag today.  I imagine it will be NSS_3_12_1_RCn for some n.

Comment 64

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, it appears there is no 3.12.1 RTM tag yet, so I propose to make
this new tag be NSS_3_12_2_RC2, in a "mini branch" off of the NSS_3_12_1_RC1 
tag as the base tag.

Comment 65

[[e-mail] (:bhearsum)]

(In reply to comment #63)
> Ben, the revision shown by the link in comment 55 appears to break a long 
> standing agreement between NSS and Mozilla.  The NSS team does not maintain 
> any branches except its own, and supports only its own release tags.  So FF 
> should pull NSS from NSS's tags, not make its own copy on its own branch, 
> if it wants and expects support from the NSS team.  Any additional branch 
> you make becomes a downstream copy and the NSS team doesn't maintain 
> downstream copies.  So, I urge you to go back to using NSS tags.

So, it appears we've been doing this since July 2007 (when we started using our release automation regularly, I believe). I believe you that there was an agreement about this, I do not think anyone still on the Build/Release team was aware of it. Let's revisit this after the release.

For our purposes of shipping Firefox 3.0.2 may I land the certread.c patch (and any other necessary patches) on our release branch to get us going?

Comment 66

[Nelson Bolyard (seldom reads bugmail)]

My mistake.  I propose that the new tag be NSS_3_12_1_RC2.  not 3_12_2_RC2.
BTW, I believe Christophe is on vacation this week.  Not sure.

Comment 67

[Nelson Bolyard (seldom reads bugmail)]

Ben, there should be LOTS of people still in MoCo who are very aware of 
the agreement.  Ultimately, the Firefox tree is yours to do with as you
wish, of course, and if you want a downstream tree, that's ultimately up
to you.  But you should be aware that the NSS team won't support binaries 
built from a tree that doesn't match a tag on an NSS team branch (or trunk) 
created by the NSS team.  You might ask dveditz for some history here.

I have created the NSS_3_12_1_BRANCH branch.

Comment 68

[Nelson Bolyard (seldom reads bugmail)]

Kai, also for the "long term fix", we need to figure out why this failure
did not cause any error or warning of any kind to be displayed to the user,
and fix that.  Perhaps the NSS function returned an indication of success 
instead of failure?

Comment 69

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 52, 
Eddy, extra trailing characters will be no problem for older browsers.

Comment 70

[Kai Engert (:KaiE:)]

(In reply to comment #61)
> 
> Are you saying that there is an additional patch that needs to land first?

To summarize: The NSS would prefer if you could use one small patch on top of that: A patch that changes the embedded version number to NSS 3.12.1 RC 2

How this is done at the NSS level shouldn't matter here.
The NSS team should create their RC2 ASAP and produce a diff between RC1 and RC2 and provide it ASAP.

Comment 71

[Kai Engert (:KaiE:)]

Attachment: Version number patch

firefox 3.0.2 release drivers, please take this change on top of the code patch.

Comment 72

[Nelson Bolyard (seldom reads bugmail)]

I landed the patch, with the extra comments suggested in comment 35, 
on the NSS_3_12_1_BRANCH.  Kai will update the version number string 
in nss.h and then tag the branch with NSS_3_12_1_RC2.

Comment 73

[Kai Engert (:KaiE:)]

Comment on attachment 337070 [details] [diff] [review]
Version number patch

sorry made a mistake

Comment 74

[Kai Engert (:KaiE:)]

Attachment: version number patch, take 2, change to 3.12.1.1 (RC2)

Comment 75

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 337070 [details] [diff] [review]
Version number patch

The basis for the r- is this:

>-#define NSS_VERSION  "3.12.1.0" _NSS_ECC_STRING _NSS_CUSTOMIZED
>+#define NSS_VERSION  "3.12.1.1" _NSS_ECC_STRING _NSS_CUSTOMIZED
> #define NSS_VMAJOR   3
> #define NSS_VMINOR   12
>-#define NSS_VPATCH   1
>+#define NSS_VPATCH   2

That last change changes the version to 3.12.2.

Comment 76

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 337073 [details] [diff] [review]
version number patch, take 2, change to 3.12.1.1 (RC2)

r=nelson

Comment 77

[Wan-Teh Chang]

Comment on attachment 337073 [details] [diff] [review]
version number patch, take 2, change to 3.12.1.1 (RC2)

r=wtc.

Comment 78

[Kai Engert (:KaiE:)]

Attachment: patch to mozilla/client.mk to bump the 1.9.0 branch to newer NSS tag

Comment 79

[Kai Engert (:KaiE:)]

Attachment: FYI, mozcvs rdiff -u -r NSS_3_12_1_RC1 -r NSS_3_12_1_RC2

FYI, this is the output of

mozcvs rdiff -u -r NSS_3_12_1_RC1 -r NSS_3_12_1_RC2 mozilla/security/nss mozilla/security/coreconf mozilla/security/dbm mozilla/dbm

Comment 80

[Kai Engert (:KaiE:)]

I want to land new tag NSS_3_12_1_RC2 into mozilla-central, no patch needed, could you please r+ this comment?

Comment 81

[Wan-Teh Chang]

r=wtc.

Comment 82

[Daniel Veditz [:dveditz]]

Comment on attachment 337075 [details] [diff] [review]
patch to mozilla/client.mk to bump the 1.9.0 branch to newer NSS tag

Approved for 1.9.0.3, a=dveditz to land on cvs-trunk.

Comment 83

[Daniel Veditz [:dveditz]]

Comment on attachment 337076 [details] [diff] [review]
FYI, mozcvs rdiff -u -r NSS_3_12_1_RC1 -r NSS_3_12_1_RC2

Approved for 1.9.0.2, a=dveditz for release-drivers, please land this change on GECKO190_20080827_RELBRANCH and add the "fixed1.9.0.2" keyword.

Comment 84

[Eddy Nigg (StartCom)]

(In reply to comment #69)
> In reply to comment 52, 
> Eddy, extra trailing characters will be no problem for older browsers.

I just wonder if browsers aren't trimming the response internally in any case. We've been trimming our responses since ever (IIRC MSIE needs it, maybe others too). For now I'll leave as is in order to have the bug confirmed fixed also by the reporter. After that I'll consider adding a"\n" to our current code.

Thanks to all for the effort to have this fixed, specially Kai and Nelson!

Comment 85

[Reed Loden [:reed]]

GECKO190_20080827_RELBRANCH:

Checking in security/nss/lib/nss/nss.h;
/cvsroot/mozilla/security/nss/lib/nss/nss.h,v  <--  nss.h
new revision: 1.58.2.1; previous revision: 1.58
done
Checking in security/nss/lib/pkcs7/certread.c;
/cvsroot/mozilla/security/nss/lib/pkcs7/certread.c,v  <--  certread.c
new revision: 1.13.2.1; previous revision: 1.13
done

Comment 86

[Kai Engert (:KaiE:)]

(In reply to comment #80)
> I want to land new tag NSS_3_12_1_RC2 into mozilla-central, no patch needed,
> could you please r+ this comment?

(In reply to comment #81)
> r=wtc.

Landed into mozilla-central
a9f50bdece8b

Comment 87

[Kai Engert (:KaiE:)]

(In reply to comment #82)
> (From update of attachment 337075 [details] [diff] [review])
> Approved for 1.9.0.3, a=dveditz to land on cvs-trunk.

Checked in

Checking in client.mk;
/cvsroot/mozilla/client.mk,v  <--  client.mk
new revision: 1.384; previous revision: 1.383
done

Comment 88

[Kai Engert (:KaiE:)]

marking fixed

Comment 89

[Nelson Bolyard (seldom reads bugmail)]

This is still an issue on the NSS trunk for 3.12.2.
I will submit a better patch to supersede attachment 336964 [details] [diff] [review].

Comment 90

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for NSS trunk, v2 (checked in on trunk)

I beleive this patch makes the code do what it was originally intended to do,
but never before actually did properly.

Comment 91

[Wan-Teh Chang]

Comment on attachment 337101 [details] [diff] [review]
patch for NSS trunk, v2 (checked in on trunk)

Please declare 'found' as PRBool and use PR_FALSE and PR_TRUE
instead of 0 and 1.

I still think the 'found' change is not necessary because the
base64 decoder will skip the blank lines.  Also, the 'found'
change will skip any junk after the NS_CERT_HEADER, for example,

-----BEGIN CERTIFICATE-----JUNKjunkJUNK\n

I believe this is an unintended effect of the 'found' change.

Nit: I would write the while loop tests like this:

+	while ( cl && ( *cp != '\n' ) ) {

i.e., an extra space before the outer closing parenthesis.

Comment 92

[Wan-Teh Chang]

If you really want the 'found' change, you need to fix it
like this (the indentation needs to be fixed):

+   if (!found) {
 	/* skip to next eol */
-	do {
+	while ( cl && ( *cp != '\n' )) {
 	    cp++;
 	    cl--;
-	} while ( ( *cp != '\n') && cl );
+	}
+   }

That is, if 'found' is true, we should jump to the
second inner while loop to skip only blank lines.

Comment 93

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, 
The effect of skipping trailing junk on the ----BEGIN line is intended.  
I fully intend for that to be done.
The intent of this code has always been to present to the B64 decoder just
the B64 encoded part, without any extraneous stuff at the beginning or end.

The spec for -----BEGIN, such as it is, does not preclude any such junk.  
IIRC, the PEM RFCs (from which that style comes) allow trailing white space.  

As for using NSPR types in this function, I know a certain Google employee 
who (it seems to me) lately has been strongly advocating not using NSPR 
functions and types in NSS when it's not really necessary.  
How do you reconcile this with that?

Comment 94

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 337101 [details] [diff] [review]
patch for NSS trunk, v2 (checked in on trunk)

In case it's not clear, comment 93 is intended as a refutation of comment 92.

Comment 95

[Wan-Teh Chang]

The old code and the current code don't allow non-whitespace
junk after NS_CERT_HEADER.  I don't understand why we should
start to allow that now.  I am tired of endless debates, so
you decide what's right.

I only advocated not using NSPR functions in NSS in the context
of the NSPR-free freebl work, and only to reduce the number
of NSPR stub functions Bob Relyea has to add to freebl.  Again,
I am tired of endless debates.  You can use int and 0 and 1 for
a boolean.

Comment 96

[Carsten Book [:Tomcat]]

Verified fixed for 1.9.0.2 using Kai's Testcase from comment#50 and Firefox 3.0.2 Build5:

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.0.2) Gecko/2008090514 Firefox/3.0.2
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.2) Gecko/2008090512 Firefox/3.0.2
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2) Gecko/2008090512 Firefox/3.0.2 

Download Certificate Window opens as expected and certificate is added to the Certificate Manager. 

Also created the Litmus Testcase -> https://litmus.mozilla.org/show_test.cgi?id=7030

Comment 97

[Kai Engert (:KaiE:)]

I propose to mark this bug as FIXED and file a follow up bug to do any remaining work.

Comment 98

[Kai Engert (:KaiE:)]

Wan-Teh reminded me that no fix has landed yet on the NSS trunk.

I propose that attachment 336966 [details] [diff] [review] (the short term emergency patch) gets commited to trunk, too, until we are able to reach agreement on additional changes.

Any objections? If you don't object, I offer to check it in, mark this bug fixed, and file the follow up bug.

Comment 99

[Nelson Bolyard (seldom reads bugmail)]

I object.

Comment 100

[Wan-Teh Chang]

Comment on attachment 337101 [details] [diff] [review]
patch for NSS trunk, v2 (checked in on trunk)

r=wtc.

I remember I set review+ on this patch when I wrote comment 91.
Must have been a browser bug.

Comment 101

[Eddy Nigg (StartCom)]

Friends, this is a critical - P1 bug. Can we get this done in an efficient way?

Comment 102

[Nelson Bolyard (seldom reads bugmail)]

Eddy, It's fixed in the FF 3.0.2 respin.  What more do you need?

Comment 103

[Eddy Nigg (StartCom)]

Trunk? 3.1 (aka 1.9.1)? Or will this go in there anyway? I understood that the fix is of temporary nature...

Comment 104

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 337101 [details] [diff] [review]
patch for NSS trunk, v2 (checked in on trunk)

Here are some additional explanatory comments about the patch I wrote for
the trunk.  

Having known the original author of this very old code, I believe I can 
state with confidence what the code was originally intended to do,
but which it did not do very well.  

Beginning with the line that bears the comment:
   /* find the beginning marker */
it was supposed to find a line that *begins* with the special string:
-----BEGIN CERTIFICATE-----
(not including any trailing characters after the 5 trailing dashes),
then discard that line, including any trailing characters on that line,
and any/all trailing end-of-line characters.  The first non-EOL character 
after the end of the line with that special string is intended to be the 
beginning of the BASE64-encoded data that contains the cert(s).  This 
beginning point is always the first character in a line of text.

At some point after that starting position, it is supposed to find an 
subsequent line of text that begins with the special string: 
-----END CERTIFICATE-----
(again, not including any trailing characters after the 5 trailing dashes).
The character preceding the first dash in that special string is the 
last character to be included in the BASE64-encoded data to be subsequently
decoded.  All the characters on all the lines of text beginning with the 
beginning point (describe above), up to and including the last character 
before the special ending string are to be included in the data passed to
the base64 decoder.  (I will not describe the steps after this.)

Since it was first publicly committed as cvs rev 1.1 in the opem mozilla
repository, the code has failed to do those things properly.  It clearly
assumed that the text used Unix style line endings, and utterly failed to
deal with \r characters.  It also failed to discard the entire line on
which the special beginning string was found.  It also forgot to subtract
the number of characters discarded from that first line from the count of
remaining characters in the buffer, which led to the crash that was 
bug 414003.  In most cases, it checked for having come to the end of a
buffer AFTER it had checked the character that might be the first one
after the end of the buffer.  

This patch attempts to address all these problems by
a) not relying on counting trailing NUL characters in constant strings 
as part of the computation of remaining characters, 
b) counting and discarding all the intended characters after the special
beginning string, including both \r and \n characters,
c) detecting that the code has come to the end of the buffer BEFORE testing
the character that could be the first one past the end, and 
d) not requiring that the line containing the special ending string be 
terminated with an EOL character.

Comment 105

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 103, unless and until such time as the NSS team 
subsequently produces another new NSS tag (whether on trunk or branch), 
and the Firefox team chooses to take the code from that NSS tag, Firefox 
builds (including minefield nightly builds) will continue to use the code 
that is now NSS 3.12.1.

Comment 106

[Eddy Nigg (StartCom)]

Perhaps if we are at it please allow me to make another suggestion. How about checking only for the chars "-----" at the respective lines (header and footer) without checking for anything beyond. Discharge those lines sine I believe that only the BASE64-encoded data is feed to the decoder.

That's because a valid response can be the BASE64-encoded data itself without any headers and footers, "-----BEGIN CERTIFICATE-----" and "-----BEGIN PKCS7-----". The later is how OpenSSL outputs it for example. Other libraries allow for any of the above variations (Certenroll/Xenroll). The approach suggested above might eliminate all the additional checks such as file endings and any other junk which might be present at the end...

Comment 107

[Julien Pierre]

Comment on attachment 337101 [details] [diff] [review]
patch for NSS trunk, v2 (checked in on trunk)

The patch does what it intends to do, so r+.

There is one thing which bothers me a little - we don't check the length of the object in between the headers at all before we decode. Even if certbegin is equal to certend, we still proceed and go to the ATOB call. I sort of wonder what happens there since ATOB_AsciiToData only takes a starting pointer but not an ending pointer or an input length. It only returns a length.

I'm guessing this won't crash because ATOB_AsciiToData won't be able to decode the end marker properly, and the string is NULL-terminated, but it seems still slightly hackish.

Purely optionally, I would suggest adding this line when certend is assigned :

cp[0] = '\0'; /* terminate base-64 object  */

This makes the code more readable and only try to decode what we intend to decode, not including the trailer.

Comment 108

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 337101 [details] [diff] [review]
patch for NSS trunk, v2 (checked in on trunk)

Checking in certread.c; new revision: 1.16; previous revision: 1.15

Comment 109

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.4 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4pre) Gecko/2008102304 GranParadiso/3.0.4pre using testcases listed here.