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

Stack overflow when parsing x509 certificate #16135

Closed
z2-2z opened this issue Jan 30, 2025 · 2 comments
Closed

Stack overflow when parsing x509 certificate #16135

z2-2z opened this issue Jan 30, 2025 · 2 comments
Assignees
Labels

Comments

@z2-2z
Copy link
Contributor

z2-2z commented Jan 30, 2025

I did this

It seems that a stack-overflow has recently been introduced into the function getASN1Element() in lib/vtls/x509asn1.c:219:

static const char *getASN1Element(struct Curl_asn1Element *elem,
                                  const char *beg, const char *end)
{
  // --- snip ---

  else if(!(b &= 0x7F)) {
    /* Unspecified length. Since we have all the data, we can determine the
       effective length by skipping element until an end element is found. */
    if(!elem->constructed)
      return NULL;
    elem->beg = beg;
    while(beg < end && *beg) {
      beg = getASN1Element(&lelem, beg, end);
      if(!beg)
        return NULL;
    }

    // --- snip ---
}

A specifically crafted x509 certificate can cause a lot of recursions by repeatedly triggering beg = getASN1Element(&lelem, beg, end); that eventually overflow the stack area and cause an abnormal crash of curl or a client using libcurl. This works because getASN1Element() calls itself again in line 219 after consuming two specific bytes from the certificate. Since only two bytes of ASN1 data are needed for a new invocation of getASN1Element(), a relatively small, malicious certificate can already cause a DOS.

According to your vuln-disclosure-policy you don't consider this to be a security vulnerability, so I report it here as an issue. I did not make a PR because I am unsure how to best fix this.

Here you can find a concrete certificate that triggers the stack-overflow when directly fed into Curl_parseX509():
crash-3571ec1518df2d194685a4345a601d10d86c2ed0.txt

I expected the following

Graceful handling of the invalid certificate without crashing.

curl/libcurl version

commit 4f95f32

operating system

Linux <redacted> 6.12.6-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 19 Dec 2024 21:29:01 +0000 x86_64 GNU/Linu

@vszakats vszakats added the TLS label Jan 31, 2025
@icing icing self-assigned this Jan 31, 2025
@icing
Copy link
Contributor

icing commented Jan 31, 2025

@z2-2z thanks for the report and the samples. I propose to fix this via #16137, where I limited the recursion level to 16. I believe that should be enough to parse valid certificates and work on all architectures.

Would you care to review (once I get the CI kinds ironed out)? Thanks.

@z2-2z
Copy link
Contributor Author

z2-2z commented Jan 31, 2025

Would you care to review (once I get the CI kinds ironed out)? Thanks.

sure :)

@bagder bagder closed this as completed in 65fca12 Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants