From d0541587139173ce96331825213f0da97bb7a033 Mon Sep 17 00:00:00 2001 From: Johanna Amann Date: Thu, 27 Aug 2015 21:44:37 -0700 Subject: [PATCH] Make asn.1 date/time parsing more robust. These changes should be safe -- testing the failure cases proves a bit difficult at the moment due to the fact that OpenSSL seems to fix the values that are present in the original ASN.1 before passing them on to us. It is thus not directly easily possible to trigger the error cases from scriptland. This also means that a lot of the new error cases we try to catch here can probably never happen. --- src/file_analysis/analyzer/x509/X509.cc | 63 +++++++++++++++++++------ src/file_analysis/analyzer/x509/X509.h | 4 +- 2 files changed, 50 insertions(+), 17 deletions(-) diff --git a/src/file_analysis/analyzer/x509/X509.cc b/src/file_analysis/analyzer/x509/X509.cc index 8c70597dca..9ba807c0bf 100644 --- a/src/file_analysis/analyzer/x509/X509.cc +++ b/src/file_analysis/analyzer/x509/X509.cc @@ -52,7 +52,7 @@ bool file_analysis::X509::EndOfFile() X509Val* cert_val = new X509Val(ssl_cert); // cert_val takes ownership of ssl_cert - RecordVal* cert_record = ParseCertificate(cert_val); // parse basic information into record + RecordVal* cert_record = ParseCertificate(cert_val, GetFile()->GetID().c_str()); // parse basic information into record // and send the record on to scriptland val_list* vl = new val_list(); @@ -84,7 +84,7 @@ bool file_analysis::X509::EndOfFile() return false; } -RecordVal* file_analysis::X509::ParseCertificate(X509Val* cert_val) +RecordVal* file_analysis::X509::ParseCertificate(X509Val* cert_val, const char* id_arg) { ::X509* ssl_cert = cert_val->GetCertificate(); @@ -131,8 +131,8 @@ RecordVal* file_analysis::X509::ParseCertificate(X509Val* cert_val) pX509Cert->Assign(3, new StringVal(len, buf)); BIO_free(bio); - pX509Cert->Assign(5, new Val(GetTimeFromAsn1(X509_get_notBefore(ssl_cert)), TYPE_TIME)); - pX509Cert->Assign(6, new Val(GetTimeFromAsn1(X509_get_notAfter(ssl_cert)), TYPE_TIME)); + pX509Cert->Assign(5, new Val(GetTimeFromAsn1(X509_get_notBefore(ssl_cert), id_arg), TYPE_TIME)); + pX509Cert->Assign(6, new Val(GetTimeFromAsn1(X509_get_notAfter(ssl_cert), id_arg), TYPE_TIME)); // we only read 255 bytes because byte 256 is always 0. // if the string is longer than 255, that will be our null-termination, @@ -515,68 +515,101 @@ unsigned int file_analysis::X509::KeyLength(EVP_PKEY *key) reporter->InternalError("cannot be reached"); } -double file_analysis::X509::GetTimeFromAsn1(const ASN1_TIME* atime) +double file_analysis::X509::GetTimeFromAsn1(const ASN1_TIME* atime, const char * id_arg) { + const char * id = id_arg; + if ( id_arg == 0 ) + id = ""; + time_t lResult = 0; char lBuffer[24]; char* pBuffer = lBuffer; size_t lTimeLength = atime->length; - char * pString = (char *) atime->data; + const char * pString = (const char *) atime->data; + + unsigned int remaining = 0; if ( atime->type == V_ASN1_UTCTIME ) { if ( lTimeLength < 11 || lTimeLength > 17 ) + { + reporter->Weird(fmt("Could not parse time in X509 certificate (fuid %s) -- UTCTime has wrong length", id)); return 0; + } memcpy(pBuffer, pString, 10); pBuffer += 10; pString += 10; + + remaining = lTimeLength-10; } - else + else // generalized time. We apparently ignore the YYYYMMDDHH case for now and assume we always have minutes and seconds { - if ( lTimeLength < 13 ) + if ( lTimeLength < 12 || lTimeLength > 23 ) + { + reporter->Weird(fmt("Could not parse time in X509 certificate (fuid %s) -- Generalized time has wrong length", id)); return 0; + } memcpy(pBuffer, pString, 12); pBuffer += 12; pString += 12; + + remaining = lTimeLength-12; } - if ((*pString == 'Z') || (*pString == '-') || (*pString == '+')) + if ( (remaining == 0) || (*pString == 'Z') || (*pString == '-') || (*pString == '+') ) { *(pBuffer++) = '0'; *(pBuffer++) = '0'; } - else + else if ( remaining >= 2 ) { *(pBuffer++) = *(pString++); *(pBuffer++) = *(pString++); + remaining -= 2; + // Skip any fractional seconds... - if (*pString == '.') + if ( (remaining > 0) && (*pString == '.') ) { pString++; - while ((*pString >= '0') && (*pString <= '9')) + remaining--; + while ( (remaining) > 0 && (*pString >= '0') && (*pString <= '9') ) pString++; + remaining--; } } + else + { + reporter->Weird(fmt("Could not parse time in X509 certificate (fuid %s) -- additional char after time", id)); + return 0; + } *(pBuffer++) = 'Z'; *(pBuffer++) = '\0'; time_t lSecondsFromUTC; - if ( *pString == 'Z' ) + if ( remaining == 0 || *pString == 'Z' ) lSecondsFromUTC = 0; - else { - if ((*pString != '+') && (pString[5] != '-')) + if ( remaining < 5 ) + { + reporter->Weird(fmt("Could not parse time in X509 certificate (fuid %s) -- not enough bytes remaining for offset", id)); return 0; + } + + if ((*pString != '+') && (*pString != '-')) + { + reporter->Weird(fmt("Could not parse time in X509 certificate (fuid %s) -- unknown offset type", id)); + return 0; + } lSecondsFromUTC = ((pString[1]-'0') * 10 + (pString[2]-'0')) * 60; lSecondsFromUTC += (pString[3]-'0') * 10 + (pString[4]-'0'); diff --git a/src/file_analysis/analyzer/x509/X509.h b/src/file_analysis/analyzer/x509/X509.h index bd4c8fc7a5..fa45ffd9e9 100644 --- a/src/file_analysis/analyzer/x509/X509.h +++ b/src/file_analysis/analyzer/x509/X509.h @@ -32,7 +32,7 @@ public: * @param Returns the new record value and passes ownership to * caller. */ - static RecordVal* ParseCertificate(X509Val* cert_val); + static RecordVal* ParseCertificate(X509Val* cert_val, const char* id_arg = 0); static file_analysis::Analyzer* Instantiate(RecordVal* args, File* file) { return new X509(args, file); } @@ -59,7 +59,7 @@ private: std::string cert_data; // Helpers for ParseCertificate. - static double GetTimeFromAsn1(const ASN1_TIME * atime); + static double GetTimeFromAsn1(const ASN1_TIME * atime, const char * id_arg = 0); static StringVal* KeyCurve(EVP_PKEY *key); static unsigned int KeyLength(EVP_PKEY *key); };