[riot-notifications] [RIOT-OS/RIOT] Initial implementation of IEEE 802.15.4 security (#15150)
benpicco
notifications at github.com
Mon Nov 2 23:32:34 CET 2020
@benpicco commented on this pull request.
This looks really good!
I only have minor comments.
Can you also add an implementation for `netdev_ieee802154_submac.c` based on `crypto/aes.h`?
I know I said one driver is fine for the start, but looking at your changes to `at86rf2xx.c` I got the feeling that this should be pretty straightforward and then I can test this on something other than the `samr21-xpro`.
It would also make for a good argument to implement the new radio HAL when by doing so you get link-layer security for free :wink:
> + * @brief Number of shifts to set/get security level bits
+ */
+#define IEEE802154_SCF_SECLEVEL_SHIFT (0)
+
+/**
+ * @brief Mask to get key mode bits
+ */
+#define IEEE802154_SCF_KEYMODE_MASK (0x18)
+
+/**
+ * @brief Number of shifts to set/get key mode bits
+ */
+#define IEEE802154_SCF_KEYMODE_SHIFT (3)
+
+/**
+ * @brief Security levels
Can you add a short description of what a MIC is here?
Is it a checksum of the entire frame, only the payload, just the header, …?
> +/**
+ * @brief Security levels
+ */
+typedef enum {
+ IEEE802154_SCF_SECLEVEL_NONE = 0x00, /**< no security */
+ IEEE802154_SCF_SECLEVEL_MIC32 = 0x01, /**< 32 bit MIC */
+ IEEE802154_SCF_SECLEVEL_MIC64 = 0x02, /**< 64 bit MIC */
+ IEEE802154_SCF_SECLEVEL_MIC128 = 0x03, /**< 128 bit MIC */
+ IEEE802154_SCF_SECLEVEL_ENC = 0x04, /**< encryption */
+ IEEE802154_SCF_SECLEVEL_ENC_MIC32 = 0x05, /**< enc. + 32 bit MIC */
+ IEEE802154_SCF_SECLEVEL_ENC_MIC64 = 0x06, /**< enc. + 64 bit MIC (mandatory) */
+ IEEE802154_SCF_SECLEVEL_ENC_MIC128 = 0x07 /**< enc. + 128 bit MIC */
+} ieee802154_scf_seclevel_t;
+
+/**
+ * @brief Key identifier modes
What are key source and key index?
> +typedef enum {
+ IEEE802154_SCF_KEYMODE_IMPLICIT = 0x00, /**< Key is determined implicitly */
+ IEEE802154_SCF_KEYMODE_INDEX = 0x01, /**< Key is determined from key index */
+ IEEE802154_SCF_KEYMODE_SHORT_INDEX = 0x02, /**< Key is determined from 4 byte key source and key index */
+ IEEE802154_SCF_KEYMODE_HW_INDEX = 0x03 /**< Key is determined from 8 byte key source and key index */
+} ieee802154_scr_keymode_t;
+
+/**
+ * @brief IEEE 802.15.4 security error codes
+ */
+typedef enum {
+ IEEE802154_SEC_OK, /**< Everything went fine */
+ IEEE802154_SEC_FRAME_COUNTER_OVERFLOW, /**< The requested operation would let the frame counter overflow */
+ IEEE802154_SEC_NO_KEY, /**< Could not find the key to perform a requested cipher operation */
+ IEEE802154_SEC_MAC_CHECK_FAILURE, /**< The computet MAC did not match */
+ IEEE802154_SEC_UNSUPORTED, /**< Unsupported oeration */
```suggestion
IEEE802154_SEC_UNSUPORTED, /**< Unsupported operation */
```
> +struct ieee802154_radio_cipher_ops;
+struct ieee802154_sec_context;
+
What are those for?
> + switch (sec_level) {
+ case IEEE802154_SCF_SECLEVEL_MIC32:
+ case IEEE802154_SCF_SECLEVEL_ENC_MIC32:
+ return 4;
+ case IEEE802154_SCF_SECLEVEL_MIC64:
+ case IEEE802154_SCF_SECLEVEL_ENC_MIC64:
+ return 8;
+ case IEEE802154_SCF_SECLEVEL_MIC128:
+ case IEEE802154_SCF_SECLEVEL_ENC_MIC128:
+ return 16;
+ default:
+ return 0;
+ }
+}
+
+static inline bool _req_mac(uint8_t sec_level)
```suggestion
/* frame is secured with checksum */
static inline bool _req_mac(uint8_t sec_level)
```
> +static inline bool _req_mac(uint8_t sec_level)
+{
+ switch (sec_level) {
+ case IEEE802154_SCF_SECLEVEL_MIC32:
+ case IEEE802154_SCF_SECLEVEL_MIC64:
+ case IEEE802154_SCF_SECLEVEL_MIC128:
+ case IEEE802154_SCF_SECLEVEL_ENC_MIC32:
+ case IEEE802154_SCF_SECLEVEL_ENC_MIC64:
+ case IEEE802154_SCF_SECLEVEL_ENC_MIC128:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static inline bool _req_encryption(uint8_t sec_level) {
```suggestion
/* frame is encrypted */
static inline bool _req_encryption(uint8_t sec_level) {
```
> +{
+ /* For non data frames (MAC commands, beacons) a and a_len would be larger.
+ ACKs are not encrypted. */
+ assert((*((uint8_t *)header)) & IEEE802154_FCF_TYPE_DATA);
+
+ if (ctx->security_level == IEEE802154_SCF_SECLEVEL_NONE) {
+ *mic_size = 0;
+ return IEEE802154_SEC_OK;
+ }
+ if (ctx->frame_counter == 0xFFFFFFFF) {
+ /* Letting the frame counter overflow is explicitly prohibited by the specification.
+ (see 9.4.2) */
+ return -IEEE802154_SEC_FRAME_COUNTER_OVERFLOW;
+ }
+ if (*header_size + payload_size >
+ (IEEE802154_FRAME_LEN_MAX
Could also be `IEEE802154G_FRAME_LEN_MAX` but I don't see how we could get that info here.
```suggestion
/* TODO: support 802.15.4g frame sizes*/
(IEEE802154_FRAME_LEN_MAX
```
@jia200x should we extend `netdev_ieee802154_t` with a `pdu` field or introduce some flag?
Currently this information is only stored inside GNRC.
> + _init_cbc_B0(&ccm, frame_counter, security_level, c_len, mac_size, src_address);
+ memset(tmp2, 0, sizeof(tmp2));
+ _cbc_next(ctx, tmp2, tmp1, (uint8_t *)&ccm, sizeof(ccm));
+ byteorder_htobebufs(tmp1, a_len);
+ off = _min(sizeof(tmp1) - sizeof(uint16_t), a_len);
+ memcpy(tmp1 + sizeof(uint16_t), a, off);
+ _cbc_next(ctx, tmp2, tmp1, tmp1, sizeof(uint16_t) + off);
+ for (;off < a_len;) {
+ off += _cbc_next(ctx, tmp2, tmp1, &a[off], a_len - off);
+ }
+ for (off = 0; off < c_len;) {
+ off += _cbc_next(ctx, tmp2, tmp1, &c[off], c_len - off);
+ }
Is there some opportunity to share code with the encrypt path?
It looks like the MIC calculation could be done in a shared helper function.
> + at86rf2xx_aes_key_write_encrypt(dev,
+ dev->netdev.sec_ctx.cipher.context.context);
Unintentional indentation?
> + (aes_block_t *)plain,
+ nblocks);
+
+}
+/**
+ * @brief Struct that contains IEEE 802.15.4 security operations
+ * which are implemented, using the transceiver´s hardware
+ * crypto capabilities
+ */
+static const ieee802154_radio_cipher_ops_t _at86rf2xx_cipher_ops = {
+ .set_key = _at86rf2xx_set_key,
+ .ecb = _at86rf2xx_ecb,
+ .cbc = _at86rf2xx_cbc
+};
+#endif /* IS_USED(MODULE_AT86RF2XX_COMMON_AES_SPI) && \
+ IS_USED(MODULE_IEEE802154_SECURITY) */
Where would the generic (software) implementation fit in?
e.g. can we check in `netdev_ieee802154_submac_init()` if `cipher_ops` is already set and if not plug in the generic cipher ops?
> + for (offset = 0, blk = 0; offset < c_len; blk++) {
+ offset += _cbc_next(ctx, block_buffer2,
+ block_buffer1, &c[offset],
+ c_len - offset);
+ }
+ if (memcmp(block_buffer2, *mic, mac_size)) {
+ return -IEEE802154_SEC_MAC_CHECK_FAILURE;
+ }
+ }
+ return IEEE802154_SEC_OK;
+}
+
+void ieee802154_sec_set_key(ieee802154_sec_dev_t *ctx,
+ const uint8_t *key, uint8_t key_size)
+{
+ /* NOP */
So this is an intentional NOP, not a TODO - maybe copy that comment then :wink:
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/15150#pullrequestreview-522007249
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20201102/faba3c3e/attachment-0001.htm>
More information about the notifications
mailing list