Fixes to SSL/TLS analyzer

Analyzer can cope with zero length client and server
certificates.  It does still generate a weird though.
Extended cipherspec_size weirds are not thrown anymore,
they are incredibly overwhelming and should be handled
completely at the scripting in my opinion.

Integrated and expanded on patch Rmkml from ticket #209
that fixes problem with not parsing or expecting SSL
extensions.  SSL extensions still are not extracted
and passed to script land, but the analyzer doesn't
fail anymore.
This commit is contained in:
Seth Hall 2011-01-28 16:24:07 -05:00
parent c8076619ce
commit 1ccfca09ac
2 changed files with 92 additions and 98 deletions

View file

@ -174,7 +174,6 @@ bool SSL_RecordBuilder::addSegment(const u_char* data, int length)
if ( ! computeExpectedSize(data, length) ) if ( ! computeExpectedSize(data, length) )
return false; return false;
// Insert weird here replacing assert.
if ( neededSize > expectedSize ) if ( neededSize > expectedSize )
{ {
sslEndpoint->Weird("SSL_RecordBuilder::addSegment neededSize > expectedSize"); sslEndpoint->Weird("SSL_RecordBuilder::addSegment neededSize > expectedSize");
@ -278,7 +277,6 @@ bool SSL_RecordBuilder::addSegment(const u_char* data, int length)
{ // another (middle) segment { // another (middle) segment
if ( length <= MIN_FRAGMENT_SIZE ) if ( length <= MIN_FRAGMENT_SIZE )
sslEndpoint->Parent()->Weird( "SSLProxy: Excessive small TCP Segment!" ); sslEndpoint->Parent()->Weird( "SSLProxy: Excessive small TCP Segment!" );
addData(data, length); addData(data, length);
break; break;
} }

View file

@ -382,24 +382,12 @@ void SSLv3_Interpreter::DeliverSSLv3_Record(SSLv3_HandshakeRecord* rec)
} }
case SSL3_1_CERTIFICATE: case SSL3_1_CERTIFICATE:
{
if ( rec->length >= 3 )
{ {
const u_char* pData = rec->data; const u_char* pData = rec->data;
uint32 certListLength = uint32 certListLength =
uint32((pData[4] << 16) | uint32((pData[4] << 16) |
pData[5] << 8) | pData[6]; pData[5] << 8) | pData[6];
// Size consistency checks.
if ( certListLength + 3 != uint32(rec->length) )
{
if ( rec->endp->IsOrig() )
Weird("SSLv3x: Corrupt length field in client certificate list!");
else
Weird("SSLv3x: Corrupt length field in server certificate list!");
return;
}
// Sum of all cert sizes has to match // Sum of all cert sizes has to match
// certListLength. // certListLength.
uint tempLength = 0; uint tempLength = 0;
@ -430,7 +418,9 @@ void SSLv3_Interpreter::DeliverSSLv3_Record(SSLv3_HandshakeRecord* rec)
(SSL_InterpreterEndpoint*) rec->endp; (SSL_InterpreterEndpoint*) rec->endp;
if ( certCount == 0 ) if ( certCount == 0 )
{ // we don't have a certificate... {
// we don't have a certificate, but this is valid
// according to RFC2246
if ( rec->endp->IsOrig() ) if ( rec->endp->IsOrig() )
{ {
Weird("SSLv3x: Client certificate is missing!"); Weird("SSLv3x: Client certificate is missing!");
@ -458,9 +448,6 @@ void SSLv3_Interpreter::DeliverSSLv3_Record(SSLv3_HandshakeRecord* rec)
certListLength-3, 1, false); certListLength-3, 1, false);
} }
}
else
Weird("SSLv3x: Certificate record too small!" );
break; break;
} }
@ -938,13 +925,15 @@ TableVal* SSLv3_Interpreter::analyzeCiphers(const SSLv3_Endpoint* s, int length,
{ {
int is_orig = (SSL_InterpreterEndpoint*) s == orig; int is_orig = (SSL_InterpreterEndpoint*) s == orig;
if ( length > ssl_max_cipherspec_size ) // This probably shouldn't be a weird. This data should be passed to
{ // script layer and dealt with there as appropriate.
if ( is_orig ) //if ( length > ssl_max_cipherspec_size )
Weird("SSLv3: Client has CipherSpecs > ssl_max_cipherspec_size"); // {
else // if ( is_orig )
Weird("SSLv3: Server has CipherSpecs > ssl_max_cipherspec_size"); // Weird("SSLv3: Client has CipherSpecs > ssl_max_cipherspec_size");
} // else
// Weird("SSLv3: Server has CipherSpecs > ssl_max_cipherspec_size");
// }
const u_char* pCipher = data; const u_char* pCipher = data;
SSL_CipherSpec* pCipherSuiteTemp = 0; SSL_CipherSpec* pCipherSuiteTemp = 0;
@ -1236,16 +1225,6 @@ SSLv3_HandshakeRecord::SSLv3_HandshakeRecord(const u_char* data, int len,
uint16 version, SSLv3_Endpoint const* e) uint16 version, SSLv3_Endpoint const* e)
: SSLv3_Record(data, len, version, e) : SSLv3_Record(data, len, version, e)
{ {
// Weird-check for minimum handshake length header.
if ( len < 4 )
{
e->Interpreter()->Weird("SSLv3x: Handshake-header-length too small!");
type = 255;
length = 0;
next = 0;
return;
}
// Don't analyze encrypted client handshake messages. // Don't analyze encrypted client handshake messages.
if ( e->IsOrig() && if ( e->IsOrig() &&
((SSLv3_Interpreter*) e->Interpreter())->change_cipher_client_seen && ((SSLv3_Interpreter*) e->Interpreter())->change_cipher_client_seen &&
@ -1270,7 +1249,10 @@ SSLv3_HandshakeRecord::SSLv3_HandshakeRecord(const u_char* data, int len,
type = uint8(*(this->data)); type = uint8(*(this->data));
length = ExtractInt24(data, len, 1); length = ExtractInt24(data, len, 1);
if ( length + 4 < len )
if ( length == 0 ) // this is a special case to deal with 0 length certs
next = 0;
else if ( length + 4 < len )
next = new SSLv3_HandshakeRecord(data + length + 4, next = new SSLv3_HandshakeRecord(data + length + 4,
len - (length + 4), version, e); len - (length + 4), version, e);
else if ( length + 4 > len ) else if ( length + 4 > len )
@ -1340,7 +1322,6 @@ int SSLv3_HandshakeRecord::checkClientHello()
uint16 cipherSuiteLength = uint16 cipherSuiteLength =
uint16(data[offset] << 8) | data[offset+1]; uint16(data[offset] << 8) | data[offset+1];
offset += (2 + cipherSuiteLength); offset += (2 + cipherSuiteLength);
if ( cipherSuiteLength < 2 ) if ( cipherSuiteLength < 2 )
endp->Interpreter()->Weird("SSLv3x: CipherSuite length too small!"); endp->Interpreter()->Weird("SSLv3x: CipherSuite length too small!");
@ -1352,16 +1333,14 @@ int SSLv3_HandshakeRecord::checkClientHello()
uint8 compressionMethodLength = uint8(data[offset]); uint8 compressionMethodLength = uint8(data[offset]);
offset += (1 + compressionMethodLength); offset += (1 + compressionMethodLength);
if ( compressionMethodLength < 1 ) if ( compressionMethodLength < 1 )
endp->Interpreter()->Weird("SSLv3x: CompressionMethod length too small!"); endp->Interpreter()->Weird("SSLv3x: CompressionMethod length too small!");
if ( offset != length ) if ( offset < length )
{ {
uint16 sslExtensionsLength = uint16 sslExtensionsLength =
uint16(data[offset] << 8 ) | data[offset+1]; uint16(data[offset] << 8) | data[offset+1];
offset += 2; offset += 2;
if ( sslExtensionsLength < 4 ) if ( sslExtensionsLength < 4 )
endp->Interpreter()->Weird("SSLv3x: Extensions length too small!"); endp->Interpreter()->Weird("SSLv3x: Extensions length too small!");
@ -1391,19 +1370,36 @@ int SSLv3_HandshakeRecord::checkServerHello()
version != SSLProxy_Analyzer::SSLv31 ) version != SSLProxy_Analyzer::SSLv31 )
endp->Interpreter()->Weird("SSLv3x: Corrupt version information in Server hello!"); endp->Interpreter()->Weird("SSLv3x: Corrupt version information in Server hello!");
uint8 sessionIDLength = uint8(data[38]); uint16 offset = 38;
uint8 sessionIDLength = uint8(data[offset]);
if ( sessionIDLength > 32 ) if ( sessionIDLength > 32 )
{ {
endp->Interpreter()->Weird("SSLv3x: SessionID too long in Server hello!"); endp->Interpreter()->Weird("SSLv3x: SessionID too long in Server hello!");
return 0; return 0;
} }
offset += (1 + sessionIDLength);
if ( (sessionIDLength + 45) != length ) offset += 3; // account for cipher and compression method
if ( offset < length )
{
uint16 sslExtensionsLength =
uint16(data[offset] << 8) | data[offset+1];
offset += 2;
if ( sslExtensionsLength < 4 )
endp->Interpreter()->Weird("SSLv3x: Extensions length too small!");
// TODO: extract SSL extensions here
offset += sslExtensionsLength;
if ( offset != length+4 )
{ {
endp->Interpreter()->Weird("SSLv3x: Corrupt length fields in Server hello!"); endp->Interpreter()->Weird("SSLv3x: Corrupt length fields in Server hello!");
return 0; return 0;
} }
return 0;
}
return 1; return 1;
} }