obgm / libcoap

A CoAP (RFC 7252) implementation in C
Other
786 stars 422 forks source link

Incorrect header passed to response_handler after finishing Replay Window scenario. #1392

Closed pawszw2 closed 3 months ago

pawszw2 commented 3 months ago

Environment

libcoap Configuration Summary

From important stuff: CONFIG_LIBCOAP_OSCORE_SUPPORT=y

Problem Description

Hi!

I've encountered a problem with header after libcoap updates a token in specific situation. There is a section of RFC for OSCORE that says there should be second message exchange if we sent a new OSCORE message after server reboot. Here everything looks fine on libcoap side, but after receiving a response for the second message, the token in it is replaced with the token from the first message. However, after this operation header doesn't get updated. It is not an issue when both tokens are the same length, but in other cases it can be. Later on, this header might be updated in send procedure, although there is also a case when pdu is passed to response handler before updating the header. Members of coap_pdu_t structure have valid values, but if we want to see raw CoAP message, there is a mismatch between token length declared in header and real token length.

I'm not sure if this behavior is intended or is it an oversight?

Token update takes place in coap_block.c:3884

I have a solution for this problem, which is to update header after updating a token. It can be done by replacing:

            /* need to put back original token into rcvd */
            coap_update_token(rcvd, p->app_token->length, p->app_token->s);
            rcvd->body_offset = saved_offset;

With:

            /* need to put back original token into rcvd */
            coap_update_token(rcvd, p->app_token->length, p->app_token->s);
            rcvd->body_offset = saved_offset;
            coap_pdu_encode_header(rcvd, session->proto);

But I don't know if it could cause some other problems.

Another idea is to update header always before calling response handler.

Expected Behavior

CoAP message held in memory should be up to date when passed to response handler.

If reading whole CoAP message from memory in response handler is invalid, probably there should be some note in coap_pdu_t documentation about it?

Actual Behavior

In my case (sending 8-byte token), CoAP header suggests, that there is a 7-byte long token in message, but in reality the token is 8-byte. Then, when I try to use external CoAP message parser, the last byte of token is read as option, leading to error in message parsing.

Steps to reproduce

This may be tricky and I don't think it would be helpful, but if you want I can try to describe it.

Debug Logs

Example flow:

  1. First PDU generated by client
    DEBG PDU to encrypt
    v:1 t:NON c:GET i:1234 {1122334455667788} [ Uri-Path:example, Request-Tag:... ]
  2. Response from server:
    DEBG Decrypted PDU
    v:1 t:NON c:4.01 i:1234 {1122334455667788} [ Echo:0x8877665544332211 ]
  3. Client response to that:
    DEBG PDU to encrypt
    v:1 t:NON c:GET i:1235 {02334455667789} [ Uri-Path:example,  Echo:0x8877665544332211, Request-Tag:... ]
  4. Final server response:
    DEBG Decrypted PDU
    v:1 t:NON c:2.05 i:1235 {02334455667789} [ ]
  5. PDU presented to app (after coap_update_token in coap_block.c:3884)
    DEBG PDU presented to app.
    v:1 t:NON c:2.05 i:1235 {1122334455667788} [ ]

    But in memory layout we have old header (printed pdu->used_size + pdu->hdr_size bytes, starting from pdu->token - pdu->hdr_size):

    57 45 12 35 11 22 33 44 55 66 77 88
mrdeep1 commented 3 months ago

As there are multiple types of CoAP PDU header format (i.e. Unreliable, Reliable, WebSockets), there would have been a general assumption that people accessing the PDU from within an application would only be interested in the content of the PDU using the functions in coap_pdu_access(3) and so this would be an oversight.

Furthermore, PDUs that contain a body of data that spans across multiple packets do not used the likes of pdu->used_size - which is why the coap_get_data_large(3) function is provided.

I cant immediately think of any side effects for your proposal (other than there being a bit of a processing overhead) - which would need to be applied to all instances where the PDU is getting the token updated before presenting the PDU to the application. I need to do some regression testing though.

pawszw2 commented 3 months ago

Thanks for quick reply, I'll look forward to updates on that.

mrdeep1 commented 3 months ago

Apologies in being slow to respond. This, what appears to be a simple change to coap_update_token(), is creating some anomalies which I am trying to tack down.

mrdeep1 commented 3 months ago

Please checkout #1401 to confirm that this now works for you.

pawszw2 commented 3 months ago

Thanks for that. Unfortunately I won't be able to check it till Monday.

pawszw2 commented 3 months ago

I've checked #1401 and it fixes my problem. Thanks for that!

mrdeep1 commented 3 months ago

Thanks for confirming.