Ticket #6500 (closed patch: fixed)

Opened 2 years ago

Last modified 2 years ago

NSS plugin doesn't verify SSL certificates

Reported by: ari Owned by: wehlhard
Milestone: 2.5.0 Component: libpurple
Version: 2.4.3 Keywords:
Cc: MarkDoliner

Description

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.)

Attachments

nss-cert-verify.patch (5.2 kB) - added by LouCipher 2 years ago.
Patch to enable certificate verification in NSS SSL plugin
nss_add_rev.patch (4.8 kB) - added by wehlhard 2 years ago.
Revised patch, with less warnings and more excitement

Change History

Changed 2 years ago by LouCipher

Patch to enable certificate verification in NSS SSL plugin

Changed 2 years ago by datallah

  • type changed from defect to patch

Changed 2 years ago by wehlhard

  • owner set to wehlhard

Changed 2 years ago by wehlhard

  • status changed from new to assigned
  • milestone set to 2.5.0

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.

Changed 2 years ago by wehlhard

Revised patch, with less warnings and more excitement

Changed 2 years ago by wehlhard

To whom should I credit this patch?

Changed 2 years ago by ari

I believe it was done by Miron Cuperman.

Changed 2 years ago by MarkDoliner

  • cc MarkDoliner added

Changed 2 years ago by 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.

Changed 2 years ago by 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.

Changed 2 years ago by LouCipher

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

Changed 2 years ago by datallah

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

Changed 2 years ago by 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.

Changed 2 years ago by wehlhard

  • status changed from new to closed
  • resolution set to fixed

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

Changed 2 years ago by MarkDoliner

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

Changed 2 years ago by wehlhard

Changed 2 years ago by MarkDoliner

Sweeeeet. Thanks!

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!