Opened 11 years ago

Closed 11 years ago

Last modified 8 years ago

#6500 closed patch (fixed)

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 (2)

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

Download all attachments as: .zip

Change History (17)

Changed 11 years ago by LouCipher

Patch to enable certificate verification in NSS SSL plugin

comment:1 Changed 11 years ago by datallah

  • Type changed from defect to patch

comment:2 Changed 11 years ago by wehlhard

  • Owner set to wehlhard

comment:3 Changed 11 years ago by wehlhard

  • Milestone set to 2.5.0
  • Status changed from new to assigned

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

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

  1. 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 11 years ago by wehlhard

Revised patch, with less warnings and more excitement

comment:4 Changed 11 years ago by wehlhard

To whom should I credit this patch?

comment:5 Changed 11 years ago by ari

I believe it was done by Miron Cuperman.

comment:6 Changed 11 years ago by MarkDoliner

  • Cc MarkDoliner added

comment:7 Changed 11 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.

comment:8 Changed 11 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.

comment:9 Changed 11 years ago by LouCipher

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

comment:10 Changed 11 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)?

comment:11 Changed 11 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.

comment:12 Changed 11 years ago by wehlhard

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:13 Changed 11 years ago by MarkDoliner

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

comment:15 Changed 11 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!