Bug#828557: Patch for sslsniff, request for review

[sebastian@breakpoint.cc (Sebastian Andrzej Siewior)]

On 2017-12-17 19:32:52 [+0100], Hilko Bengen wrote:
> Control: tag -1 patch
> 
> Hi,
> 
> here's a patch that fixes the OpenSSL-1.1-related FTBFS for sslsniff.
> 
> I'd appreciate a review of the patch.

It is not back compatible with openssl 1.0.2

>Index: sslsniff/SessionCache.cpp
>===================================================================
>--- sslsniff.orig/SessionCache.cpp
>+++ sslsniff/SessionCache.cpp
>@@ -47,7 +47,9 @@ void SessionCache::removeSessionId(unsig
> }
> 
> int SessionCache::setNewSessionId(SSL *s, SSL_SESSION *session) {
>-  return setNewSessionId(s, session, session->session_id, session->session_id_length);
>+  unsigned int id_length;
>+  const unsigned char *id = SSL_SESSION_get_id(session, &id_length);
>+  return setNewSessionId(s, session, (unsigned char*)id, id_length);
> }
> 
> int SessionCache::setNewSessionId(SSL *s, SSL_SESSION *session, 
>@@ -117,8 +119,8 @@ SSL_SESSION * SessionCache::getSessionId
> 
> // Trampoline Functions.  Yay C.
> 
>-SSL_SESSION * SessionCache::getSessionIdTramp(SSL *s, unsigned char *id, int idLength, int *ref) {
>-  return SessionCache::getInstance()->getSessionId(s, id, idLength, ref);
>+SSL_SESSION * SessionCache::getSessionIdTramp(SSL *s, const unsigned char *id, int idLength, int *ref) {
>+  return SessionCache::getInstance()->getSessionId(s, (unsigned char*)id, idLength, ref);

since you propage that `const' to getSessionIdTramp(), you could propage it
further and drop that cast.

> }
> 
> int SessionCache::setNewSessionIdTramp(SSL *s, SSL_SESSION *session) {
>Index: sslsniff/SessionCache.hpp
>===================================================================
>--- sslsniff.orig/SessionCache.hpp
>+++ sslsniff/SessionCache.hpp
>@@ -49,7 +49,7 @@ class SessionCache {
> 
> public:
>   static SessionCache* getInstance();
>-  static SSL_SESSION * getSessionIdTramp(SSL *s, unsigned char *id, int idLength, int *ref);
>+  static SSL_SESSION * getSessionIdTramp(SSL *s, const unsigned char *id, int idLength, int *ref);
>   static int setNewSessionIdTramp(SSL *s, SSL_SESSION *session);
> 
>   int setNewSessionId(SSL *s, SSL_SESSION *session);
>Index: sslsniff/certificate/Certificate.hpp
>===================================================================
>--- sslsniff.orig/certificate/Certificate.hpp
>+++ sslsniff/certificate/Certificate.hpp
>@@ -92,7 +92,7 @@ private:
>   }
> 
>   void parseCommonName(X509 *cert) {
>-    std::string distinguishedName(cert->name);
>+    std::string distinguishedName(X509_NAME_oneline(X509_get_subject_name(cert), NULL, 0));

X509_NAME_oneline() allocates memory which you leak.

>     std::string::size_type cnIndex = distinguishedName.find("CN=");

That part grabs the hostname from the CN= part of the subject. I haven't
look *why* this is done but usually one wants to check the "subject
alternative name" extension and the content may conttain an asterisk.

>     if (cnIndex == std::string::npos) throw BadCertificateException();
>Index: sslsniff/certificate/TargetedCertificateManager.cpp
>===================================================================
>--- sslsniff.orig/certificate/TargetedCertificateManager.cpp
>+++ sslsniff/certificate/TargetedCertificateManager.cpp
>@@ -117,6 +117,6 @@ void TargetedCertificateManager::dump()
>   std::list<Certificate*>::iterator i;
> 
>   for(i=certificates.begin(); i != certificates.end(); ++i) 
>-    std::cout << "Certificate: " << (*i)->getCert()->name << std::endl;
>+    std::cout << "Certificate: " << X509_NAME_oneline(X509_get_subject_name((*i)->getCert()), NULL, 0) << std::endl;

also a leak.

> 
> }

> Cheers,
> -Hilko

Sebastian