7.44.0: asn1_output does not handle 4-digit year #427

Closed
srkemp opened this Issue Sep 11, 2015 · 11 comments

Projects

None yet

3 participants

@srkemp
srkemp commented Sep 11, 2015

Using curl 7.44.0
I saw the following (partial) output from running curl in verbose mode:

  •    start date: 2015-09-08 10:43:39 GMT
    
  •    expire date: 2015-09-08 10:43:39 GMT
    

Note the dates are the same, but this is not the case in the actual certificate.

Digging further, there is a bug in file lib/vtls/openssl.c function asn1_output(); it does not cater for a 4-digit year in the input field.

e.g. here is some output from the function itself showing the fields in the input "tm" and each character in the "tm->data" field. The first set is correct, and the second has a 4-digit year which causes an error.

The calling code (in get_cert_chain) does not check the return-code (to print the "not before" and "not after" dates in a certificate), hence the same buffer is printed for the "expire date"

Debug output:

// Correct output, for the "start date"
tm->length=13
tm->type=23
0: 0x31 '1'
1: 0x35 '5'
2: 0x30 '0'
3: 0x39 '9'
4: 0x30 '0'
5: 0x38 '8'
6: 0x31 '1'
7: 0x30 '0'
8: 0x34 '4'
9: 0x33 '3'
10: 0x33 '3'
11: 0x39 '9'
12: 0x5a 'Z'

  •    start date: 2015-09-08 10:43:39 GMT
    

// Incorrect, for the "expire date"
tm->length=15
tm->type=24
0: 0x32 '2'
1: 0x31 '1'
2: 0x31 '1'
3: 0x35 '5'
4: 0x30 '0'
5: 0x38 '8'
6: 0x31 '1'
7: 0x35 '5'
8: 0x31 '1'
9: 0x30 '0'
10: 0x34 '4'
11: 0x33 '3'
12: 0x33 '3'
13: 0x39 '9'
14: 0x5a 'Z'

  •    expire date: 2015-09-08 10:43:39 GMT
    
@bagder
Member
bagder commented Sep 11, 2015

Do you happen to know a public site that offers the longer version that I can try against?

@bagder bagder added the SSL/TLS label Sep 11, 2015
@srkemp
srkemp commented Sep 11, 2015

No, I do not know of any public site; we found this during internal testing of our app.

@bagder
Member
bagder commented Sep 12, 2015

I'm about to push a patch like this that I think should correct this problem and in fact clean up this function a little bit:

From 5c82c8a31192b5ec7983e4f26812ed274570c723 Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Sat, 12 Sep 2015 12:20:30 +0200
Subject: [PATCH] openssl: make the asn1 date parser handle 4-digit years

---
 lib/vtls/openssl.c | 67 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index 8600c61..7cc88a9 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -1013,49 +1013,62 @@ void Curl_ossl_close_all(struct SessionHandle *data)
 #else
   (void)data;
 #endif
 }

+/*
+ * Two example strings.
+ * '150908104339Z' (13 bytes)
+ * '20150908104339Z' (15 bytes)
+ */
+
 static int asn1_output(const ASN1_UTCTIME *tm,
                        char *buf,
                        size_t sizeofbuf)
 {
-  const char *asn1_string;
+  const char *str = (const char *)tm->data;
   int gmt=FALSE;
-  int i;
-  int year=0, month=0, day=0, hour=0, minute=0, second=0;
+  size_t i;
+  size_t len = tm->length;
+  int century=0, year=0;
+  int s=0; /* start index for the 2-digit year */

-  i=tm->length;
-  asn1_string=(const char *)tm->data;
+  *buf=0; /* in case of bad input */

-  if(i < 10)
+  if((len < 12) || (len > 15))
     return 1;
-  if(asn1_string[i-1] == 'Z')
+  if(len > 13) {
+    s = 2;
+  }
+  if(str[len-1] == 'Z') {
     gmt=TRUE;
-  for(i=0; i<10; i++)
-    if((asn1_string[i] > '9') || (asn1_string[i] < '0'))
+    len--;
+  }
+  for(i=0; i<len; i++)
+    if(!ISDIGIT(str[i]))
       return 2;

-  year= (asn1_string[0]-'0')*10+(asn1_string[1]-'0');
-  if(year < 50)
-    year+=100;
-
-  month= (asn1_string[2]-'0')*10+(asn1_string[3]-'0');
-  if((month > 12) || (month < 1))
-    return 3;
-
-  day= (asn1_string[4]-'0')*10+(asn1_string[5]-'0');
-  hour= (asn1_string[6]-'0')*10+(asn1_string[7]-'0');
-  minute=  (asn1_string[8]-'0')*10+(asn1_string[9]-'0');
-
-  if((asn1_string[10] >= '0') && (asn1_string[10] <= '9') &&
-     (asn1_string[11] >= '0') && (asn1_string[11] <= '9'))
-    second= (asn1_string[10]-'0')*10+(asn1_string[11]-'0');
+  year= (str[s]-'0')*10+(str[s+1]-'0');
+  if(s) {
+    century= (str[0]-'0')*10+(str[1]-'0');
+    year += century * 100;
+  }
+  else {
+    if(year < 80)
+      year += 2000;
+    else
+      year += 1900;
+  }

-  snprintf(buf, sizeofbuf,
-           "%04d-%02d-%02d %02d:%02d:%02d %s",
-           year+1900, month, day, hour, minute, second, (gmt?"GMT":""));
+  snprintf(buf, sizeofbuf, "%04d-%c%c-%c%c %c%c:%c%c:%c%c%s",
+           year,
+           str[s+2], str[s+3],
+           str[s+4], str[s+5],
+           str[s+6], str[s+7],
+           str[s+8], str[s+9],
+           str[s+10], str[s+11],
+           (gmt?" GMT":""));

   return 0;
 }

 /* ====================================================== */
-- 
2.5.1

@bagder bagder self-assigned this Sep 12, 2015
@ghedo
Member
ghedo commented Sep 12, 2015

@bagder why not use ASN1_TIME_print() instead of this whole mess of code? I can put together a patch if you are interested.

@bagder
Member
bagder commented Sep 12, 2015

Good question @ghedo! I think I once wrote this function because of some problem with that, but when I look back now I cannot find any explanation plus that we don't longer work with those old versions we had back then.

Yes, let's scrap this crappy function and go with ASN1_TIME_print(). And thanks a lot, I'll certainly appreciate a patch!

@ghedo
Member
ghedo commented Sep 12, 2015

FWIW, I think the whole get_cert_chain() function can be simplified a bit by using OpenSSL's BIO_s_mem thing.

@bagder
Member
bagder commented Sep 12, 2015

I like simplified! =)

@ghedo
Member
ghedo commented Sep 12, 2015

Soooo, I got carried away a bit and this is the result... "62 additions and 129 deletions" nice! However the output slightly changed (for example see the public key's BIGNUMs and the signature outputs). The original format could probably be restored, but more code is required.

There's still room for improvement though, for example asn1_output() is still used in servercert(), and X509V3_ext() can be improved as well. Also, I was thinking if we really need to do both Curl_ssl_push_certinfo() and infof() for every value, instead of looping the certinfo at the end and infofing that

@ghedo
Member
ghedo commented Sep 12, 2015

...or even drop the infof() completely.

@ghedo
Member
ghedo commented Sep 12, 2015

FWIW, by removing the infof calls I got to "49 insertions(+), 145 deletions(-)" (I still have to do servercert() and X509V3_ext()).

@ghedo
Member
ghedo commented Sep 12, 2015

Final report:

I can open a PR if needed.

@bagder bagder closed this in c00cec9 Sep 19, 2015
@jgsogo jgsogo added a commit to jgsogo/curl that referenced this issue Oct 19, 2015
@ghedo @jgsogo ghedo + jgsogo openssl: refactor certificate parsing to use OpenSSL memory BIO
Fixes #427
43d531e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment