Initial implementation of IEEE 802.15.4 security #15150

fabian18 · 2020-10-03T08:45:13Z

Contribution description

This PR aims to integrate initial IEEE 802.15.4 security support for RIOT.
The implementation waives to use a proper key store to mitigate complexity. Instead it always uses the key that has been set via
NETOPT_ENCRYPTION_KEY. So in terms of specification, the key is always implicitly known (IEEE802154_SCF_KEYMODE_IMPLICIT).
This is what I have been using as a reference.

Testing procedure

You can test the implementation with examples/gnrc_networking, if you add USEMODULE += ieee802154_security and try ping6. It is supposed to work with any 802.15.4 radio. If you have an at86rf2xx with SPI, you can additionally add USEMODULE += at86rf2xx_aes_spi, if you want to utilize the transceiver´s hardware crypto module.
I tested the implementation on two nucleo-f767zi with an at86rf233.

Issues/PRs references

Could be related to #14950

benpicco · 2020-10-03T12:14:08Z

Awesome, thank you!
Want to split out 5c2a1eb as a separate PR?
While technically an API change, we should be able to merge that quick.

I'll look into the other things soon.

sys/net/gnrc/netif/ieee802154/gnrc_netif_ieee802154.c

jia200x · 2020-10-05T08:58:46Z

I think code-wise makes a lot of sense. My only concern is the dependency from sys/ieee802154 to netdev, specially since we are currently migrating to the Radio HAL.
But I think integrating it to netdev would work for applications that still rely on that.

Also, do you think it could make sense to (optionally) expand the radio_ops of the Radio HAL to expose the primitives to use the internal crypto accelerator?
I think in the long run would be nice to have a separated crypto structure, but in the short run we could live with this in the radio descriptor.

what do you think?

fabian18 · 2020-10-05T09:10:36Z

Want to split out 5c2a1eb as a separate PR?

Did this real quick

fabian18 · 2020-10-05T09:40:20Z

Also, do you think it could make sense to (optionally) expand the radio_ops of the Radio HAL to expose the primitives to use the internal crypto accelerator?

That´s the hard question. I could try to add these and see what comes out. Or do you want to add the CAP´s ?

jia200x · 2020-10-05T09:43:11Z

I could try to add these and see what comes out.

That could be nice!

jia200x · 2020-10-05T10:05:01Z

btw, I think this could work already out of the box using netdev_ieee802154_submac! Since it depends on netdev_ieee802154.

That means, CC2420 and nrf802154 can be used with full CSMA-CA and retransmissions + security

benpicco · 2020-10-05T10:10:16Z

Also, do you think it could make sense to (optionally) expand the radio_ops of the Radio HAL to expose the primitives to use the internal crypto accelerator?

That´s the hard question. I could try to add these and see what comes out. Or do you want to add the CAP´s ?

I mean ideally we would have a general crypto API that is either backed by a hardware or a software implementation depending on the platform. But that is for another endeavor.

I like your idea that the radio driver can 'catch' those netops, so if the radio can handle crypto itself we already get this abstraction for free.

fabian18 · 2020-10-05T12:30:51Z

btw, I think this could work already out of the box using netdev_ieee802154_submac! Since it depends on netdev_ieee802154.

So as a first step, I should move my ieee802154_sec_context_t from netdev_ieee802154_t to netdev_ieee802154_submac_t?

jia200x · 2020-10-05T12:34:19Z

So as a first step, I should move my ieee802154_sec_context_t from netdev_ieee802154_t to netdev_ieee802154_submac_t?

It shouldn't be necessary, since netdev_ieee802154_submac_t already depends on netdev_ieee802154_t and most IEEE 802.15.4 drivers depends on netdev_ieee802154_t (but not netdev_ieee802154_submac_t). At the moment we remove netdev from the IEEE 802.15.4 drivers we can decide where to allocate that context (I would say the SubMAC is an option)

benpicco · 2020-10-18T21:07:45Z

Sorry for the long radio silence, I tried to do some interoperability testing, but wasn't very successful on that end.

for Linux I followed this guide using the nl802154_llsec branch of wpan-tools and made sure to enable the IEEE802154_NL802154_EXPERIMENTAL option in the kernel config. I would however only get a netlink: 'iwpan': attribute type 1 has an invalid length. when trying to configure the encryption key 😞
for Zephyr I flashed samples/net/sockets/echo_server with the NET_L2_IEEE802154_SECURITY option enabled onto a nrf52840dk_nrf52840. I also set CONFIG_NET_L2_IEEE802154_SECURITY_CRYPTO_DEV_NAME to CRYPTO_TC and enabled TinyCrypt since the driver for the nRF build-in crypto hardware was not supporting asyc operations which apparently the MAC layer needs.
With that I got no run-time errors, but still no encrypted traffic 😕

Anyway so with a samr21-xpro this is sending encrypted packets just fine.
I tried on an openmote-b with netdev_ieee802154_submac and cc2538_rf but that would not get me any encrypted traffic. It wouldn't decode the packets from samr21-xpro either.

avr-rss2 also has an at862xx radio, but the memory mapped variant that's not supported by the aes_spi driver just yet - it would still encrypt and decrypt packets.

The at86rf233 radio attached to an esp32 also worked fine with encryption.

I also noticed nodes can still receive unencrypted frames - that'll be convenient if we want to do some Diffie-Hellman key exchange and have per-node keys 😀

benpicco

Please squash & rebase before addressing the review comments.

sys/net/link_layer/ieee802154/security.c

benpicco

A so this is only hooked up to the at86rf2xx driver so far, but it could be hooked up the the sub-MAC and then catch all drivers that implement the radio HAL? (That would be for a later PR)

I'm happy with starting at a basic but solid foundation so we can get results, and make it more practical going forward, so a hard-coded key for all nodes and just supporting a single driver is fine.

But those encrypt and decrypt routines are a bit scary - manipulating buffers with long lists of offsets… if there is a bug in there I wouldn't want to search it.
Those mathematical variable names need explanations, otherwise they are just cryptic.

sys/include/net/ieee802154_security.h

sys/net/link_layer/ieee802154/security.c

sys/include/net/ieee802154_security.h

sys/net/link_layer/ieee802154/security.c

maribu

Before I again forget to submit my review, here is at least the chunk I looked at so far.

drivers/at86rf2xx/at86rf2xx.c

drivers/netdev/ieee802154.c

sys/Makefile.dep

sys/include/net/ieee802154_security.h

benpicco · 2020-12-03T19:31:21Z

tests/ieee802154_security will need a Makefile.ci

tests/ieee802154_security/Makefile.ci

tests/ieee802154_security/Makefile

sys/net/link_layer/ieee802154/security.c

benpicco · 2020-12-06T22:20:25Z

@miri64 have your comments been addressed?

benpicco · 2020-12-08T15:34:57Z

sys/net/link_layer/ieee802154/security.c

+    uint32_t frame_counter = byteorder_ntohl(
+                                byteorder_ltobl((le_uint32_t){aux->fc}));


Suggested change

uint32_t frame_counter = byteorder_ntohl(

byteorder_ltobl((le_uint32_t){aux->fc}));

uint32_t frame_counter = byteorder_ltohl((le_uint32_t){aux->fc});

should also work now.

benpicco · 2020-12-08T15:37:33Z

sys/net/link_layer/ieee802154/security.c

+    ahr->scf = _scf(ctx->security_level, ctx->key_id_mode);
+    /* If you look in the specification: Annex C,
+       integers values are in little endian */
+    ahr->fc = byteorder_btoll(byteorder_htonl(ctx->frame_counter)).u32;


Suggested change

ahr->fc = byteorder_btoll(byteorder_htonl(ctx->frame_counter)).u32;

ahr->fc = byteorder_htoll(ctx->frame_counter).u32;

feel free to squash directly

My comments were addressed

re-ACK to re-trigger CI

maribu · 2020-12-09T14:24:46Z

🎉 Good to have this in :-)

fabian18 · 2020-12-09T20:12:27Z

Hurray 🎊. Thank you @benpicco and @miri64

jia200x · 2020-12-10T10:02:42Z

congratz! One stop closer to the full MAC layer :)

fabian18 requested review from cgundogan, daniel-k, haukepetersen, jia200x, miri64, PeterKietzmann and smlng as code owners October 3, 2020 08:45

fabian18 mentioned this pull request Oct 3, 2020

ieee802154_submac: add initial support for common MAC sub layer #14950

Merged

benpicco added Area: network Area: Networking Area: security Area: Security-related libraries and subsystems Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Oct 3, 2020

waehlisch assigned PeterKietzmann Oct 3, 2020

benpicco assigned jia200x and benpicco Oct 3, 2020

jia200x reviewed Oct 5, 2020

View reviewed changes

sys/net/gnrc/netif/ieee802154/gnrc_netif_ieee802154.c Outdated Show resolved Hide resolved

fabian18 mentioned this pull request Oct 5, 2020

Add const to ciphers #15156

Merged

maribu self-requested a review October 6, 2020 08:20

benpicco reviewed Oct 18, 2020

View reviewed changes

sys/net/link_layer/ieee802154/security.c Outdated Show resolved Hide resolved

sys/net/link_layer/ieee802154/security.c Outdated Show resolved Hide resolved

benpicco reviewed Oct 18, 2020

View reviewed changes

maribu reviewed Oct 19, 2020

View reviewed changes

fabian18 force-pushed the ieee802154_security branch from 68fcc85 to 349c24c Compare December 3, 2020 19:11

fabian18 force-pushed the ieee802154_security branch from 349c24c to 3007905 Compare December 3, 2020 19:54

benpicco reviewed Dec 3, 2020

View reviewed changes

tests/ieee802154_security/Makefile.ci Show resolved Hide resolved

benpicco reviewed Dec 3, 2020

