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

PSK identity length check - tinydtls binding #708

Open
boaks opened this issue Jun 22, 2021 · 4 comments
Open

PSK identity length check - tinydtls binding #708

boaks opened this issue Jun 22, 2021 · 4 comments

Comments

@boaks
Copy link

boaks commented Jun 22, 2021

libcoap: v4.3.0-rc4 (tinydtls binding):

Using a PSK identity with the exact maximum supported length fails with an empty identity in the client_key_exchange message. The check in coap_tinydtls - get_psk_info

      if (psk_info == NULL)
        return 0;
      if (psk_info->identity.length >= result_length)
        return 0;
      if (psk_info->key.length > sizeof(psk))
        return 0;

uses >=. Though tinydtls seems not to use a zero-terminated string representation in dtls_handshake_parameters_psk_t.identity, that compare may be relaxed to > to support identities up to the requested limit. Additionaliy, such an overrun seems to be silently ignored, may be adding a diagnostic message make it easier to understand the behavior.

      if (psk_info->identity.length > result_length) {
        coap_log(LOG_WARN, " psk_identity too large, %d bytes exceeds limit of %d bytes\n", psk_info->identity.length, result_length);
        return 0;
     }
@obgm
Copy link
Owner

obgm commented Jul 1, 2021

The issue here is that tinydtls treats the PSK identity as opaque data, following Section 4.2 of RFC 7925, whereas other TLS implementations such as OpenSSL and GnuTLS treat it as human-readable data that can be represented as zero-terminated string. As a result, liibcoap's wrapper code adds a zero-byte at the end of the PSK identity (see lines 259—262).
I will check to make this more consistent and ensure a sufficient buffer size.
The warning is a good idea and will be added.

@boaks
Copy link
Author

boaks commented Jul 1, 2021

My feeling:
The 0-termination is a implementation detail, specific to C.
I would prefer, if the other bindings take care of that, while the bindings, which not depend on the terminating 0, may use that byte.
AFAIK, that code is only used for tinyDTLS, so there it is possible, or?

@obgm
Copy link
Owner

obgm commented Jul 1, 2021

Yes, within coap_tinydtls.c, this is not an issue. I need to check if there is anything in the generic session-handling code that makes this assumption (it should not, but...).

@mrdeep1
Copy link
Collaborator

mrdeep1 commented Jul 1, 2021

In general, identity is held as a coap_bin_const_t within libcoap (I tried to standardize on this a while back) and when, for example it is presented to the app in validate_id_call_back() it is a coap_bin_const_t*. When setting up the PSK, the original *_psk() interface provided identity as a char*, but with the new *_psk2() interface it is passed in via the setup parameter as a coap_bin_const_t*. Calling *_psk() converts it to _*psk2() as appropriate. The interface between libcoap and (D)TLS libraries should be converting between char* and coap_bin_const_t* as appropriate if needed.

There was a debate about whether coap_string_t* should have ->s zero-byte terminated or not (making printf() functions easier to code/read and more bullet proof) which is why an extra byte is always added and made 0. The new binary create equivalents call coap_new_string() and so always get a zero terminated byte. So, there actually is no need to re-add a zero terminated byte.

I agree that a warning about oversize should be printed, an exact length match should be allowed, but then only return back the first part of identity if TinyDTLS does not provide sufficient buffer space rather than no identity at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants