New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integration of Link Layer Security #557

Merged
merged 42 commits into from Oct 8, 2014

Conversation

Projects
None yet
8 participants
@kkrentz
Contributor

kkrentz commented Feb 4, 2014

This pull request addresses #526.

I added a new NETSTACK layer, called LLSEC, in between the MAC and the NETWORK layer for implementing link layer security. This change is in line with the current architecture and the handling of the LLSEC layer is similar to that of other NETSTACK layers. For example, link layer security can be disabled by configuring the nullsec driver.

The interface of LLSEC drivers comprises four functions bootstrap, send, on_frame_created, and input. I have successfully used this abstraction for implementing two LLSEC drivers, which provide authentication, confidentiality, as well as replay protection. One of them uses a network-wide key (Noncoresec) and the other one uses pairwise keys (Coresec). This pull request only contains Noncoresec, which is an 802.15.4-compliant LLSEC driver.

The employed AES driver is encapsulated as a preprocessor variable. I included a software-based, as well as a hardware-accelerated AES driver. Implementing other such drivers should be straightforward using these examples.

To make all this work, I extended the framer to support frames with security headers. Furthermore, I adapted contiki-sky-main.c to support link layer security. I also refactored the CC2420 driver.

What is left is to adapt the main files of other platform's main files to support link layer security. However, this task can be deferred since everything continues to work when leaving link layer security disabled.

Further information is here.

@adamdunkels

This comment has been minimized.

Show comment
Hide comment
@adamdunkels

adamdunkels Feb 4, 2014

Member

Very cool! This is definitely an area that needs fixing, and this looks like it does a very thorough job of doing so!

A couple of random questions/comments:

I'm not sure exactly what the bootstrap() function is intended to do. Do you have any example of how pairwise keys might be set up in a way that would delay the network initialization? (Doesn't have to be in this pull request, just an example to help understanding it would be great.)

Is the paper available somewhere outside the ACM digital library? ACM allows having copies on personal websites and similar.

Maybe struct llsec_driver should have a set_key() function? There seems to be a hard-coded key in the noncoresec.c file.

Also, travis seems to fail hard on the sky platform.

Member

adamdunkels commented Feb 4, 2014

Very cool! This is definitely an area that needs fixing, and this looks like it does a very thorough job of doing so!

A couple of random questions/comments:

I'm not sure exactly what the bootstrap() function is intended to do. Do you have any example of how pairwise keys might be set up in a way that would delay the network initialization? (Doesn't have to be in this pull request, just an example to help understanding it would be great.)

Is the paper available somewhere outside the ACM digital library? ACM allows having copies on personal websites and similar.

Maybe struct llsec_driver should have a set_key() function? There seems to be a hard-coded key in the noncoresec.c file.

Also, travis seems to fail hard on the sky platform.

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Feb 4, 2014

Contributor

The bootstrap function is e.g. used by the Coresec driver. There it is used to establish pairwise keys at startup. To avoid that RPL interferes with the pairwise key establishment, starting the network layer is delayed.

The paper can be obtained from here. The paper is actually a short version of my master's thesis.

Admitedly, noncoresec.c needs some improvement. The network-wide key should be configured, e.g., by using a CONF variable or by storing the network-wide key in external flash. In general, each LLSEC driver will have some strategy for key management, but that does not need to be part of the LLSEC API. Rather, I suggest that each LLSEC Driver defines its own way of configuring keys and other cryptographic material.

Regarding the failed build, I will try to figure out what is wrong.

Contributor

kkrentz commented Feb 4, 2014

The bootstrap function is e.g. used by the Coresec driver. There it is used to establish pairwise keys at startup. To avoid that RPL interferes with the pairwise key establishment, starting the network layer is delayed.

The paper can be obtained from here. The paper is actually a short version of my master's thesis.

Admitedly, noncoresec.c needs some improvement. The network-wide key should be configured, e.g., by using a CONF variable or by storing the network-wide key in external flash. In general, each LLSEC driver will have some strategy for key management, but that does not need to be part of the LLSEC API. Rather, I suggest that each LLSEC Driver defines its own way of configuring keys and other cryptographic material.

Regarding the failed build, I will try to figure out what is wrong.

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Feb 5, 2014

Contributor

Another use case of the bootstrap function is in the context of replay protection: A node can learn the clock differences with neighboring nodes by performing some initial message exchange within the bootstrap function. Subsequently, timestamps can be used instead of frame counters. Using timestamps for replay protection is pretty advantageous. Instead of sending 4-byte frame counters, only the last byte of a timestamp needs to be sent, which saves energy. (The IV must however include the full timestamp.) Furthermore, timestamps are more resilient to node resets than frame counters.

After all, the bootstrap function is a bit intrusive, but I hope its flexibility will become in handy ...

Contributor

kkrentz commented Feb 5, 2014

Another use case of the bootstrap function is in the context of replay protection: A node can learn the clock differences with neighboring nodes by performing some initial message exchange within the bootstrap function. Subsequently, timestamps can be used instead of frame counters. Using timestamps for replay protection is pretty advantageous. Instead of sending 4-byte frame counters, only the last byte of a timestamp needs to be sent, which saves energy. (The IV must however include the full timestamp.) Furthermore, timestamps are more resilient to node resets than frame counters.

After all, the bootstrap function is a bit intrusive, but I hope its flexibility will become in handy ...

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Feb 9, 2014

Contributor

Changes:

Contributor

kkrentz commented Feb 9, 2014

Changes:

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Feb 13, 2014

Contributor

It should be fine by now.

Contributor

kkrentz commented Feb 13, 2014

It should be fine by now.

@myeisha

This comment has been minimized.

Show comment
Hide comment
@myeisha

myeisha Feb 24, 2014

We also require link layer security, and I'm really glad to find this patchset. It does all of the things we need and then some, most of them even like I would have done them :)

I do see some issues, though:

  • the frame counter may wrap, which violates the standard. 802.15.4 explicitly states that an implementation must fail with an error if the frame counter ever wraps
  • the counters are not endianness-safe and will fail on big endian devices (not an issue for me, but noteworthy)

myeisha commented Feb 24, 2014

We also require link layer security, and I'm really glad to find this patchset. It does all of the things we need and then some, most of them even like I would have done them :)

I do see some issues, though:

  • the frame counter may wrap, which violates the standard. 802.15.4 explicitly states that an implementation must fail with an error if the frame counter ever wraps
  • the counters are not endianness-safe and will fail on big endian devices (not an issue for me, but noteworthy)
@jdede

This comment has been minimized.

Show comment
Hide comment
@jdede

jdede Mar 4, 2014

Contributor

Great job!
Works well for me. Exactly what I needed.

Contributor

jdede commented Mar 4, 2014

Great job!
Works well for me. Exactly what I needed.

@jdede

This comment has been minimized.

Show comment
Hide comment
@jdede

jdede Mar 5, 2014

Contributor

There seems to be a problem in the following constellation:

  • csma_driver
  • contikimac_driver
  • #define CONTIKIMAC_CONF_WITH_PHASE_OPTIMIZATION 1
    In this case, only broadcast messages seem to be transmitted.

If I deactivate csma_driver or PHASE_OPTIMIZATION, the packets are transmitted correctly.

This problem is reproducible in cooja (tested with examples/ipv6/rpl-udp/rpl-udp.csc). In case of active PHASE_OPTIMIZATION, no packets are transmitted.

Contributor

jdede commented Mar 5, 2014

There seems to be a problem in the following constellation:

  • csma_driver
  • contikimac_driver
  • #define CONTIKIMAC_CONF_WITH_PHASE_OPTIMIZATION 1
    In this case, only broadcast messages seem to be transmitted.

If I deactivate csma_driver or PHASE_OPTIMIZATION, the packets are transmitted correctly.

This problem is reproducible in cooja (tested with examples/ipv6/rpl-udp/rpl-udp.csc). In case of active PHASE_OPTIMIZATION, no packets are transmitted.

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Mar 5, 2014

Contributor

I am currently looking at the endianness problem. I could solve this by simply calling UIP_HTONL. However, I noticed that by default little endianness is used:

#ifdef UIP_CONF_BYTE_ORDER
#define UIP_BYTE_ORDER     (UIP_CONF_BYTE_ORDER)
#else /* UIP_CONF_BYTE_ORDER */
#define UIP_BYTE_ORDER     (UIP_LITTLE_ENDIAN)
#endif /* UIP_CONF_BYTE_ORDER */

Shouldn't it be:

#ifdef UIP_CONF_BYTE_ORDER
#define UIP_BYTE_ORDER     (UIP_CONF_BYTE_ORDER)
#else /* UIP_CONF_BYTE_ORDER */
#define UIP_BYTE_ORDER     (UIP_BIG_ENDIAN)
#endif /* UIP_CONF_BYTE_ORDER */
Contributor

kkrentz commented Mar 5, 2014

I am currently looking at the endianness problem. I could solve this by simply calling UIP_HTONL. However, I noticed that by default little endianness is used:

#ifdef UIP_CONF_BYTE_ORDER
#define UIP_BYTE_ORDER     (UIP_CONF_BYTE_ORDER)
#else /* UIP_CONF_BYTE_ORDER */
#define UIP_BYTE_ORDER     (UIP_LITTLE_ENDIAN)
#endif /* UIP_CONF_BYTE_ORDER */

Shouldn't it be:

#ifdef UIP_CONF_BYTE_ORDER
#define UIP_BYTE_ORDER     (UIP_CONF_BYTE_ORDER)
#else /* UIP_CONF_BYTE_ORDER */
#define UIP_BYTE_ORDER     (UIP_BIG_ENDIAN)
#endif /* UIP_CONF_BYTE_ORDER */
@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Mar 5, 2014

Contributor

I will see if I can reproduce your problem jdede. At least the border router + broadcast example work properly with this configuration:

#undef NETSTACK_CONF_RDC
#define NETSTACK_CONF_RDC                 contikimac_driver
#define WITH_PHASE_OPTIMIZATION           1
#undef NETSTACK_CONF_MAC
#define NETSTACK_CONF_MAC                 csma_driver
#undef QUEUEBUF_CONF_NUM
#define QUEUEBUF_CONF_NUM                 5
#undef PACKETBUF_CONF_HDR_SIZE
#define PACKETBUF_CONF_HDR_SIZE           28
#undef NETSTACK_CONF_LLSEC
#define NETSTACK_CONF_LLSEC               noncoresec_driver
#define LLSEC802154_CONF_AES              cc2420_aes_driver
#undef LLSEC802154_CONF_SECURITY_LEVEL
#define LLSEC802154_CONF_SECURITY_LEVEL   7
Contributor

kkrentz commented Mar 5, 2014

I will see if I can reproduce your problem jdede. At least the border router + broadcast example work properly with this configuration:

#undef NETSTACK_CONF_RDC
#define NETSTACK_CONF_RDC                 contikimac_driver
#define WITH_PHASE_OPTIMIZATION           1
#undef NETSTACK_CONF_MAC
#define NETSTACK_CONF_MAC                 csma_driver
#undef QUEUEBUF_CONF_NUM
#define QUEUEBUF_CONF_NUM                 5
#undef PACKETBUF_CONF_HDR_SIZE
#define PACKETBUF_CONF_HDR_SIZE           28
#undef NETSTACK_CONF_LLSEC
#define NETSTACK_CONF_LLSEC               noncoresec_driver
#define LLSEC802154_CONF_AES              cc2420_aes_driver
#undef LLSEC802154_CONF_SECURITY_LEVEL
#define LLSEC802154_CONF_SECURITY_LEVEL   7
@jdede

This comment has been minimized.

Show comment
Hide comment
@jdede

jdede Mar 6, 2014

Contributor

Okay, I am using the ti_aes_driver (also for sky in cooja). This seems to lead to the following problem in noncoresec.c, on_frame_created():

  • The packet is encrypted first time correctly
  • Due to the delay, the phase is missing, packet has to be sent again
  • Packet is stored in queue again (or left there)
  • Next time, packet is recognized as being encrypted. But in fact, the original (unencrypted) buffer is restored (including the wrong frame length).

Disabling the check for already secured frame numbers would destroy the timing: No packets are transmitted.

I just tested the behavior using the cc2420_aes_driver: This leads to a slightly better behavior (but still far away from optimal).

I use the following configuration:

#define LLSEC802154_CONF_SECURITY_LEVEL 7
#define NETSTACK_CONF_LLSEC noncoresec_driver
#define CONTIKIMAC_CONF_WITH_PHASE_OPTIMIZATION     1
#define NETSTACK_CONF_MAC     csma_driver

sky-platform, cooja-example: examples/ipv6/rpl-udp/rpl-udp.csc with the current checkout of your llsec-integration.
I think that the buffer and the buffer length (and also the attributes) should be synced with the queuebuffer after NETSTACK_LLSEC.on_frame_created() in contikimac.

Contributor

jdede commented Mar 6, 2014

Okay, I am using the ti_aes_driver (also for sky in cooja). This seems to lead to the following problem in noncoresec.c, on_frame_created():

  • The packet is encrypted first time correctly
  • Due to the delay, the phase is missing, packet has to be sent again
  • Packet is stored in queue again (or left there)
  • Next time, packet is recognized as being encrypted. But in fact, the original (unencrypted) buffer is restored (including the wrong frame length).

Disabling the check for already secured frame numbers would destroy the timing: No packets are transmitted.

I just tested the behavior using the cc2420_aes_driver: This leads to a slightly better behavior (but still far away from optimal).

I use the following configuration:

#define LLSEC802154_CONF_SECURITY_LEVEL 7
#define NETSTACK_CONF_LLSEC noncoresec_driver
#define CONTIKIMAC_CONF_WITH_PHASE_OPTIMIZATION     1
#define NETSTACK_CONF_MAC     csma_driver

sky-platform, cooja-example: examples/ipv6/rpl-udp/rpl-udp.csc with the current checkout of your llsec-integration.
I think that the buffer and the buffer length (and also the attributes) should be synced with the queuebuffer after NETSTACK_LLSEC.on_frame_created() in contikimac.

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Mar 8, 2014

Contributor

Hope that does the trick. Let me know.

Contributor

kkrentz commented Mar 8, 2014

Hope that does the trick. Let me know.

@jdede

This comment has been minimized.

Show comment
Hide comment
@jdede

jdede Mar 12, 2014

Contributor

Unfortunately not. It seems as every packet is encrypted: The PACKETBUF_ATTR_WAS_SECURED seems not to work as expected. Can you reproduce this problem on your machine in cooja? Or does the simulation run without any problems?

Contributor

jdede commented Mar 12, 2014

Unfortunately not. It seems as every packet is encrypted: The PACKETBUF_ATTR_WAS_SECURED seems not to work as expected. Can you reproduce this problem on your machine in cooja? Or does the simulation run without any problems?

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Mar 15, 2014

Contributor

The frame pending flag was causing trouble. This flag was sometimes set in frames that were previously created without this flag. Due to my check that skips securing frames that have already been secured, those frames were sent with old nonauthentic MICs and therefore dropped by the recipient(s). However, I found that I can skip the check altogether.

Contributor

kkrentz commented Mar 15, 2014

The frame pending flag was causing trouble. This flag was sometimes set in frames that were previously created without this flag. Due to my check that skips securing frames that have already been secured, those frames were sent with old nonauthentic MICs and therefore dropped by the recipient(s). However, I found that I can skip the check altogether.

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Mar 16, 2014

Contributor

To summarize the latest changes:

  • Fixed endianness
  • Hopefully fixed @jdede 's problem by securing all the time. (ContikiMAC recreates a frame in every round, as well. As a future work, we could devise some mechanism to avoid recreating and resecuring a frame in every round.)
  • Removed the superseded NETSTACK_ENCRYPT, NETSTACK_DECRYPT, and NETSTACK_ENCRYPTION_INIT macros
  • Obviated the need for the packetbuf attribute PACKETBUF_ATR_HDR_LEN by making the function packetbuf_hdrlen function applicable to incoming frames, too. This enabled me to leave the hdrreduce and hdralloc behavior unchanged.
Contributor

kkrentz commented Mar 16, 2014

To summarize the latest changes:

  • Fixed endianness
  • Hopefully fixed @jdede 's problem by securing all the time. (ContikiMAC recreates a frame in every round, as well. As a future work, we could devise some mechanism to avoid recreating and resecuring a frame in every round.)
  • Removed the superseded NETSTACK_ENCRYPT, NETSTACK_DECRYPT, and NETSTACK_ENCRYPTION_INIT macros
  • Obviated the need for the packetbuf attribute PACKETBUF_ATR_HDR_LEN by making the function packetbuf_hdrlen function applicable to incoming frames, too. This enabled me to leave the hdrreduce and hdralloc behavior unchanged.
@jdede

This comment has been minimized.

Show comment
Hide comment
@jdede

jdede Mar 17, 2014

Contributor

I tested the latest checkout again in cooja. I think the problem is the following:

  • #define LLSEC802154_CONF_AES cc2420_aes_driver seems to work (but not very reliable)
  • #define LLSEC802154_CONF_AES ti_aes_driver seems not to work

Again, I tested in cooja with sky nodes and the the rpl-udp example. I extended the contiki-conf.h by the following lines:

    #define LLSEC802154_CONF_SECURITY_LEVEL 7
    #define NETSTACK_CONF_LLSEC noncoresec_driver
    #define LLSEC802154_CONF_AES ti_aes_driver

The problem is the ContikiMAC: The encryption takes about 23 ms using ti_aes_driver and 7 ms using cc2420_aes_driver (both measured in a simulation with a sky node in cooja). When using the phase optimization this will destroy the timing: No (or only few) packets are transmitted.

As the phase optimization is disabled by default in contiki-default-conf.h (and all platforms), I suggest to merge this pull request. But it should mentioned somewhere that llsec leads to problems in combination with the phase optimization.

The solution of this problem could become complex. Perhaps the delay caused by the encryption have to be concerned by the phase optimization. But at the moment I am unsure how to do this in a generic way.

I think that the point with the phase optimization should be kept in mind as a TODO. The phase optimization is a good way to reduce the power consumption and increase the lifetime of battery powered nodes.

Contributor

jdede commented Mar 17, 2014

I tested the latest checkout again in cooja. I think the problem is the following:

  • #define LLSEC802154_CONF_AES cc2420_aes_driver seems to work (but not very reliable)
  • #define LLSEC802154_CONF_AES ti_aes_driver seems not to work

Again, I tested in cooja with sky nodes and the the rpl-udp example. I extended the contiki-conf.h by the following lines:

    #define LLSEC802154_CONF_SECURITY_LEVEL 7
    #define NETSTACK_CONF_LLSEC noncoresec_driver
    #define LLSEC802154_CONF_AES ti_aes_driver

The problem is the ContikiMAC: The encryption takes about 23 ms using ti_aes_driver and 7 ms using cc2420_aes_driver (both measured in a simulation with a sky node in cooja). When using the phase optimization this will destroy the timing: No (or only few) packets are transmitted.

As the phase optimization is disabled by default in contiki-default-conf.h (and all platforms), I suggest to merge this pull request. But it should mentioned somewhere that llsec leads to problems in combination with the phase optimization.

The solution of this problem could become complex. Perhaps the delay caused by the encryption have to be concerned by the phase optimization. But at the moment I am unsure how to do this in a generic way.

I think that the point with the phase optimization should be kept in mind as a TODO. The phase optimization is a good way to reduce the power consumption and increase the lifetime of battery powered nodes.

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Apr 21, 2014

Contributor

I figured out that WITH_PHASE_OPTIMIZATION doesn't do a thing. Only CONTIKIMAC_CONF_WITH_PHASE_OPTIMIZATION indeed enables phase optimization. With this knowledge I was able to observe your error ;). At the moment, phase optimization only works in conjunction with nullmac (not with CSMA). Either disabling phase optimization or using nullmac should make it work.

Other changes:

  • Encapsulated CCM
  • The hardware-accelerated AES-128 driver of the CC2420 is now used by default on Sky motes.
Contributor

kkrentz commented Apr 21, 2014

I figured out that WITH_PHASE_OPTIMIZATION doesn't do a thing. Only CONTIKIMAC_CONF_WITH_PHASE_OPTIMIZATION indeed enables phase optimization. With this knowledge I was able to observe your error ;). At the moment, phase optimization only works in conjunction with nullmac (not with CSMA). Either disabling phase optimization or using nullmac should make it work.

Other changes:

  • Encapsulated CCM
  • The hardware-accelerated AES-128 driver of the CC2420 is now used by default on Sky motes.
@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Apr 26, 2014

Contributor

Hope 2a76636 eventually overcomes the ContikiMAC incompatibility. Here is what I did:

  • I moved the ContikiMAC header creation and the appending of zeroes into a special FRAMER called contikimac_framer. #define NETSTACK_CONF_FRAMER contikimac_framer enables this FRAMER.
  • Having encapsulated the ContikiMAC frame creation particularities, I was able to create and secure frames in advance in ContikiMAC. That is, ContikiMAC no longer recreates and resecures frames in every round, but only once when outgoing frames are passed to ContikiMAC.

P.S.: An added benefit of creating the ContikiMAC header in the NETSTACK_FRAMER relates to #193. sicslowpan.c calls NETSTACK_FRAMER.create() in order to check how many header bytes will be allocated by the link layer. Now this call accounts for the ContikiMAC header bytes, too.

Contributor

kkrentz commented Apr 26, 2014

Hope 2a76636 eventually overcomes the ContikiMAC incompatibility. Here is what I did:

  • I moved the ContikiMAC header creation and the appending of zeroes into a special FRAMER called contikimac_framer. #define NETSTACK_CONF_FRAMER contikimac_framer enables this FRAMER.
  • Having encapsulated the ContikiMAC frame creation particularities, I was able to create and secure frames in advance in ContikiMAC. That is, ContikiMAC no longer recreates and resecures frames in every round, but only once when outgoing frames are passed to ContikiMAC.

P.S.: An added benefit of creating the ContikiMAC header in the NETSTACK_FRAMER relates to #193. sicslowpan.c calls NETSTACK_FRAMER.create() in order to check how many header bytes will be allocated by the link layer. Now this call accounts for the ContikiMAC header bytes, too.

@kkrentz kkrentz closed this Apr 26, 2014

@kkrentz kkrentz reopened this Apr 26, 2014

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Apr 29, 2014

Contributor

Added replay protection in 4f92b29 and 3784cd7. If any other changes are needed let me know.

Contributor

kkrentz commented Apr 29, 2014

Added replay protection in 4f92b29 and 3784cd7. If any other changes are needed let me know.

@cmorty

This comment has been minimized.

Show comment
Hide comment
@cmorty

cmorty Apr 29, 2014

Contributor

f4ade20 should probably go into a separate pull-request. The same might apply for e7eb15a, but I didn't look too deep into the code.

Contributor

cmorty commented Apr 29, 2014

f4ade20 should probably go into a separate pull-request. The same might apply for e7eb15a, but I didn't look too deep into the code.

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Apr 29, 2014

Contributor

4493e86 and 182f59d could be separated.

ce99c02, however, depends on LLSEC stuff.

Contributor

kkrentz commented Apr 29, 2014

4493e86 and 182f59d could be separated.

ce99c02, however, depends on LLSEC stuff.

@cmorty

This comment has been minimized.

Show comment
Hide comment
@cmorty

cmorty Apr 29, 2014

Contributor

Although this pull-request looks really good, and I'd give it a 👍 , I'm currently not familiar enough with the topic to make a decent review. Maybe you can find someone on the ML to review it. That makes it easier to persuade the core devs of merging it. 9b3d718 and f4ade20 get a big 👍 though. ;)

Contributor

cmorty commented Apr 29, 2014

Although this pull-request looks really good, and I'd give it a 👍 , I'm currently not familiar enough with the topic to make a decent review. Maybe you can find someone on the ML to review it. That makes it easier to persuade the core devs of merging it. 9b3d718 and f4ade20 get a big 👍 though. ;)

@jdede

This comment has been minimized.

Show comment
Hide comment
@jdede

jdede Apr 29, 2014

Contributor

I have just tested the current version of your branch. For me, it looks good. 👍
Again, I used the following configuration for the sky platform:

#define LLSEC802154_CONF_SECURITY_LEVEL 7
#define NETSTACK_CONF_LLSEC noncoresec_driver
#define CONTIKIMAC_CONF_WITH_PHASE_OPTIMIZATION 1

Furthermore I removed the part with the hardware AES to check if the timing issue I reported earlier is fixed. I tested again with examples/ipv6/rpl-udp/rpl-udp.csc and reduced the simulation timeout to 15 minutes. I got an PRR of 1 for the default settings of the client app.

Afterwards, I increased the payload by 90 bytes (leading to 113 - 115 bytes total UDP payload) to test the fragmentation. In this case, 3 of 419 packets got lost. This leads to a PRR of ~0.99. For a network consisting of 31 nodes, fragmentation, duty cycling and a send interval of 60 seconds this sounds okay for me.
With enabled hardware AES, the PRR for the long packets is 1!

Great work! A big 👍 from me !

Contributor

jdede commented Apr 29, 2014

I have just tested the current version of your branch. For me, it looks good. 👍
Again, I used the following configuration for the sky platform:

#define LLSEC802154_CONF_SECURITY_LEVEL 7
#define NETSTACK_CONF_LLSEC noncoresec_driver
#define CONTIKIMAC_CONF_WITH_PHASE_OPTIMIZATION 1

Furthermore I removed the part with the hardware AES to check if the timing issue I reported earlier is fixed. I tested again with examples/ipv6/rpl-udp/rpl-udp.csc and reduced the simulation timeout to 15 minutes. I got an PRR of 1 for the default settings of the client app.

Afterwards, I increased the payload by 90 bytes (leading to 113 - 115 bytes total UDP payload) to test the fragmentation. In this case, 3 of 419 packets got lost. This leads to a PRR of ~0.99. For a network consisting of 31 nodes, fragmentation, duty cycling and a send interval of 60 seconds this sounds okay for me.
With enabled hardware AES, the PRR for the long packets is 1!

Great work! A big 👍 from me !

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Apr 29, 2014

Contributor

glad to hear that

Contributor

kkrentz commented Apr 29, 2014

glad to hear that

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz May 1, 2014

Contributor
  • Fixed some bugs in contikimac_framer
  • Replay protection was over-engineered. I came up with a simpler version.
  • ce99c02 keeps sicslowpan independent from the configured llsec_driver.
Contributor

kkrentz commented May 1, 2014

  • Fixed some bugs in contikimac_framer
  • Replay protection was over-engineered. I came up with a simpler version.
  • ce99c02 keeps sicslowpan independent from the configured llsec_driver.
@jdede

This comment has been minimized.

Show comment
Hide comment
@jdede

jdede May 15, 2014

Contributor

I have some problems using the llsec with the rime stack. The 2 byte short addresses seem to be problematic as llsec uses 8 byte extended addresses. If I disable the llsec for rime or set the LINKADDR_CONF_SIZE to 8 byte, my test app (example-collect.csc) works fine.

There are at least three ways to solve this problem:

  • Disable llsec if 2 byte short addresses are used
  • Force 8 byte addresses if llsec is used
  • Extend llsec to work with 2 and 8 byte addresses
  • (Ignore this item as extended addresses are used in most cases?)

Are there any known caveats using rime with 8 byte extended addresses?

Contributor

jdede commented May 15, 2014

I have some problems using the llsec with the rime stack. The 2 byte short addresses seem to be problematic as llsec uses 8 byte extended addresses. If I disable the llsec for rime or set the LINKADDR_CONF_SIZE to 8 byte, my test app (example-collect.csc) works fine.

There are at least three ways to solve this problem:

  • Disable llsec if 2 byte short addresses are used
  • Force 8 byte addresses if llsec is used
  • Extend llsec to work with 2 and 8 byte addresses
  • (Ignore this item as extended addresses are used in most cases?)

Are there any known caveats using rime with 8 byte extended addresses?

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz May 16, 2014

Contributor

This is indeed an issue. The noncoresec_driver and, in particular, ccm.c do not currently support short addresses. This is mainly because 802.15.4 security mandates to use the following CCM* nonce:
"... || extended source address || frame Counter || ...". This way, each node can maintain its own frame counter but still be sure that no other node reuses its CCM* nonces. However, the receiver of a frame needs to obtain the sender's extended address in some way. Otherwise the receiver will not be able to recreate the CCM* nonce.

In the coresec_driver the solution is easy: nodes exchange their short addresses during key establishment so that they can later map short addresses on extended addresses.

A straightforward solution for the noncoresec_driver could like that: We could derive EUI 64s from short addresses like the 6LoWPAN adaption layer (0000:00FF:FE00:XXXX, where XXXX is the short address). Of course, short addresses must be unique in this case.

Contributor

kkrentz commented May 16, 2014

This is indeed an issue. The noncoresec_driver and, in particular, ccm.c do not currently support short addresses. This is mainly because 802.15.4 security mandates to use the following CCM* nonce:
"... || extended source address || frame Counter || ...". This way, each node can maintain its own frame counter but still be sure that no other node reuses its CCM* nonces. However, the receiver of a frame needs to obtain the sender's extended address in some way. Otherwise the receiver will not be able to recreate the CCM* nonce.

In the coresec_driver the solution is easy: nodes exchange their short addresses during key establishment so that they can later map short addresses on extended addresses.

A straightforward solution for the noncoresec_driver could like that: We could derive EUI 64s from short addresses like the 6LoWPAN adaption layer (0000:00FF:FE00:XXXX, where XXXX is the short address). Of course, short addresses must be unique in this case.

@nvt

This comment has been minimized.

Show comment
Hide comment
@nvt

nvt May 22, 2014

Member

Thanks for the nice contribution and for the subsequent effort in addressing the community's comments. I stumbled upon a license issue while doing an initial browsing through the source code.

core/lib/aes-128.c says this in a comment: "Wrapped AES-128 implementation from Texas Instruments." Can you clarify the license status of that implementation?

Member

nvt commented May 22, 2014

Thanks for the nice contribution and for the subsequent effort in addressing the community's comments. I stumbled upon a license issue while doing an initial browsing through the source code.

core/lib/aes-128.c says this in a comment: "Wrapped AES-128 implementation from Texas Instruments." Can you clarify the license status of that implementation?

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz May 22, 2014

Contributor

The code comes from here. This is the original header:

/**************************************************************
                        AES128 
Author:   Uli Kretzschmar
             MSP430 Systems
             Freising
AES software support for encryption and decryption
ECCN 5D002 TSU - Technology / Software Unrestricted
**************************************************************/
Contributor

kkrentz commented May 22, 2014

The code comes from here. This is the original header:

/**************************************************************
                        AES128 
Author:   Uli Kretzschmar
             MSP430 Systems
             Freising
AES software support for encryption and decryption
ECCN 5D002 TSU - Technology / Software Unrestricted
**************************************************************/
@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz May 25, 2014

Contributor

Added support for short addresses by applying the described workaround

Contributor

kkrentz commented May 25, 2014

Added support for short addresses by applying the described workaround

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz May 31, 2014

Contributor

I accidentally added some commits that are out of scope here ... now they are gone

Contributor

kkrentz commented May 31, 2014

I accidentally added some commits that are out of scope here ... now they are gone

@nvt

This comment has been minimized.

Show comment
Hide comment
@nvt

nvt Jun 2, 2014

Member

@kkrentz That header doesn't specify the copyright holder and license, which is a problem since all software included in Contiki must have a license that is compatible with the BSD license. Could we find a way to work around this issue without requiring too much effort (e.g., by replacing this part with an implementation that has a compatible license)?

Member

nvt commented Jun 2, 2014

@kkrentz That header doesn't specify the copyright holder and license, which is a problem since all software included in Contiki must have a license that is compatible with the BSD license. Could we find a way to work around this issue without requiring too much effort (e.g., by replacing this part with an implementation that has a compatible license)?

@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Jun 6, 2014

Contributor

Sooner or later, we will hopefully have a hardware-accelerated aes_128_driver for all mainstream transceivers. It would, however, be nice to have a software-based AES driver as a fallback solution. So if you know an AES implementation that I may drop in there, just tell me. For the time being, I could also clear the current software-based AES driver and write TODO :).

Contributor

kkrentz commented Jun 6, 2014

Sooner or later, we will hopefully have a hardware-accelerated aes_128_driver for all mainstream transceivers. It would, however, be nice to have a software-based AES driver as a fallback solution. So if you know an AES implementation that I may drop in there, just tell me. For the time being, I could also clear the current software-based AES driver and write TODO :).

@adamdunkels

This comment has been minimized.

Show comment
Hide comment
@adamdunkels

adamdunkels Jun 11, 2014

Member

The recent changes in the radio API (#617) caused some non-trivial merge problems in dev/cc2420.c that git cannot figure out how to automatically merge, so this needs a manual fix.

Member

adamdunkels commented Jun 11, 2014

The recent changes in the radio API (#617) caused some non-trivial merge problems in dev/cc2420.c that git cannot figure out how to automatically merge, so this needs a manual fix.

@nvt

This comment has been minimized.

Show comment
Hide comment
@nvt

nvt Jun 11, 2014

Member

@kkrentz TI seems to have released a version with a license similar to BSD's here: http://www.ti.com/tool/aes-128. Another candidate could be the one in axTLS: http://axtls.sourceforge.net

Member

nvt commented Jun 11, 2014

@kkrentz TI seems to have released a version with a license similar to BSD's here: http://www.ti.com/tool/aes-128. Another candidate could be the one in axTLS: http://axtls.sourceforge.net

kkrentz and others added some commits Apr 21, 2014

ContikiMAC: Create and parse ContikiMAC header in special framer; Exp…
…anded framer interface

to allow for creating and securing frames in advance; Create and secure frames in advance when sending bursts; Do neither recreate nor resecure frames that come from phase
@kkrentz

This comment has been minimized.

Show comment
Hide comment
@kkrentz

kkrentz Aug 6, 2014

Contributor

BTW 2cf7d98 also fixes #297

Contributor

kkrentz commented Aug 6, 2014

BTW 2cf7d98 also fixes #297

@adamdunkels

This comment has been minimized.

Show comment
Hide comment
@adamdunkels

adamdunkels Oct 8, 2014

Member

Time is nigh to finally merge this fantastic pull request! It brings some very important functionality and there has already been a lot of testing done and if things break because of it, we'll just have to fix it down the line.

Great work on this one everybody, particularly @kkrentz of course!

👍

Member

adamdunkels commented Oct 8, 2014

Time is nigh to finally merge this fantastic pull request! It brings some very important functionality and there has already been a lot of testing done and if things break because of it, we'll just have to fix it down the line.

Great work on this one everybody, particularly @kkrentz of course!

👍

adamdunkels added a commit that referenced this pull request Oct 8, 2014

Merge pull request #557 from kkrentz/llsec-integration
Integration of Link Layer Security

@adamdunkels adamdunkels merged commit d891d11 into contiki-os:master Oct 8, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@adamdunkels

This comment has been minimized.

Show comment
Hide comment
@adamdunkels

adamdunkels Oct 9, 2014

Member

I wrote a short blog post about this pull request here: http://contiki-os.blogspot.se/2014/10/a-big-step-for-contiki-built-in.html

Member

adamdunkels commented Oct 9, 2014

I wrote a short blog post about this pull request here: http://contiki-os.blogspot.se/2014/10/a-big-step-for-contiki-built-in.html

@laurentderu

This comment has been minimized.

Show comment
Hide comment
@laurentderu

laurentderu Oct 9, 2014

Member

Two small annoying issues : now there is two 18-… non-regression tests (18-compile-arm-ports and 18-llsec), also ccm.* files clash with ccm.* files from tinydtls (and most probably with other sec implementation). Wouldn't be better to prefix them with llsec- ?

Member

laurentderu commented Oct 9, 2014

Two small annoying issues : now there is two 18-… non-regression tests (18-compile-arm-ports and 18-llsec), also ccm.* files clash with ccm.* files from tinydtls (and most probably with other sec implementation). Wouldn't be better to prefix them with llsec- ?

@kkrentz kkrentz referenced this pull request Oct 9, 2014

Merged

llsec renaming #811

@kkrentz kkrentz deleted the kkrentz:llsec-integration branch Oct 9, 2014

@kkrentz kkrentz referenced this pull request Oct 10, 2014

Merged

ContikiMAC framer fix #816

@cmorty

This comment has been minimized.

Show comment
Hide comment
@cmorty

cmorty Nov 5, 2014

Contributor

@adamdunkels, @kkrentz Wow, while the llsec stuff might be nice, f513ef9 should be reverted. It breaks everything working with RSSI values on the CC2420. This includes the rssi-scanner in the examples directory and any other research project working with rssi values. I was lucky to spot this while rebasing.

Contributor

cmorty commented Nov 5, 2014

@adamdunkels, @kkrentz Wow, while the llsec stuff might be nice, f513ef9 should be reverted. It breaks everything working with RSSI values on the CC2420. This includes the rssi-scanner in the examples directory and any other research project working with rssi values. I was lucky to spot this while rebasing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment