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

Fix heap over-read when parsing x509 certificates #7536

Closed
wants to merge 5 commits into from
Closed

Fix heap over-read when parsing x509 certificates #7536

wants to merge 5 commits into from

Conversation

@z2-2z
Copy link
Contributor

@z2-2z z2-2z commented Aug 5, 2021

The function encodeDN in lib/x509asn1.c has a heap over-read.

Root cause:

static const char *octet2str(const char *beg, const char *end)
{
  size_t n = end - beg;
  char *buf = NULL;

  if(n <= (SIZE_T_MAX - 1) / 3) {
    buf = malloc(3 * n + 1);
    if(buf)
      for(n = 0; beg < end; n += 3)
        msnprintf(buf + n, 4, "%02x:", *(const unsigned char *) beg++);
  }
  return buf;
}

If beg == end, the returned chunk will be uninitialized.

The uninitialized chunk gets returned by ASN1tostr in encodeDN:

/* Generate value. */
str = ASN1tostr(&value, 0);
if(!str)
  return -1;
for(p3 = str; *p3; p3++) {
  if(l < buflen)
    buf[l] = *p3;
  l++;
}
free((char *) str);

If the chunk doesn't contain a NUL byte, p3 will go over the chunk boundaries and fills buf with some heap metadata.

This can be triggered in two different ways:

  1. Decode a zero-length octet string
  2. Decode a bit string of length 1

Luckily this isn't dangerous at all since the resulting leak is only visible to a client using libcurl.

@monnerat
Copy link
Contributor

@monnerat monnerat commented Aug 6, 2021

You're right on the ground, but I would not fix it by returning a NULL, as zero-length octet/bit strings are valid.

Maybe something as:

  static const char *octet2str(const char *beg, const char *end)
  {
    size_t n = end - beg;
    char *buf = NULL;

    if(n <= (SIZE_T_MAX - 1) / 3) {
      buf = malloc(3 * n + 1);
-     if(buf)
+     if(buf) {
+       *buf = '\0';
        for(n = 0; beg < end; n += 3)
          msnprintf(buf + n, 4, "%02x:", *(const unsigned char *) beg++);
+     }
    }
    return buf;
  }

@z2-2z
Copy link
Contributor Author

@z2-2z z2-2z commented Aug 6, 2021

Sure thing. I replaced malloc with calloc so that everything gets zeroed out:

--- a/lib/x509asn1.c
+++ b/lib/x509asn1.c
@@ -208,11 +208,8 @@ static const char *octet2str(const char *beg, const char *end)
   size_t n = end - beg;
   char *buf = NULL;
 
-  if(!n)
-    return NULL;
-
   if(n <= (SIZE_T_MAX - 1) / 3) {
-    buf = malloc(3 * n + 1);
+    buf = calloc(3 * n + 1, 1);
     if(buf)
       for(n = 0; beg < end; n += 3)
         msnprintf(buf + n, 4, "%02x:", *(const unsigned char *) beg++);

@jay jay added the SSL/TLS label Aug 7, 2021
@bagder
Copy link
Member

@bagder bagder commented Aug 8, 2021

How about this slight tweak, to

  1. don't assume the remaining buffer always fits 4 bytes, but use the remainder of the allocation
  2. use the actual output size to advance n
  if(n <= (SIZE_T_MAX - 1) / 3) {
    size_t len = 3 * n + 1;
    buf = malloc(len);
    if(buf) {
      *buf = '\0';
      for(n = 0; beg < end;) {
        size_t o = msnprintf(&buf[n], len, "%02x:",
                             *(const unsigned char *) beg++);
        n += o;
        len -= o;
      }
    }
  }

@monnerat
Copy link
Contributor

@monnerat monnerat commented Aug 8, 2021

How about this slight tweak, to...

I don't think it's very useful: each msnprintf call stores exactly 4 bytes: 2 hex digits, a colon and the null terminator unless it is broken. Anyway, if it complies, the result is the same.
in addition, we would be in a very bad condition in highly improbable cases where msnprintf returns -1 !

@bagder
Copy link
Member

@bagder bagder commented Aug 8, 2021

Sure, my version is just slightly more "defensive" and doesn't assume as much, like that the remaining allocation always fit 4 bytes. I won't insist, but I think that's a better way to write this code.

msnprintf() cannot return -1.

@monnerat
Copy link
Contributor

@monnerat monnerat commented Aug 8, 2021

... like that the remaining allocation always fit 4 bytes.

It does: the source of allocation size is the same as the loop count control and overflow cannot occur thanks to the SIZE_T_MAX test.
I also consider the current code involving as much constants as possible more readable, but that's a personal sentiment.

@monnerat
Copy link
Contributor

@monnerat monnerat commented Aug 9, 2021

my version is just slightly more "defensive" and doesn't assume as much

If it makes you feel safer, we could also use a dynbuf now that they exist.

@bagder
Copy link
Member

@bagder bagder commented Aug 9, 2021

we could also use a dynbuf now that they exist.

Oh right! Yes I would approve of that.

@monnerat
Copy link
Contributor

@monnerat monnerat commented Aug 9, 2021

Something like:

#include "dynbuf.h"
.
.
.
static const char *octet2str(const char *beg, const char *end)
{
  struct dynbuf buf;
  CURLcode result;

  Curl_dyn_init(&buf, 3 * CURL_ASN1_MAX + 1);
  for(result = Curl_dyn_addn(&buf, "", 0); !result && beg < end; beg++)
    result = Curl_dyn_addf(&buf, "%02x:", *(const unsigned char *) beg);
  return Curl_dyn_ptr(&buf);
}

@z2-2z : Do you want to create a commit for it or would you prefer I do it ?

@bagder
Copy link
Member

@bagder bagder commented Aug 9, 2021

The typecast can be removed too now, right? Like:

    result = Curl_dyn_addf(&buf, "%02x:", beg[0]);

@monnerat
Copy link
Contributor

@monnerat monnerat commented Aug 9, 2021

The typecast can be removed too now, right?

Don't you fear a possible sign extension?

@z2-2z
Copy link
Contributor Author

@z2-2z z2-2z commented Aug 12, 2021

The dynbuf code is now included in the pull request

monnerat
Copy link
Contributor

monnerat commented on be257d0 Aug 12, 2021

Without the cast, '\xb4' will be output as ffffffb4 ...

@z2-2z
Copy link
Contributor Author

@z2-2z z2-2z commented Aug 12, 2021

Without the cast, '\xb4' will be output as ffffffb4 ...

whoopsie, fixed now

bagder
bagder approved these changes Aug 14, 2021
@bagder
Copy link
Member

@bagder bagder commented Aug 16, 2021

Thanks!

@bagder bagder closed this in 5f3ca7f Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants