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

RFC8613: Add in OSCORE support #764

Merged
merged 2 commits into from
Jan 14, 2023
Merged

RFC8613: Add in OSCORE support #764

merged 2 commits into from
Jan 14, 2023

Conversation

mrdeep1
Copy link
Collaborator

@mrdeep1 mrdeep1 commented Oct 10, 2021

Based on work done in https://gitlab.informatik.uni-bremen.de/obergman/libcoap

New directories with files
include/oscore
src/oscore

New files
src/coap_oscore.c
include/oscore/coap_oscore.h
include/oscore/coap_crypto_internal.h
include/oscore/coap_oscore_internal.h
man/coap-oscore-conf.txt.in
man/coap_oscore.txt.in

Supported by new option (enabled by default)
./configure --enable-oscore
cmake .. -DENABLE_OSCORE=ON

Requires a TLS library configured to do the OSCORE encryption and hashing.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Oct 11, 2021

Added support for building with native Visual Studio.

Mainly updates to variable types to remove Windows variable size mismatch warnings.

@kkrentz
Copy link
Contributor

kkrentz commented Oct 15, 2021

Interesting. I had just finished my own implementation of OSCORE because I needed it. Does your implementation cover the session key establishment that is described in Appendix B.2 of the RFC?

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Oct 15, 2021

@kkrentz Simple answer is no. kid context is supported, but the defined protocol is not yet implemented, which is on my TODO list. I'm currently working on getting draft-ietf-core-oscore-groupcomm to work.

Was your implementation based off libcoap?

@kkrentz
Copy link
Contributor

kkrentz commented Oct 15, 2021

I also took Peter van der Stok's code as a basis, but did not bother retaining his implementation of group communication.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Oct 25, 2021

Code update pushed with more rigours buffer checking and some reworking to better support the ongoing draft-ietf-core-oscore-groupcomm work.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Oct 25, 2021

Code updated with more rigorous checking of buffer sizes along with some re-organization for better support of the draft-ietf-core-oscore-groupcomm work.

@mrdeep1 mrdeep1 force-pushed the oscore branch 2 times, most recently from 6eab123 to 566835a Compare October 26, 2021 15:08
@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Oct 26, 2021

Added in RFC8613 Appendix C test vectors to unit tests.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Nov 2, 2021

Added in Appendix B.1.1 support. OSCORE specific API has been updated.

@kkrentz
Copy link
Contributor

kkrentz commented Dec 14, 2021

Does OSCORE specify what should happen when an RST is being received? Looking at your code, further retransmissions seem to be cancelled when an RST is being received. Since RSTs can (or even must?) be sent unauthenticated, an attacker can cancel the retransmission process, doesn't he?

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Dec 14, 2021

@kkrentz Interesting question

Does OSCORE specify what should happen when an RST is being received?

Not that I can find, even under the security considetations. However, RFC7252 https://datatracker.ietf.org/doc/html/rfc7252#section-11.4 does state

In principle, other kinds of spoofing can be detected by CoAP only in
case Confirmable message semantics is used, because of unexpected
Acknowledgement or Reset messages coming from the deceived endpoint.
But this imposes keeping track of the used Message IDs, which is not
always possible, and moreover detection becomes available usually
after the damage is already done. This kind of attack can be
prevented using security modes other than NoSec.

So, when using a security model other than NoSec, this kind of RST attack cannot occur. The RFC8613 libcoap implementation can provide (D)TLS wrappers to the OSCORE packets so security modes other than NoSec are supported.

Otherwise with NoSec, the RST can only terminate a retransmission, not the initial transmission.

@kkrentz
Copy link
Contributor

kkrentz commented Dec 14, 2021

I ended up ignoring RSTs. Of course, if you run DTLS in addition, you can process them.

@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Jan 4, 2022

Added in configurable support for RFC8613 Appendix B.1.2 and Appendix B.2

@mrdeep1 mrdeep1 force-pushed the oscore branch 4 times, most recently from 20582ae to eb1c9b3 Compare October 20, 2022 17:12
Copy link
Owner

@obgm obgm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still working through this one. Force-pushes do not make reviewer's life easier.
There is a bunch of non-COSE, non-OSCORE stuff in the COSE header files. I would like to remove anything that is not OSCORE from this PR. I can point out specific parts in the next review.

examples/coap-client.c Outdated Show resolved Hide resolved
include/coap3/coap_pdu_internal.h Outdated Show resolved Hide resolved
@mrdeep1
Copy link
Collaborator Author

mrdeep1 commented Oct 21, 2022

Still working through this one. Force-pushes do not make reviewer's life easier.

Apologies. Changes pushed this time as separate commits to be squashed in later.

There is a bunch of non-COSE, non-OSCORE stuff in the COSE header files. I would like to remove anything that is not OSCORE from this PR. I can point out specific parts in the next review.

Unnecessary stuff removed and changes pushed.

@mrdeep1 mrdeep1 force-pushed the oscore branch 3 times, most recently from 4c3fb25 to 2958ece Compare November 3, 2022 22:40
Copy link
Owner

@obgm obgm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments from my side. I have reviewed everything except for the the oscore_cose.c and the coap_*tls.c adaptations.

Once again, thanks to Jon and Peter for this huge contribution. This is some really amazing stuff.

include/coap3/coap_oscore_internal.h Outdated Show resolved Hide resolved
coap_pdu_t *coap_oscore_new_pdu_encrypted(coap_session_t *session,
coap_pdu_t *pdu,
coap_bin_const_t *kid_context,
int send_partial_iv);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you know, I like enums (or defines if combinable) for flags to enhance the code readability. This could be something like OSCORE_SEND_PARTIAL_IV vs. OSCORE_SEND_NO_IV.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

include/coap3/coap_session_internal.h Outdated Show resolved Hide resolved
include/oscore/oscore_context.h Outdated Show resolved Hide resolved
include/oscore/oscore_context.h Show resolved Hide resolved
src/oscore/oscore_context.c Outdated Show resolved Hide resolved
include/oscore/oscore_context.h Outdated Show resolved Hide resolved
src/oscore/oscore_cbor.c Outdated Show resolved Hide resolved
src/oscore/oscore_cbor.c Outdated Show resolved Hide resolved
src/oscore/oscore_cbor.c Outdated Show resolved Hide resolved
@obgm
Copy link
Owner

obgm commented Nov 4, 2022

One more thing: There is a comment on #ifdef HAVE_OSCORE vs. #if HAVE_OSCORE: Please note that there is currently a third variant: #if defined(HAVE_OSCORE).

@mrdeep1 mrdeep1 force-pushed the oscore branch 3 times, most recently from 8b1d538 to 04f275f Compare November 7, 2022 10:08
@mrdeep1 mrdeep1 force-pushed the oscore branch 2 times, most recently from f770d63 to 53f1421 Compare November 30, 2022 16:31
@mrdeep1 mrdeep1 force-pushed the oscore branch 2 times, most recently from e8ce092 to a859760 Compare December 13, 2022 11:16
@mrdeep1 mrdeep1 force-pushed the oscore branch 2 times, most recently from 1a16f28 to 917aaed Compare December 20, 2022 22:09
@mrdeep1 mrdeep1 force-pushed the oscore branch 2 times, most recently from e802d08 to 709f4d7 Compare January 2, 2023 12:07
Copy link
Owner

@obgm obgm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing the updates. I found some nits you might consider before (squashing and) merging this PR.


#define COSE_KEY_EC2 2

/* key, tag, and signature lengths */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is hard to understand. What exactly are the definitions here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment deleted

#define COSE_ALGORITHM_Ed25519_PRIV_KEY_LEN 32
#define COSE_ALGORITHM_Ed25519_PUB_KEY_LEN 32

#define COSE_algorithm_AES_CCM_64_64_128_KEY_LEN 16
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, I suggest using all uppercase for #defined constants.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that they are all UCd now.

#define COSE_ALGORITHM_Ed25519_PUB_KEY_LEN 32

#define COSE_algorithm_AES_CCM_64_64_128_KEY_LEN 16
#define COSE_algorithm_AES_CCM_64_64_128_IV_LEN 7
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not sure about the IV length here—IIRC this depends on L in the CCM blocks which is not defined here.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it is Nonce length. _IV_LEN changed to _NONCE_LEN in all the appropriate places.

uint8_t *okm,
size_t okm_len);

/* Return 0 if signing failure. Signatue length otherwise, signature length and
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Signatue" → "Signature"

As this is one of the few comments in this file, this could at least be made a Doxygen comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected. All functions in this file now have Doxygen comments.

src/coap_notls.c Outdated
int
coap_crypto_check_cipher_alg(cose_alg_t alg) {
return 0;
return get_cipher_alg(alg);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems redundant (also for the following *_hkdf_alg).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed all the stub functions - would be a separate PR for RIOT etc. support.

}
}

#define MAX_IV_LEN 10 /* maximum length of iv buffer */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The COSE definitions suggest that the (AEAD) IV can be up to 13 bytes. Is this a different type of IV?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it is used for "Partial IV" which is used for the sender sequence number (8 bytes). Re-ordered code variable location and size to make this clearer.

#include <stdbool.h>

#define AAD_BUF_LEN 60 /* length of aad_buffer */
#define MAX_IV_LEN 10 /* maximum length of iv buffer */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repeats a #definein another source file. I suggest moving this into the oscore_internal header for consistency. (The same may be true for AAD_BUF_LEN which is set to 200 elsewhere.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted both #define as they are no longer relevant.

uint8_t zeroes_data[32];
coap_bin_const_t zeroes;

memset(zeroes_data, 0, 32);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For safety reasons, I suggest using sizeof(zeroes_data) here and for zeroes.length.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed and corrected.

doc/Makefile.am Outdated
@@ -65,6 +65,7 @@ man-page-start: man-page-prepare

man-page-build: upg-page-build man-page-start
@MAN_FILES=`find $(top_srcdir)/man/ -type f -name "coap.txt.in" ; find $(top_srcdir)/man/ -type f -name "coap_*.in" | ALL=C sort ; find $(top_srcdir)/man/ -type f -name "coap-*.in" | LC_ALL=C sort` ;\
MAN3_FILES=`find $(top_srcdir)/man/ -type f -name "coap_*.in" | ALL=C sort | LC_ALL=C sort` ;\
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The part ALL=C sort looks bogus.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. All ALL=C corrected or removed as appropriate..

Based on work done in https://gitlab.informatik.uni-bremen.de/obergman/libcoap

New directories with files
 include/oscore
 src/oscore

New files
 include/coap3/coap_oscore.h
 include/coap3/coap_crypto_internal.h
 include/coap3/coap_oscore_internal.h
 man/coap-oscore-conf.txt.in
 man/coap_oscore.txt.in
 src/coap_oscore.c
 tests/test_oscore.c
 tests/test_oscore.h

Supported by new option (enabled by default)
 ./configure --enable-oscore
 cmake .. -DENABLE_OSCORE=ON

Requires a TLS library configured to do the OSCORE encryption and hashing.
See https://core-wg.github.io/oscore/test-spec5.html

Add in new test server oscore-interop-server to provide the necessary
functionality.

Add in script and configuration files to run the tests locally.
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

Successfully merging this pull request may close these issues.

3 participants