RFC8613: Add in OSCORE support #764

mrdeep1 · 2021-10-10T20:09:44Z

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 · 2021-10-11T17:43:23Z

Added support for building with native Visual Studio.

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

kkrentz · 2021-10-15T11:57:58Z

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 · 2021-10-15T13:06:59Z

@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 · 2021-10-15T19:47:46Z

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

mrdeep1 · 2021-10-25T10:35:52Z

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

mrdeep1 · 2021-10-25T10:43:54Z

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 · 2021-10-26T15:10:25Z

Added in RFC8613 Appendix C test vectors to unit tests.

mrdeep1 · 2021-11-02T12:44:55Z

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

kkrentz · 2021-12-14T10:26:06Z

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 · 2021-12-14T10:57:20Z

@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 · 2021-12-14T15:02:07Z

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

mrdeep1 · 2022-01-04T16:16:10Z

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

obgm

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

include/coap3/coap_pdu_internal.h

mrdeep1 · 2022-10-21T10:15:54Z

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.

obgm

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

obgm · 2022-11-04T14:01:58Z

include/coap3/coap_oscore_internal.h

+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);


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.

include/coap3/coap_session_internal.h

include/oscore/oscore_context.h

src/oscore/oscore_context.c

include/oscore/oscore_context.h

src/oscore/oscore_cbor.c

obgm · 2022-11-04T16:22:07Z

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).

obgm

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

obgm · 2023-01-12T23:41:09Z

include/oscore/oscore_cose.h

+
+#define COSE_KEY_EC2 2
+
+/* key, tag, and signature lengths  */


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

Comment deleted

obgm · 2023-01-12T23:42:20Z

include/oscore/oscore_cose.h

+#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


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

I believe that they are all UCd now.

obgm · 2023-01-12T23:48:50Z

include/oscore/oscore_cose.h

+#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


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

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

obgm · 2023-01-12T23:54:01Z

include/oscore/oscore_crypto.h

+                       uint8_t *okm,
+                       size_t okm_len);
+
+/* Return 0 if signing failure. Signatue length otherwise, signature length and


"Signatue" → "Signature"

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

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

obgm · 2023-01-12T23:57:24Z

src/coap_notls.c

+int
+coap_crypto_check_cipher_alg(cose_alg_t alg) {
+  return 0;
+  return get_cipher_alg(alg);


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

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

obgm · 2023-01-13T00:02:30Z

src/coap_oscore.c

+  }
+}
+
+#define MAX_IV_LEN 10 /* maximum length of iv buffer */


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

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.

obgm · 2023-01-13T00:09:09Z

src/oscore/oscore.c

+#include <stdbool.h>
+
+#define AAD_BUF_LEN 60 /* length of aad_buffer */
+#define MAX_IV_LEN  10 /* maximum length of iv buffer */


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.)

Deleted both #define as they are no longer relevant.

obgm · 2023-01-13T00:14:47Z

src/oscore/oscore_crypto.c

+    uint8_t zeroes_data[32];
+    coap_bin_const_t zeroes;
+
+    memset(zeroes_data, 0, 32);


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

Agreed and corrected.

obgm · 2023-01-13T00:30:28Z

doc/Makefile.am

@@ -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` ;\


The part ALL=C sort looks bogus.

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.

mrdeep1 force-pushed the oscore branch from 209c9a4 to 2bdf07b Compare October 11, 2021 17:40

mrdeep1 force-pushed the oscore branch from 2bdf07b to 77f4f1c Compare October 25, 2021 10:33

mrdeep1 force-pushed the oscore branch from 77f4f1c to 3adf450 Compare October 25, 2021 10:40

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

mrdeep1 force-pushed the oscore branch from 566835a to 5022b99 Compare November 2, 2021 12:44

mrdeep1 force-pushed the oscore branch 2 times, most recently from 9a8a4f6 to 6c2adeb Compare November 5, 2021 16:23

mrdeep1 mentioned this pull request Dec 9, 2021

RFC 9175: Add in support #786

Merged

mrdeep1 force-pushed the oscore branch from 6c2adeb to abd9404 Compare December 9, 2021 13:13

mrdeep1 mentioned this pull request Jan 3, 2022

Question: Two subscriptions to a single server showing some strange behavior #810

Closed

2 tasks

mrdeep1 force-pushed the oscore branch from abd9404 to f5ae0e3 Compare January 4, 2022 15:45

mrdeep1 force-pushed the oscore branch from f5ae0e3 to 1da057a Compare January 30, 2022 15:11

mrdeep1 force-pushed the oscore branch 4 times, most recently from ef216e9 to e84d314 Compare February 8, 2022 15:29

mrdeep1 mentioned this pull request Feb 15, 2022

Management of options from the proxy server #825

Closed

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

obgm reviewed Oct 21, 2022

View reviewed changes

examples/coap-client.c Outdated Show resolved Hide resolved

include/coap3/coap_pdu_internal.h Outdated Show resolved Hide resolved

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

obgm reviewed Nov 4, 2022

View reviewed changes

mrdeep1 force-pushed the oscore branch 3 times, most recently from 8b1d538 to 04f275f Compare November 7, 2022 10:08

mrdeep1 force-pushed the oscore branch 2 times, most recently from f770d63 to 53f1421 Compare November 30, 2022 16:31

mrdeep1 force-pushed the oscore branch 2 times, most recently from e8ce092 to a859760 Compare December 13, 2022 11:16

mrdeep1 force-pushed the oscore branch 2 times, most recently from 1a16f28 to 917aaed Compare December 20, 2022 22:09

mrdeep1 force-pushed the oscore branch 2 times, most recently from e802d08 to 709f4d7 Compare January 2, 2023 12:07

obgm approved these changes Jan 13, 2023

View reviewed changes

mrdeep1 force-pushed the oscore branch from 5ac6d57 to fda8954 Compare January 13, 2023 21:19

mrdeep1 added 2 commits January 13, 2023 21:33

OSCORE: Add in interopability tests

a8f2be2

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.

mrdeep1 force-pushed the oscore branch from fda8954 to a8f2be2 Compare January 13, 2023 21:41

mrdeep1 merged commit 7623e0f into obgm:develop Jan 14, 2023

mrdeep1 deleted the oscore branch January 14, 2023 10:27

mrdeep1 mentioned this pull request Jan 20, 2023

Add support for unix sockets for reverse proxies #958

Closed

RFC8613: Add in OSCORE support #764

RFC8613: Add in OSCORE support #764

Conversation

mrdeep1 commented Oct 10, 2021 • edited Loading

mrdeep1 commented Oct 11, 2021

kkrentz commented Oct 15, 2021

mrdeep1 commented Oct 15, 2021

kkrentz commented Oct 15, 2021

mrdeep1 commented Oct 25, 2021

mrdeep1 commented Oct 25, 2021

mrdeep1 commented Oct 26, 2021

mrdeep1 commented Nov 2, 2021 • edited Loading

kkrentz commented Dec 14, 2021

mrdeep1 commented Dec 14, 2021

kkrentz commented Dec 14, 2021

mrdeep1 commented Jan 4, 2022

obgm left a comment

Choose a reason for hiding this comment

mrdeep1 commented Oct 21, 2022

obgm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obgm commented Nov 4, 2022

obgm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdeep1 commented Oct 10, 2021 •

edited

Loading

mrdeep1 commented Nov 2, 2021 •

edited

Loading