Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

signature verification fails due to failure in parsing date time #103

Closed
blacklion9279 opened this issue Feb 23, 2015 · 15 comments
Closed
Labels
bug Something isn't working compatibility Compatibility with existing software

Comments

@blacklion9279
Copy link

I am providing MimeContext by creating a class that inherits from SecureMimeContext. So, at some point, during signature verification MimeKit invokes secureMimeContext class and fails parsing date with following exception:

The DateTime represented by the string is not supported in calendar System.Globalization.GregorianCalendar.
at System.DateTimeParse.ParseExact(String s, String format, DateTimeFormatInfo dtfi, DateTimeStyles style)
at Org.BouncyCastle.Asn1.DerUtcTime.ParseDateString(String dateStr, String formatStr)
at MimeKit.Cryptography.SecureMimeContext.GetDigitalSignatures(CmsSignedDataParser parser)
at MimeKit.Cryptography.MultipartSigned.Verify(CryptographyContext ctx)

There is no specific pattern to replicate this issue. But when it happens the date format looks very different ASN.1 decoder.

The format would look like this when exception is thrown:
image

it will look like this when it's able to understand the date format:
image

Now, this issue is only noticed so far when composing emails from Apple Mail client. We could ignore this issue as a client issue however, it seems like all mail clients are able to handle this email just fine making me think, if we are missing a trick or two to handle different date formats.

Thanks!

@jstedfast
Copy link
Owner

Okay, I think I see the problem...

in SecureMimeContext.cs on line 905, the following line is where the exception gets thrown:

signature.CreationDate = signingTime.ToAdjustedDateTime ();

BouncyCastle's DerUTCTime.ToAdjustedDateTime() assumes that the date/time string is in the format yyyyMMddHHmmss'GMT'zzz, however when the exception is thrown, it looks like it might be in the format yyMMddHHmmss'GMT'zzz.

A possible workaround might be to change MimeKit's code to:

try {
    signature.CreationDate = signingTime.ToAdjustedDateTime ();
} catch {
    signature.CreationDate = signingTime.ToDateTime ();
}

Any chance you could test this theory and let me know if that works?

@jstedfast
Copy link
Owner

Does this work for you?

@blacklion9279
Copy link
Author

that results into same exception.

Error: System.FormatException: The DateTime represented by the string is not supported in calendar System.Globalization.GregorianCalendar.
at System.DateTimeParse.ParseExact(String s, String format, DateTimeFormatInfo dtfi, DateTimeStyles style)
at Org.BouncyCastle.Asn1.DerUtcTime.ParseDateString(String dateStr, String formatStr)
at MimeKit.Cryptography.SecureMimeContext.GetDigitalSignatures(CmsSignedDataParser parser)
at MimeKit.Cryptography.MultipartSigned.Verify(CryptographyContext ctx)

@jstedfast
Copy link
Owner

Ah, on closer inspection this is obvious :-\

Turns out the AdjustedTime string is just TimeString with 2 leading digits added based on the first digit in TimeString (to make the 2-digit year a 4-digit year).

The problem might actually be the "60" at the end, but I have no idea.

I'm not sure this can really be fixed inside of MimeKit other than to catch exceptions and then just... ignore them? That seems bad tho...

@jstedfast
Copy link
Owner

In Bouncy Castle's DerUTCTime.cs, try modifying the ParseDateString() method to this:

        private static DateTime ParseDateString(
            string  dateStr,
            string  formatStr)
        {
            int hour = 0, minute = 0, second = 0;
            int year = 0, month = 0, day = 0;
            int i = 0;

            while (i < dateStr.Length && i < formatStr.Length && formatStr[i] != '\'') {
                if (dateStr[i] < '0' || dateStr[i] > '9')
                    throw new FormatException ();

                int digit = dateStr[i] - '0';

                switch (formatStr[i]) {
                case "y": year = (year * 10) + digit; break;
                case 'M': month = (month * 10) + digit; break;
                case 'd': day = (day * 10) + digit; break;
                case 'H': hour = (hour * 10) + digit; break;
                case 'm': minute = (minute * 10) + digit; break;
                case 's': second = (second * 10) + digit; break;
                }

                i++;
            }

            minute += second / 60;
            second = second % 60;

            hour += minute / 60;
            minute = minute % 60;

            string fixedDateStr = string.Format ("{0}{1}{2}{3}{4}{5}{6}", year, month, day,
                hour, minute, second, dateStr.Substring (i));

            DateTime dt = DateTime.ParseExact(
                fixedDateStr,
                formatStr,
                DateTimeFormatInfo.InvariantInfo);

            return dt.ToUniversalTime();
        }

@blacklion9279
Copy link
Author

good catch! yeah i agree handling that in try/catch is not a good idea! I don't know how other mail clients like outlook are able to handle this kind of date. The email was sent from Apple mail client. The fix you suggested results into signature verification failure with following exception:

. Error: System.InvalidOperationException: invalid date string: String was not recognized as a valid DateTime.
at Org.BouncyCastle.Asn1.X509.Time.ToDateTime()
at Org.BouncyCastle.X509.X509Certificate.ToString()

@jstedfast
Copy link
Owner

oh, duh. because I need to make sure each value is 2 digits. d'oh!

@jstedfast
Copy link
Owner

This should hopefully work (just changed the string.Format() format string):

        private static DateTime ParseDateString(
            string  dateStr,
            string  formatStr)
        {
            int hour = 0, minute = 0, second = 0;
            int year = 0, month = 0, day = 0;
            int i = 0;

            while (i < dateStr.Length && i < formatStr.Length && formatStr[i] != '\'') {
                if (dateStr[i] < '0' || dateStr[i] > '9')
                    throw new FormatException ();

                int digit = dateStr[i] - '0';

                switch (formatStr[i]) {
                case 'y': year = (year * 10) + digit; break;
                case 'M': month = (month * 10) + digit; break;
                case 'd': day = (day * 10) + digit; break;
                case 'H': hour = (hour * 10) + digit; break;
                case 'm': minute = (minute * 10) + digit; break;
                case 's': second = (second * 10) + digit; break;
                }

                i++;
            }

            minute += second / 60;
            second = second % 60;

            hour += minute / 60;
            minute = minute % 60;

            string fixedDateStr = string.Format ("{0:00}{1:00}{2:00}{3:00}{4:00}{5:00}{6}", year, month, day,
                hour, minute, second, dateStr.Substring (i));

            DateTime dt = DateTime.ParseExact(
                fixedDateStr,
                formatStr,
                DateTimeFormatInfo.InvariantInfo);

            return dt.ToUniversalTime();
        }

@jstedfast
Copy link
Owner

I couldn't get DateTime.ParseExact() to work and I hated the fact that I was already effectively parsing the entire DateTime values only to serialize and parse it again just to get the timezone adjustment, so I modified the above code a bit and added it to DateUtils.cs in MimeKit and updated the SecureMimeContext code to use it instead.

@jstedfast
Copy link
Owner

Okay, so, a seconds value of 60 is only allowed if it is a leap second... but according to Wikipedia:

The most recent one happened on June 30, 2012 at 23:59:60 UTC.[1] A leap second
will again be inserted at the end of June 30, 2015 at 23:59:60 UTC.[2]

The date encoded by Apple Mail is February 13, 2015 16:54:60 UTC.

It's been suggested that I don't accept it, but I think, for now, I'll continue using my parser which will accept it unless I discover that there are security implications in doing so.

@jstedfast
Copy link
Owner

Could you submit a bug report about this to Apple? They should really fix this bug.

@blacklion9279
Copy link
Author

Yes. We will submit a bug report to apple. Thanks for providing the fix. Your parser seems to work for those emails.

@blacklion9279
Copy link
Author

so, we submitted a bug report to apple.

I pulled in your latest commits and i wanted to validate if i am supposed to see following exception with latest changes.

87|2015_03_02_11:59:32.5551|Trace|A signature verification for an email failed. Email Server Id: 5:2. Error: MimeKit.Cryptography.DigitalSignatureVerifyException: Failed to verify digital signature: invalid date string: The DateTime represented by the string is not supported in calendar System.Globalization.GregorianCalendar. ---> System.InvalidOperationException: invalid date string: The DateTime represented by the string is not supported in calendar System.Globalization.GregorianCalendar.
at Org.BouncyCastle.Asn1.Cms.Time.get_Date()
at Org.BouncyCastle.Cms.SignerInformation.Verify(X509Certificate cert)
at MimeKit.Cryptography.SecureMimeDigitalSignature.Verify()
--- End of inner exception stack trace ---
at MimeKit.Cryptography.SecureMimeDigitalSignature.Verify()

@jstedfast
Copy link
Owner

This is a different location in the code, although a similar error. Unfortunately, in this case, it doesn't look like I can easily work around the problem in MimeKit since it is in Org.BouncyCastle.Cms.SignerInformation.Verify(X509Certificate cert).

Unfortunately, it doesn't appear that Bouncy Castle considers this to be a bug worth working around, either :-(

I suppose that technically, since the date is invalid, it really is an invalid signature.

What you could do is modify DerUtcTime.ParseDateString() to use the same approach as I used in MimeKit's DateUtils.TryParse() in this commit: 635478b

I'm not sure I want to implement this workaround in my fork of BouncyCastle because I'm hoping to get my changes merged into mainstream BouncyCastle once 1.8 stable goes out and I think that, based on the bug report I submitted to BouncyCastle, that it'll be rejected.

@blacklion9279
Copy link
Author

Thanks for explanation! yeah i don't think it makes sense to change that behavior!

@jstedfast jstedfast added the bug Something isn't working label Mar 10, 2015
@jstedfast jstedfast added the compatibility Compatibility with existing software label Mar 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compatibility Compatibility with existing software
Projects
None yet
Development

No branches or pull requests

2 participants