View reviewed changes

tests/ieee802154_security/Makefile Show resolved Hide resolved

fabian18 force-pushed the ieee802154_security branch from 3007905 to 233c3f6 Compare December 4, 2020 08:18

benpicco reviewed Dec 4, 2020

View reviewed changes

sys/net/link_layer/ieee802154/security.c Outdated Show resolved Hide resolved

fabian18 added 6 commits December 4, 2020 09:40

sys: Add IEEE 802.15.4 security

dc16c14

drivers/netdev: Add IEEE 802.15.4 security

1b5bcec

drivers/at86rf2xx: Use hardware security for IEEE 802.15.4 security

f074e8a

sys/include/net/ieee802154: Add sec ops to radio HAL

6123e87

sys/net/gnrc/netif/init_devs: IEEE802154_STACKSIZE_DEFAULT

7b5391b

tests/ieee802154_security: dummy test for CI process

9c45cf1

fabian18 force-pushed the ieee802154_security branch from 233c3f6 to 9c45cf1 Compare December 4, 2020 08:42

benpicco reviewed Dec 8, 2020

View reviewed changes

benpicco added Area: CI Area: Continuous Integration of RIOT components and removed Area: CI Area: Continuous Integration of RIOT components labels Dec 9, 2020

benpicco approved these changes Dec 9, 2020

View reviewed changes

benpicco merged commit 1477a34 into RIOT-OS:master Dec 9, 2020

benpicco requested a review from jue89 December 9, 2020 22:49

leandrolanzieri mentioned this pull request Feb 2, 2021

net/ieee802154_security: remove radio hal dependency and cleanup #15909

Merged

leandrolanzieri mentioned this pull request May 5, 2021

sys/crypto: Enable support for AES-192, AES-256 #16183

Merged

chrysn mentioned this pull request Sep 12, 2021

net/ieee802154_security doc: Shape security expectations #16841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of IEEE 802.15.4 security #15150

Initial implementation of IEEE 802.15.4 security #15150

fabian18 commented Oct 3, 2020

benpicco commented Oct 3, 2020 •

edited

Loading

jia200x commented Oct 5, 2020

fabian18 commented Oct 5, 2020 •

edited

Loading

fabian18 commented Oct 5, 2020

jia200x commented Oct 5, 2020

jia200x commented Oct 5, 2020

benpicco commented Oct 5, 2020 •

edited

Loading

fabian18 commented Oct 5, 2020

jia200x commented Oct 5, 2020 •

edited

Loading

benpicco commented Oct 18, 2020

benpicco left a comment •

edited

Loading

benpicco left a comment

maribu left a comment

benpicco commented Dec 3, 2020

benpicco commented Dec 6, 2020

benpicco Dec 8, 2020

benpicco Dec 8, 2020 •

edited

Loading

maribu commented Dec 9, 2020

fabian18 commented Dec 9, 2020

jia200x commented Dec 10, 2020

		uint32_t frame_counter = byteorder_ntohl(
		byteorder_ltobl((le_uint32_t){aux->fc}));

	uint32_t frame_counter = byteorder_ntohl(
	byteorder_ltobl((le_uint32_t){aux->fc}));
	uint32_t frame_counter = byteorder_ltohl((le_uint32_t){aux->fc});

	ahr->fc = byteorder_btoll(byteorder_htonl(ctx->frame_counter)).u32;
	ahr->fc = byteorder_htoll(ctx->frame_counter).u32;

Initial implementation of IEEE 802.15.4 security #15150

Initial implementation of IEEE 802.15.4 security #15150

Conversation

fabian18 commented Oct 3, 2020

Contribution description

Testing procedure

Issues/PRs references

benpicco commented Oct 3, 2020 • edited Loading

jia200x commented Oct 5, 2020

fabian18 commented Oct 5, 2020 • edited Loading

fabian18 commented Oct 5, 2020

jia200x commented Oct 5, 2020

jia200x commented Oct 5, 2020

benpicco commented Oct 5, 2020 • edited Loading

fabian18 commented Oct 5, 2020

jia200x commented Oct 5, 2020 • edited Loading

benpicco commented Oct 18, 2020

benpicco left a comment • edited Loading

Choose a reason for hiding this comment

benpicco left a comment

Choose a reason for hiding this comment

maribu left a comment

Choose a reason for hiding this comment

benpicco commented Dec 3, 2020

benpicco commented Dec 6, 2020

benpicco Dec 8, 2020

Choose a reason for hiding this comment

benpicco Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

maribu commented Dec 9, 2020

fabian18 commented Dec 9, 2020

jia200x commented Dec 10, 2020

benpicco commented Oct 3, 2020 •

edited

Loading

fabian18 commented Oct 5, 2020 •

edited

Loading

benpicco commented Oct 5, 2020 •

edited

Loading

jia200x commented Oct 5, 2020 •

edited

Loading

benpicco left a comment •

edited

Loading

benpicco Dec 8, 2020 •

edited

Loading