drivers/xbee: add optional AES encryption support #2842

FrancescoErmini · 2015-04-21T12:08:54Z

To use AES encryption with Xbee module, change the value in the macro

define OPT_AES_ENCRYPTION (1)

and use the function:
xbee_encrypt_config(&dev,key_buf,1);

WARNING: if want to disable encryption, before set OPT_AES_ENCRYPTION (0) run xbee_encrypt_config(&dev,key_buf,0) to change the EE value in the Xbee to 1.

Kijewski · 2015-04-21T12:19:39Z

drivers/include/xbee.h

@@ -75,6 +75,11 @@
 #define XBEE_DEFAULT_CHANNEL        (17U)

 /**
+  * @brief  Set this flag to 1 allows the use of AES encryption in the Xbee Driver
+  */
+#define OPT_AES_ENCRYPTION          (0)


Please surround with #ifndef OPT_AES_ENCRYPTION

miri64 · 2015-04-21T12:34:08Z

I would honestly add this as a submodule xbee/aes_encrypt (e. g.) of xbee. The fields and behavior you need additionally you can guard with #define MODULE_XBEE_AES_ENCRYPT

miri64 · 2015-04-21T12:37:52Z

drivers/xbee/xbee.c

    DEBUG("xbee: Initialization successful\n");
    return 0;
 }

+int xbee_encrypt_config(xbee_t * dev, uint8_t * key_buf, unsigned int encryption_toggle)


Rather than adding a function for this you can use ng_netdev->driver->set() for that by adding NETCONF_OPT_ENCRYPTION and NETCONF_OPT_ENCRYPTION_KEY to ng_netconf.h

(regardless of that: it seems weird to me to have one function to set two options)

FrancescoErmini · 2015-04-21T17:30:24Z

@authmillenon I agree with your thoughts, let me know if I have understand:

in ng_netconf.h add NETCONF_OPT_ENCRYPTION and NETCON_OPT_ENCRYPTION_KEY to ng_netconf_opt_t
in xbee.c, inside the _set() function add:

  case NETCONF_OPT_ENCRYPTION
            return  _set_encryption(dev, (uint8_t *)value, value_len);
  case NETCONF_OPT_ENCRYPTION_KEY
            return  _set_encryption_key(dev, (uint8_t *)value, value_len);

in xbee.c at the end of xbee_init ()

#ifndef MODULE_XBEE_AES_ENCRYPT
_set_encryption(dev, dev->encrypt_toggle,1)
_set_encryption_key(dev, dev->aes_key,16)
#endif

is this right?
At this point, in main.c where do I assign the buffer key address?

miri64 · 2015-04-21T18:09:51Z

To the first 1. and 2. yes. At the second 1. no, you would copy the value from dev to dev. You would need to use some default value to set dev->encrypt_toggle and dev->aes_key. Also I would use ng_netconf_enable_t for NETCONF_OPT_ENCRYPTION for consistency with the rest of the API.

miri64 · 2015-04-21T18:12:42Z

drivers/xbee/xbee.c

+
+static int _set_aes_encryption_key(xbee_t *dev, size_t size) {
+
+        uint8_t cmd[18];


Inconsistent indentation in this function

FrancescoErmini · 2015-04-21T19:17:52Z

Ok, so second 1( I wanted to write 3...) should be:

tmp[0]=XBEE_DEFAULT_ENCRYPT_ENABLE;
_set_encryption(dev, tmp,1);
 uint8_t key_buf[16]={0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,
                           0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,};
_set_encryption_key(dev, key_buf,16)

with XBEE_DEFAULT_ENCRYPT_ENABLE a macro equal to 1.

As far as I understand the _set() function is called by ng_nomac_init thread in
res = dev->driver->set(dev, opt->opt, opt->data, opt->data_len);

I can't figure out on how to use ng_netconf_enable_t to puts encryption on/off...
( Sorry for all those questions,I'm not familiar with the network interface )

miri64 · 2015-04-21T20:19:19Z

As far as I understand the _set() function is called by ng_nomac_init thread in
res = dev->driver->set(dev, opt->opt, opt->data, opt->data_len);

Actually

res = dev->driver->set(dev, opt->opt, opt->data, opt->data_len);

can be called by everyone. Since the network stack itself is using netapi however NG_NETAPI_MSG_TYPE_SET messages are translated to set() calls by nomac (and most of every other future MAC layer). For use with ng_netconf_enable_t take the following example for your init code:

ng_netconf_enable_t tmp = NETCONF_ENABLE;
uint8_t key_buf[XBEE_ENCRYPTION_KEY_LEN]={
        0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,
        0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF
    };

_set_encryption(dev, &tmp, sizeof(ng_netconf_enable));
_set_encryption_key(dev, key_buf, sizeof(key_buf)

but actually I would ditch these function calls in the initialization function:

dev->encrypt = 1;
memcpy(dev->encrypt_key, key_buf, sizeof(key_buf)).

and leave the use of _set_encryption() and _set_encryption_key() for the demultiplexing of set()

FrancescoErmini · 2015-04-22T16:12:33Z

Sorry for the late reply,

Following your instruction (Thanks for that) I got it working.
Since my need is to pass the key buffer from the main.c I used:

uint8_t encrypt = (uint8_t) NETCONF_ENABLE;
static  uint8_t key_buf[16]={0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,
                 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,};
dev.driver->set((ng_netdev_t *)&dev,NETCONF_OPT_ENCRYPTION, &encrypt,1);
dev.driver->set((ng_netdev_t*)&dev,NETCONF_OPT_ENCRYPTION_KEY,key_buf,sizeof(key_buf));

With this method the two value in the device descriptor

    uint8_t * encrypt_key;                   
    unsigned int encrypt;         
} xbee_t;

are useless and can be removed.
In this way we can avoid the use of a default value because there aren't uninitialized pointer, right?

dev->encrypt = 1;
memcpy(dev->encrypt_key, key_buf, sizeof(key_buf)).

Let me know your thought, thanks Francesco

miri64 · 2015-04-22T16:43:18Z

Since my need is to pass the key buffer from the main.c I used:

uint8_t encrypt = (uint8_t) NETCONF_ENABLE;
static  uint8_t key_buf[16]={0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,
                 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,};
dev.driver->set((ng_netdev_t *)&dev,NETCONF_OPT_ENCRYPTION, &encrypt,1);
dev.driver->set((ng_netdev_t*)&dev,NETCONF_OPT_ENCRYPTION_KEY,key_buf,sizeof(key_buf));

I interpret this code as part of your main.c... If not say so.

With this method the two value in the device descriptor
    uint8_t * encrypt_key;                   
    unsigned int encrypt;         
} xbee_t;
are useless and can be removed.

Then were do you store the encryption status and key for the device?!? IMHO encrypt_key should not be a pointer but an array of size 16 and encrypt a bool (from <stdbool.h>).

FrancescoErmini · 2015-04-22T18:31:40Z

Well, to set encryption in the xbee you only need to write two commands EE1 and KY+(value of the key).
As I wrote above, in the main.c I write the values that I want to set. i.e

uint8_t encrypt = (uint8_t) NETCONF_ENABLE; // that is the 1 for the EE command
static  uint8_t key_buf[16]={0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,
                 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,}; //that is the value for the KY command

than, when I launch set() I pass those value:

dev.driver->set((ng_netdev_t *)&dev,NETCONF_OPT_ENCRYPTION, &encrypt,1);
dev.driver->set((ng_netdev_t*)&dev,NETCONF_OPT_ENCRYPTION_KEY,key_buf,sizeof(key_buf));

In this way, you don't need to store the encryption status and the key address inside the driver, once those value are set, they are stored inside the Xbee.

Then, If you need to have dev->encrypt_status and dev->encrypt_key to communicate those value at the network layer, we can set those value...of course... but What I wanted to say it's that with this approach It's not necessary.

miri64 · 2015-04-22T18:59:49Z

Ah, now I get it. For the key_buf I might understand your thinking for security purposes, but for get() of NETCONF_OPT_ENCRYPTION it also be faster to have it also stored in the dev struct.

uint8_t encrypt = (uint8_t) NETCONF_ENABLE; // that is the 1 for the EE command

I don't understand why you cast here...

[edit: make encrypt a ng_netconf_enable_t to begin with]

FrancescoErmini · 2015-04-22T21:16:12Z

I'll remove the casting.
About the NETCONF_OPT_ENCRYPTION , of course it's useful to have the current encrypt status stored in the dev struct, e.g

static int _set_encryption(xbee_t *dev, uint8_t *val, size_t len){
dev->encrypt = *val; // store the current status of encryption in the device descriptor
...

Please, take note that

dev.driver->set((ng_netdev_t *)&dev,NETCONF_OPT_ENCRYPTION, &encrypt,1);

it's also used by the user (in main.c) to turn off the encryption, i.e write EE 0. for that, It's sufficient that the user changes the value:
ng_netconf_enable_t encrypt = NETCONF_ENABLE; in ng_netconf_enable_t encrypt = NETCONF_DISABLE;

I know that this implementation doesn't sound good, because the function that set encryption are outside the driver, but it's the user that optionally decide to either turn on/off the encryption or to change the value of the key, and, I guess, the user 'behavior' are defined in main.c, not inside the driver.

Anyway, I appreciate the help you're giving me.

FrancescoErmini · 2015-04-26T17:35:05Z

@authmillenon Today I found the time to fix some merge conflicts and minor issues and now Travis is ok; if are you still interested about Xbee encryption option, then take a look at changes.

Thanks, F.

OlegHahm · 2015-05-12T15:50:48Z

This PR neems another rebase.

FrancescoErmini · 2015-05-14T17:01:05Z

@OlegHahm rebased. Sorry for the late reply, I thought that this PR was useless at this moment since the encryption is an optional. If you wish to merge this PR, maybe, some changes are required in order to follow the philosophy of the driver that uses shell commands to set optional value i.e NETCONF_OPT_CHANNEL. Should I modify some shell commands functions ?

FrancescoErmini · 2016-03-31T08:40:09Z

@Yonezawa-T2 Thanks for the explanation. I can proceed apply the patches

Yonezawa-T2 · 2016-03-31T09:35:40Z

Updated my branch https://github.com/Yonezawa-T2/RIOT/commits/drivers_xbee_encryption_3

FrancescoErmini · 2016-03-31T17:46:46Z

Thanks @Yonezawa-T2, will you close this PR and open a new PR from your branch?

Yonezawa-T2 · 2016-04-01T03:35:45Z

I will not open a new PR. If @FrancescoErmini cannot proceed, I will open a new PR but it will be late.

FrancescoErmini · 2016-04-01T11:51:56Z

Sorry @Yonezawa-T2 I misunderstood...I saw that in your branch you already did all patches that you told me to do and I thought that I wouldn't proceed anymore. The code in your branch looks fine....you add both the hex patch and the 95 MTU patch....does it miss something?
I can copy/paste the patches that you add on my branch and open a new PR if I have the permission to do that...

Yonezawa-T2 · 2016-04-04T01:15:26Z

The code in your branch looks fine....you add both the hex patch and the 95 MTU patch....does it miss something?

It should work fine but I'm not sure there are no another pitfalls.

I can copy/paste the patches that you add on my branch and open a new PR if I have the permission to do that...

Surely, please.

Yonezawa-T2 · 2016-04-04T01:19:04Z

sys/shell/commands/sc_netif.c

+            return 1;
+        }
+
+        key[i] = (uint8_t)((i1 << 4) + i2);


Sorry, this should be key[i / 2].

Yonezawa-T2 · 2016-04-19T02:00:55Z

Tested with RIOT and XCTU without any problems :). Other than the style fix, ACK. Please squash.

rejects key of invalid size
rejects key not in hexadecimal format
rejects key if xbee_encryption module is disabled
rejects 256 bit key
accepts 128 bit key
when encryption is disabled on the receiver:
- receives packet sent by txtsnd if encryption is disabled on the sender (even if a key is set)
- does not receive packet at all if encryption is enabled on the sender
- receives packet if encryption is disabled on the sender again
when encryption is enabled on the receiver:
- receives packet if encryption is enabled on sender and the keys matches
- receives packet with incorrect payload if encryption is enabled on the sender but the keys does not matches
- does not receive packet at all if encryption is disabled on the sender
- receives packet if encryption is enabled on the sender again and the keys matches

Yonezawa-T2 · 2016-04-19T08:21:33Z

drivers/xbee/xbee.c

+        cmd[1] = 'Y';
+
+       for (int i=0; i < 16; i++) { /* Append the key to the KY API AT command */
+           cmd[i + 2]=val[i];


Please add spaces around = sign in above two lines and squash immediately.

fixed, thanks!

Yonezawa-T2 · 2016-04-20T03:09:19Z

We have to update _netopt_strmap in sys/net/crosslayer/netopt/netopt.c.

Yonezawa-T2 · 2016-04-20T10:32:18Z

sys/net/crosslayer/netopt/netopt.c:57: trailing whitespace.
+    [NETOPT_ENCRYPTION_KEY]  = "NETOPT_ENCRYPTION_KEY",    
sys/net/crosslayer/netopt/netopt.c:57: trailing whitespace.
+    [NETOPT_ENCRYPTION_KEY]  = "NETOPT_ENCRYPTION_KEY",    
ERROR: This change introduces new whitespace errors

add encryption to drivers fix new line at the end of file add shell command for enable encryption and set encryption key on a given device modify _net_if_set_encrypt_key to support any key length modify _net_if_set_encrypt_key to support any key length of the key modify blank line fix ace before tab in indent fix ace before tab indent fix ace before tab indent an error fix trailing white space drivers/xbee: encryption support add encryption to drivers fix new line at the end of file add shell command for enable encryption and set encryption key on a given device modify _net_if_set_encrypt_key to support any key length modify _net_if_set_encrypt_key to support any key length of the key modify blank line fix ace before tab in indent fix ace before tab indent fix ace before tab indent an error fix trailing white space modify drivers/xbee/xbee.c fix white spaces on xbee.c Update xbee encryption driver white line at end xbee.h fix error fix sc_netif.c fix rebase master interactive drivers/xbee: encryption support add encryption to drivers fix new line at the end of file add shell command for enable encryption and set encryption key on a given device modify _net_if_set_encrypt_key to support any key length modify _net_if_set_encrypt_key to support any key length of the key modify blank line fix ace before tab in indent fix ace before tab indent fix ace before tab indent an error fix trailing white space drivers/xbee: encryption support add encryption to drivers fix new line at the end of file add shell command for enable encryption and set encryption key on a given device modify _net_if_set_encrypt_key to support any key length modify _net_if_set_encrypt_key to support any key length of the key modify blank line fix ace before tab in indent fix ace before tab indent fix ace before tab indent an error fix trailing white space modify drivers/xbee/xbee.c fix white spaces on xbee.c Update xbee encryption driver white line at end xbee.h fix error fix rebase conflict 4 fix same missing in patches changes fix ascii to hex index parser fix syntax rules fix syntax issue 2 add _netopt_strmap NETOPT_ENCRYPTION e NETOPT_ENCRYPTION_KEY fix trailng white spaces

Yonezawa-T2 · 2016-04-21T01:24:31Z

Murdock is happy. ACK and go.

OlegHahm · 2016-04-21T08:00:31Z

Cool, a small step closer to a secure IoT. 👍

Thanks for the nice contribution, @FrancescoErmini and thanks for the shepherding, @Yonezawa-T2!

FrancescoErmini · 2016-04-21T08:59:14Z

OK. Thanks to @Yonezawa-T2 and @authmillenon ! I hope that this PR would be a first step to start experimenting security on IEEE 802.15.4 WSN! The style fixes on xbee.c are horrible but effective. Maybe in a future, when a global strategy for the use of encryption will be done, those fixes will change.
@OlegHahm Talking about IoT security...next step in the University of Florence should be the use of micro CoAP in RIOT to manage keys on IEEE 802.15.4 nodes, but that's not my business!
Cheers Francesco.

OlegHahm assigned miri64 Apr 21, 2015

OlegHahm added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Apr 21, 2015

Kijewski reviewed Apr 21, 2015
View reviewed changes

miri64 reviewed Apr 21, 2015
View reviewed changes

FrancescoErmini force-pushed the drivers_xbee_encryption branch 5 times, most recently from 0b67857 to 0e12d75 Compare April 26, 2015 09:47

FrancescoErmini force-pushed the drivers_xbee_encryption branch 2 times, most recently from 0d8ddf4 to 0d134b7 Compare May 14, 2015 13:20

FrancescoErmini force-pushed the drivers_xbee_encryption branch from 0d134b7 to 6034fdb Compare May 16, 2015 19:41

FrancescoErmini closed this Mar 31, 2016

FrancescoErmini reopened this Mar 31, 2016

FrancescoErmini force-pushed the drivers_xbee_encryption branch 3 times, most recently from cec455d to f068544 Compare April 2, 2016 14:35

Yonezawa-T2 reviewed Apr 4, 2016
View reviewed changes

sys/shell/commands/sc_netif.c

return 1;

}

key[i] = (uint8_t)((i1 << 4) + i2);

Copy link

Contributor

Yonezawa-T2 Apr 4, 2016

Choose a reason for hiding this comment

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

Sorry, this should be key[i / 2].

FrancescoErmini force-pushed the drivers_xbee_encryption branch from 310167c to b48305f Compare April 19, 2016 07:56

Yonezawa-T2 reviewed Apr 19, 2016
View reviewed changes

Yonezawa-T2 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 19, 2016

FrancescoErmini force-pushed the drivers_xbee_encryption branch from d1de7b9 to 0c12db0 Compare April 19, 2016 11:03

kYc0o added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 19, 2016

FrancescoErmini force-pushed the drivers_xbee_encryption branch from c3d3743 to ff4473e Compare April 20, 2016 10:14

FrancescoErmini force-pushed the drivers_xbee_encryption branch from bdf4037 to 9fa4684 Compare April 20, 2016 15:43

Yonezawa-T2 merged commit 3e70191 into RIOT-OS:master Apr 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/xbee: add optional AES encryption support #2842

drivers/xbee: add optional AES encryption support #2842

FrancescoErmini commented Apr 21, 2015

Kijewski Apr 21, 2015

miri64 commented Apr 21, 2015

miri64 Apr 21, 2015

miri64 Apr 21, 2015

FrancescoErmini commented Apr 21, 2015

miri64 commented Apr 21, 2015

miri64 Apr 21, 2015

FrancescoErmini commented Apr 21, 2015

miri64 commented Apr 21, 2015

FrancescoErmini commented Apr 22, 2015

miri64 commented Apr 22, 2015

FrancescoErmini commented Apr 22, 2015

miri64 commented Apr 22, 2015

FrancescoErmini commented Apr 22, 2015

FrancescoErmini commented Apr 26, 2015

OlegHahm commented May 12, 2015

FrancescoErmini commented May 14, 2015

FrancescoErmini commented Mar 31, 2016

Yonezawa-T2 commented Mar 31, 2016

FrancescoErmini commented Mar 31, 2016

Yonezawa-T2 commented Apr 1, 2016

FrancescoErmini commented Apr 1, 2016

Yonezawa-T2 commented Apr 4, 2016

Yonezawa-T2 Apr 4, 2016

Yonezawa-T2 commented Apr 19, 2016 •

edited

Loading

Yonezawa-T2 Apr 19, 2016

FrancescoErmini Apr 19, 2016

Yonezawa-T2 commented Apr 20, 2016

Yonezawa-T2 commented Apr 20, 2016

Yonezawa-T2 commented Apr 21, 2016

OlegHahm commented Apr 21, 2016

FrancescoErmini commented Apr 21, 2016


		static int _set_aes_encryption_key(xbee_t *dev, size_t size) {

		uint8_t cmd[18];

drivers/xbee: add optional AES encryption support #2842

drivers/xbee: add optional AES encryption support #2842

Conversation

FrancescoErmini commented Apr 21, 2015

define OPT_AES_ENCRYPTION (1)

Kijewski Apr 21, 2015

Choose a reason for hiding this comment

miri64 commented Apr 21, 2015

miri64 Apr 21, 2015

Choose a reason for hiding this comment

miri64 Apr 21, 2015

Choose a reason for hiding this comment

FrancescoErmini commented Apr 21, 2015

miri64 commented Apr 21, 2015

miri64 Apr 21, 2015

Choose a reason for hiding this comment

FrancescoErmini commented Apr 21, 2015

miri64 commented Apr 21, 2015

FrancescoErmini commented Apr 22, 2015

miri64 commented Apr 22, 2015

FrancescoErmini commented Apr 22, 2015

miri64 commented Apr 22, 2015

FrancescoErmini commented Apr 22, 2015

FrancescoErmini commented Apr 26, 2015

OlegHahm commented May 12, 2015

FrancescoErmini commented May 14, 2015

FrancescoErmini commented Mar 31, 2016

Yonezawa-T2 commented Mar 31, 2016

FrancescoErmini commented Mar 31, 2016

Yonezawa-T2 commented Apr 1, 2016

FrancescoErmini commented Apr 1, 2016

Yonezawa-T2 commented Apr 4, 2016

Yonezawa-T2 Apr 4, 2016

Choose a reason for hiding this comment

Yonezawa-T2 commented Apr 19, 2016 • edited Loading

Yonezawa-T2 Apr 19, 2016

Choose a reason for hiding this comment

FrancescoErmini Apr 19, 2016

Choose a reason for hiding this comment

Yonezawa-T2 commented Apr 20, 2016

Yonezawa-T2 commented Apr 20, 2016

Yonezawa-T2 commented Apr 21, 2016

OlegHahm commented Apr 21, 2016

FrancescoErmini commented Apr 21, 2016

Yonezawa-T2 commented Apr 19, 2016 •

edited

Loading