[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:          |  
--------------------+-------------------------------------------------------

Comment(by wehlhard):

 Replying to [comment:8 datallah]:
 > 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 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.

 I honestly don't remember any more why I hand-rolled a verifier; I suspect
 it had to do with GnuTLS API shortcomings, real or imagined. I agree that
 it'd be ideal to offload as much of the SSL functionality into the library
 as possible, *particularly since this is security software*. This
 unfortunately requires some significant plumbing, since libpurple
 abstracts away its SSL provider so that it can support multiple libraries.



 > > libpurple/certificate.c:298 /* If this is a single-certificate chain,
 say that it is valid */
 > > [[BR]]!^ ... 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`.

 Daniel's analysis here is correct. This function only checks the integrity
 of the chain, not its root.



 I'm pleased to see that paranoid people are actually looking for problems
 in this code; SSL is frequently implemented dangerously wrongly, and while
 I did my best, I'm certain that I didn't have the time or skills to think
 of everything that could possibly go wrong with security implementation.
 Also, I've never had a lobotomy nor any other serious head trauma, so you
 can rule that out as a cause.

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