CHECK_SIZE in tls_parse_verify_tls13

[headscott]

I found a problem in tls_parse_verify_tls13: the second CHECK_SIZE always runs into TLS_NEED_MORE_DATA because, you just need 3 + size bytes, not 7 + size at this point. When I change it manually, it runs into another error when connecting to Google for example, because it tries to check for more zero bytes in pkcs_1_pss_decode. Is it an error of the code or did I do something wrong? Please fix and reply

[eduardsui]

Hello!

It's been a while since I've worked at the protocol level. I'm not sure that this is the problem, 7 + signature_size looks correct. Also, I use this code in production, I run a test with google.com and it works just fine. Can you share a minimal file that illustrates the problem?

    unsigned int size = buf[0] * 0x10000 + buf[1] * 0x100 + buf[2];
    unsigned short signature = ntohs(*(unsigned short *)&buf[3]);
    unsigned short signature_size = ntohs(*(unsigned short *)&buf[5]); // (bytes 6 and 7)

Thanks, Eduard

[headscott]

Hello!

Thank you for your response.

I believe the issue lies in the following line of code:

unsigned int size = buf[0] 0x10000 + buf[1] 0x100 + buf[2];

For instance, if buf_len is 500 and the calculated size is 503, it includes the bytes referred to by size (500) plus the additional bytes claimed by size in the packet (3 bytes). However, further in the code, you have the line:

CHECK_SIZE(7 + size, buf_len, TLS_NEED_MORE_DATA)

Here, you request 7 + size bytes, but since size already includes 3 bytes, the correct calculation should be 3 + size. Please let me know if my understanding is correct or if I misunderstood the code.

I'm uncertain if I can provide a precise example where my code encounters an error. I've made several changes, and there are significant differences between my code and yours. However, the issue I encountered appears to be related to the same section of code that I didn't modify. Nevertheless, I can't be entirely certain that the problem with 'pkcs_1_pss_decode' isn't somehow connected to my changes. Despite this, I'm confident that a problem lies with the CHECK_SIZE(7 + size, buf_len, TLS_NEED_MORE_DATA) (line 7428) – it seems to be incorrect from my perspective.

Thanks, Fabian

[eduardsui]

I think you are right. I think it should be something like this:

Instead of: CHECK_SIZE(7 + size, buf_len, TLS_NEED_MORE_DATA)

The correct check seems to be: CHECK_SIZE(7 + signature_size, buf_len, TLS_NEED_MORE_DATA)

E.

[headscott]

True, that's even better than CHECK_SIZE(3 + size, buf_len, TLS_NEED_MORE_DATA).

I would be very happy if you could try the code with google again and tell me if it runs into the same problem then in 'pkcs_1_pss_decode'. By the way, the signature used in my case was 0x0804 so rsa_pss_rsae_sha256.

F.

[headscott]

I already tried it now on your current version: I got a problem in tls_parse_certificate when using TLS_V13 in tlsclienthello.c for the context:

CHECK_SIZE(size_of_all_certificates, buf_len - res, TLS_NEED_MORE_DATA)

this returns the TLS_NEED_MORE_DATA error, because you forgot that there is the request context byte(s). So if size_of_all_certificates is 905 and buf_len is then 908, you have res = 4. CHECK_SIZE does this check: 905 > 908 - 4 which is correct, so the error is returned. You could just check for buf_len - 3 and additional you could check if there really is just one zero byte for the request context. After I did all these fixes, I also got the same problem in pkcs_1_pss_decode like in the other code version from your code, where I made some changes. So I am sure, if you do the CHECK_SIZE fix in tls_parse_certificate, you will run into the problem where pkcs_1_pss_decode checks for the zero Bytes in DB and also returns an error.

Thanks in advance. And I hope you could understand my explanation about the issue.

F.

[headscott]

If you don't understand my problem, feel free to ask. I would be happy if we could fix or if you could explain that if it's the correct behavior.

F.

[eduardsui]

Hello,

I’m traveling to the arctic circle untill around September 1st. I will take a look then.

E.

[headscott]

I hope your travel was good. If you still need more information from me about the problem I got, let me know

F.

[headscott]

I found out, that I forgot to set the sni for google. After I set it, the code did not failed anymore in pkcs_1_pss_decode, but now it failes in tls_parse_certificate, because the check for ignoring extensions seems to be wrong:

your solution was:

if((context->version == TLS_V13) ...){
  if(remaining >= 2){
      remaining -= 2;
      unsigned short size = nthos(*(unsigned short *)&buf[res2]);
      if(size && size >= remaining){
         res2 += size;
         remaining -= size;
      }
  }
}

what I did now, and It worked fine, probably its not perfect in every case but maybe it already helps:

if((context->version == TLS_V13) ...){
  if(remaining >= 2){
      remaining -= 2;
      unsigned short size = nthos(*(unsigned short *)&buf[res2]);
      if(size){
         res2 += size;
         remaining -= size;
      }
      res2 += 2;
  }
}

After that it failed in the tls_default_verify because there were no root_certificates set. I "fixed" this, by just returning no_error;

And now Google tries to use the 0x0403 signature algorithm (secp256r1 + sha256 ECDSA). So now it "fails" inside the ecc_verify_hash function in the part:

/* does v == r */
if(mp_cmp(v, r) == MP_EQ) {
  *stat = 1;
}

The check for validity seems not to work there. I am not sure if this is due to the leak of a root_certificate inside the context or whatever. I hope this helps you to fix it or to explain me what I did probably wrong

[headscott]

Hey,

I don't want to rush, but I just wanted to ask what the status is. I believe most, if not all, of the errors that caused these problems here int in #89 can be removed with the ideas mentioned in this issue and mentioned in #89 . At least it works fine for me now.

F.

[eduardsui]

Hello!

You are right about res2 += 2, thank you. I've modified it. Sorry for the delay, lots of things to do :). Also, I'm working at DTLS-SRTP and WebRTC RTCPeerConnection support for TLSe, it is possible to have some features broken these days. In a few days I will re-test the entire library.

Thank you, E.

[otsmr]

Hey @eduardsui :) I already sent you an email, but I just came across this issue, so I just want to add this here. In the same function is a missing length check of the size variable, leading to an integer overflow, which causes an EXC_BAD_ACCESS error.

The vulnerable code can be found in line 7282 where the size is parsed from user-controlled input and then used to subtract from the remaining variable. This leads to an integer overflow of the remaining variable. This will cause the variable res2 will get too big, leading to an EXC_BAD_ACCESS error when res2 is used to access data from buf.

I found the vulnerability with fuzzing, if you're interested, I could add the fuzzing setup with a PR. In the mail, I also sent you the input file of the fuzzer which causes the crash.