[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