NSS plugin doesn't verify SSL certificates

[ari]

Originally from http://bugs.debian.org/492434:

I recently set up a Jabber server. I used the default snakeoil certificate. When I configured Pidgin to connect to my new server, using SSL, it connected without any complaint whatsoever.

(Pidgin in Debian/Ubuntu is built with NSS, under the recommendation of you guys. The GNUTLS plugin apparently does do proper certificate verification. This is a fairly major problem, since people assuming their connections are secure can be subject to man-in-the-middle attacks.)

[LouCipher]

Patch to enable certificate verification in NSS SSL plugin

[wehlhard]

Thank you for submitting this patch; this has been sitting on my TODO list for a long time, and now you have resolved it.

I have a couple things that I would like to see corrected before I put this into the main tree, though:

1. Around line 281 of the revised ssl-nss.c, there is the following:

	cert = SSL_PeerCertificate(socket);
	curcert = CERT_DupCertificate(cert);

While there is a call to CERT_DestroyCertificate for curcert later on, there is no corresponding call for cert itself. Since SSL_PeerCertificate returns a CERT_DupCertificate and returns its result, I think this is a memory leak. Can you double-check this?

See http://mxr.mozilla.org/security/source/security/nss/lib/ssl/sslauth.c#46

2. I get the following when compiling the revised ssl-nss.c:

ssl-nss.c: In function 'x509_import_from_nss':
ssl-nss.c:264: warning: passing argument 1 of 'CERT_DupCertificate' discards qualifiers from pointer target type
ssl-nss.c: In function 'ssl_nss_get_peer_certificates':
ssl-nss.c:291: warning: ISO C90 forbids mixed declarations and code
ssl-nss.c:299: warning: too many arguments for format
ssl-nss.c: In function 'x509_signed_by':
ssl-nss.c:675: warning: 'return' with no value, in function returning non-void
ssl-nss.c:678: warning: 'return' with no value, in function returning non-void
ssl-nss.c:684: warning: this function may return with or without a value

I don't like warnings. The third block of them is particularly worrisome.

3. I put the portion touching the Jabber protocol as its own patch #6516

I have attached a revised patch that covers the above issues. Please double-check that I have made no mistakes, and if I hear back from you in the next few days, this will make it into next week's release.

[wehlhard]

Revised patch, with less warnings and more excitement

[wehlhard]

To whom should I credit this patch?

[ari]

I believe it was done by Miron Cuperman.

[LouCipher]

(haven't been receiving emailed updates for some reason...)

There should be another instance of:

CERT_DestroyCertificate(curcert);

outside the loop. Otherwise looks fine. Sorry for not reviewing the patch more carefully for leaks and warnings.

Please credit Lou Cipher, if you don't mind pseudonyms.

Thanks.

[MarkDoliner]

Pseudonyms are ok for the ChangeLog file, but they're not great for copyright attribution. We'd much prefer to use a real name for that. Or if you want you can agree to assign right copyright to Instant Messaging Freedom, Inc., which is the non-profit we set up to protect developers and what not.

[LouCipher]

I irrevocably assign my copyright in the patch to Instant Messaging Freedom, Inc. . Thanks.

[datallah]

What's the status on this? Do we feel comfortable putting it in 2.5.0 (i.e. committing it today)?

[MarkDoliner]

I haven't looked at it. It seems like it would be good to wait for 2.5.1, since we so close to the 2.5.0 release.

[wehlhard]

I have already reviewed it, and just pushed it to the main repository.

[MarkDoliner]

Should there be an entry in the ChangeLog crediting Lou Cipher?

[wehlhard]

I added one in d36d5715caaa1d97f16a786c8cab3c8cdb66ceff

[MarkDoliner]

Sweeeeet. Thanks!