Bug#828557: Patch for sslsniff, request for review
Sebastian Andrzej Siewior
sebastian at breakpoint.cc
Mon Dec 18 20:25:54 UTC 2017
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
More information about the Pkg-security-team
mailing list