[Pidgin] #15308: SSL support appears to have been written by a lobotomy victim

[Pidgin]

#15308: SSL support appears to have been written by a lobotomy victim
--------------------+-------------------------------------------------------
 Reporter:  athena  |        Owner:           
     Type:  defect  |       Status:  pending  
Milestone:          |    Component:  libpurple
  Version:  2.10.6  |   Resolution:           
 Keywords:          |  
--------------------+-------------------------------------------------------
Changes (by datallah):

 * cc: williamehlhardt at gmail.com (added)


Comment:

 Replying to [comment:6 abadidea]:
 > I did try to explain that you appear to have a homerolled certificate
 validator in lieu of the stubbed-out one but it was hard to have the
 conversation over twitter.

 It's always fun and exciting to jump to conclusions and it's easy to get
 people riled up on twitter :)

 > It really gave me a fright to see it stubbed out without remark though,
 so I have a question: what is the rationale for using a homerolled
 validation method separate from NSS, and could that rationale be added
 inline as a comment to forestall any gray hairs in the future? :)

 That seems like a reasonable request, unfortunately I don't know the
 answer.
 The code came out of a Summer of Code project in 2008; I've CC'd William
 who worked on it in the hopes that he may be able to help us.

 It is also possible that this is something that can be improved to take
 advantage of more functionality within NSS - unless there's a compelling
 reason to do it ourselves, it certainly seems like we'd want to make the
 dedicated library do what it's good at.


 > That being said I have some other questions about said home-rolled
 implementation.

 Great.  Code review on this is certainly welcome.

 > libpurple/certificate.c:298 /* If this is a single-certificate chain,
 say that it is valid */
 >
 > ^ ... that doesn't sound right

 I am not particularly familiar with the cert validation code, but looking
 at that line, it appears that the intent of that code is to verify chained
 certificates' expiration and relationship to the first cert, not to
 compare whether the first cert itself can be traced to a valid CA and is
 itself effective based on date (if that makes any sense). The effective
 date of the first cert would have been checked previously in
 `x509_tls_cached_start_verify` and the validation against trusted CAs
 would happen later in `x509_tls_cached_unknown_peer`.


 > libpurple/certificate.c:1671 /* Next, attempt to verify the last
 certificate is signed by a trusted
 >        * CA, or is a trusted CA (based on fingerprint).
 >        */
 >
 > ^ nor this, as it seems to be saying that you intend to accept
 certificates as signers that are not themselves authorities.

 I think what this means is "Either the last in the cert chain is signed by
 one of the trusted CAs or the cert chain actually ends with a trusted CA
 directly"

-- 
Ticket URL: <http://developer.pidgin.im/ticket/15308#comment:8>
Pidgin <http://pidgin.im>
Pidgin