Merge remote-tracking branch 'origin/topic/johanna/BIT-1465'

I did a bit more cleanup in GetTimeFromAsn1().

BIT-1465 #merged

* origin/topic/johanna/BIT-1465:
  Make asn.1 date/time parsing more robust.

src/file_analysis/analyzer/x509/X509.cc

@@ -52,7 +52,8 @@ 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
+	// parse basic information into record.
+	RecordVal* cert_record = ParseCertificate(cert_val, GetFile()->GetID().c_str());
 
 	// and send the record on to scriptland
 	val_list* vl = new val_list();
@@ -84,7 +85,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* fid)
 	{
 	::X509* ssl_cert = cert_val->GetCertificate();
 
@@ -131,8 +132,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(5, new Val(GetTimeFromAsn1(X509_get_notBefore(ssl_cert), fid), TYPE_TIME));
-	pX509Cert->Assign(6, new Val(GetTimeFromAsn1(X509_get_notAfter(ssl_cert)), TYPE_TIME));
+	pX509Cert->Assign(6, new Val(GetTimeFromAsn1(X509_get_notAfter(ssl_cert), fid), 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,54 +516,79 @@ 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* arg_fid)
 	{
+	const char *fid = arg_fid ? arg_fid : "";
 	time_t lResult = 0;
 
 	char lBuffer[24];
 	char* pBuffer = lBuffer;
 
-	size_t lTimeLength = atime->length;
+	const char *pString = (const char *) atime->data;
-	char * pString = (char *) atime->data;
+	unsigned int remaining = atime->length;
 
 	if ( atime->type == V_ASN1_UTCTIME )
 		{
-		if ( lTimeLength < 11 || lTimeLength > 17 )
+		if ( remaining < 11 || remaining > 17 )
+			{
+			reporter->Weird(fmt("Could not parse time in X509 certificate (fuid %s) -- UTCTime has wrong length", fid));
 			return 0;
+			}
 
 		memcpy(pBuffer, pString, 10);
 		pBuffer += 10;
 		pString += 10;
+		remaining -= 10;
 		}
 
 	else
 		{
-		if ( lTimeLength < 13 )
+		// generalized time. We apparently ignore the YYYYMMDDHH case
+		// for now and assume we always have minutes and seconds.
+
+		if ( remaining < 12 || remaining > 23 )
+			{
+			reporter->Weird(fmt("Could not parse time in X509 certificate (fuid %s) -- Generalized time has wrong length", fid));
 			return 0;
+			}
 
 		memcpy(pBuffer, pString, 12);
 		pBuffer += 12;
 		pString += 12;
+		remaining -= 12;
 		}
 
-	if ((*pString == 'Z') || (*pString == '-') || (*pString == '+'))
+	if ( (remaining == 0) || (*pString == 'Z') || (*pString == '-') || (*pString == '+') )
 		{
 		*(pBuffer++) = '0';
 		*(pBuffer++) = '0';
 		}
 
+	else if ( remaining >= 2 )
+		{
+		*(pBuffer++) = *(pString++);
+		*(pBuffer++) = *(pString++);
+
+		remaining -= 2;
+
+		// Skip any fractional seconds...
+		if ( (remaining > 0) && (*pString == '.') )
+			{
+			pString++;
+			remaining--;
+
+			while ( (remaining > 0) && (*pString >= '0') && (*pString <= '9') )
+				{
+				pString++;
+				remaining--;
+				}
+			}
+		}
+
 	else
 		{
-		*(pBuffer++) = *(pString++);
+		reporter->Weird(fmt("Could not parse time in X509 certificate (fuid %s) -- additional char after time", fid));
-		*(pBuffer++) = *(pString++);
+		return 0;
-
-		// Skip any fractional seconds...
-		if (*pString == '.')
-			{
-			pString++;
-			while ((*pString >= '0') && (*pString <= '9'))
-				pString++;
-			}
 		}
 
 	*(pBuffer++) = 'Z';
@@ -570,13 +596,21 @@ double file_analysis::X509::GetTimeFromAsn1(const ASN1_TIME* atime)
 
 	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", fid));
 			return 0;
+			}
+
+		if ((*pString != '+') && (*pString != '-'))
+			{
+			reporter->Weird(fmt("Could not parse time in X509 certificate (fuid %s) -- unknown offset type", fid));
+			return 0;
+			}
 
 		lSecondsFromUTC = ((pString[1] - '0') * 10 + (pString[2] - '0')) * 60;
 		lSecondsFromUTC += (pString[3] - '0') * 10 + (pString[4] - '0');
@@ -604,7 +638,7 @@ double file_analysis::X509::GetTimeFromAsn1(const ASN1_TIME* atime)
 
 	if ( lResult )
 		{
-		if ( 0 != lTime.tm_isdst )
+		if ( lTime.tm_isdst  != 0 )
 			lResult -= 3600;  // mktime may adjust for DST  (OS dependent)
 
 		lResult += lSecondsFromUTC;

src/file_analysis/analyzer/x509/X509.h

@@ -29,10 +29,13 @@ public:
 	 *
 	 * @param cert_val The certificate to converts.
 	 *
+	 * @param fid A file ID associated with the certificate, if any
+	 * (primarily for error reporting).
+	 *
 	 * @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* fid = 0);
 
 	static file_analysis::Analyzer* Instantiate(RecordVal* args, File* file)
 		{ return new X509(args, file); }
@@ -59,7 +62,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* fid);
 	static StringVal* KeyCurve(EVP_PKEY *key);
 	static unsigned int KeyLength(EVP_PKEY *key);
 };