[ieee802154] CCM* implementation and new symmetric encryption HIL #684

bbbert · 2017-11-14T16:54:23Z

Pull Request Overview

capsules::aes_ccm: CCM* implementation on top of new symmetric_encryption HIL
- handles all padding and data formatting for encryption and decryption
- interface assumes "a data" and "m data" as defined in 802.15.4 spec are adjacent in a static buffer
capsules::ieee802154
- updated MacDevice to use the above for link-layer security
capsules::test::aes_ccm
- runs CCM* test vectors

Testing Strategy

CCM* test vectors encrypt and decrypt correctly on an imix.

OK! (current_test=0, encrypting=true, tag_is_valid=true)
OK! (current_test=0, encrypting=false, tag_is_valid=true
OK! (current_test=1, encrypting=true, tag_is_valid=true)
OK! (current_test=1, encrypting=false, tag_is_valid=true
OK! (current_test=2, encrypting=true, tag_is_valid=true)
OK! (current_test=2, encrypting=false, tag_is_valid=true)

TODO or Help Wanted

Documentation Updated

Kernel: The relevant files in /docs have been updated or no updates are required.
Userland: The application README has been added, updated, or no updates are required.

Formatting

make formatall has been run.

- hil::symmetric_encryption - provide distinct traits for Ctr/CBC modes so we can be sure these hardware features exist at compile time - allow distinct src/dst encryption or in-place operation - fix up sam4l::aes to match the new interface, and track input/output with the hardware properly - add TakeCell.map_or_else() - tests are in boards/imix/src/aes_test.rs, and may be run by adding this just before the call to `kernel::main()`: aes_test::run(); NOTE: this commit removes capsules::symmetric_encryption, the userspace interface to AES. Future work will have to patch that back onto the modified HIL, and perhaps also provide a mux for userspace+kernel users.

…'s not pretty.

These methods now have the same semantics as put/take for a TakeCell, except that when the hardware is busy (between a call to crypt() and the following invocation of crypt_done()), they have degenerate behavior: put has no effect, and take returns None.

This changes the interface once more so that there is no opportunity for confusion over the state of buffer ownership.

bbbert · 2017-11-14T18:56:49Z

This build is failing due to the nrf AES implementation having not been updated to match the new HIL. I'm not familiar with how the nrf hardware works or if there are any subtleties about the current AES implementation. Can someone chime in if there should be anything special I should be looking for when porting it over?

niklasad1 · 2017-11-14T19:07:33Z

@bbbert I can have a look ;)

niklasad1 · 2017-11-14T23:28:29Z

Well, the AES implementation is only AES_CTR and not AES-CCM.
It uses AES_ECB in hardware and CTR in software but because the user-space functionality and the capsule is changed it is removed it safe "comment away" the existing AES code for to get the tests to pass (for now):

bradjc · 2017-11-27T19:53:39Z

Is it feasible that we could update the capsule to match the new HIL before merging this PR? It'd be a bummer to lose that userspace support.

niklasad1 · 2018-01-16T20:21:10Z

What is the state of this PR? I can probably fix the NRF5X ports to the new capsule if you help with fixing user-space support to the new capsule!

bbbert · 2018-01-20T00:38:28Z

@niklasad1 I believe nothing has changed since the last discussion about whether to update the NRF5X AES capsule or drop support. I'd be happy to work on updating the userspace capsule.

Could you take a look at the NRF5X AES driver and see if it can easily be refactored to fit the new HIL? Thanks!

niklasad1 · 2018-01-21T14:34:54Z

@bbbert
Yeah, I'm looking at the moment it should be plausible (I think).
I will implement something similar and send a PR to your ccm branch

But so far two things comes to mind:

You assume that AES_BLOCK_SIZE and KEY_SIZE are the same (it's true for AES128 but not for AES192 or AES256).
I suggest to introduce associated constants instead of global constants for example:

pub trait AES128<'a> {
  
  const KEY_SIZE = 16;
  const BLOCK_SIZE = 16;

  // methods
  .....
}

niklasad1 · 2018-01-21T19:50:41Z

capsules/src/test/aes.rs

+
+        let start = DATA_OFFSET;
+        let stop = DATA_OFFSET + DATA_LEN;
+


Why start at DATA_OFFSET (16)?

This will make my implementation overflow because I assume the output is as big the input and I don't use start and stop!

I believe that the point of the AES refactor is to allow the AES hardware to operate on arbitrary ranges of slices. Previously you could only encrypt/decrypt a mutable buffer from the beginning, because if you had sliced into it and passed it to the driver, you would never get the original slice back. In other words, whenever you wanted to encrypt only part of a buffer, you would need a copy just because of the HIL's restrictiveness. There are also some new options for encrypting/decrypting different parts of buffers: for example if the source isn't present, then in-place encryption should be done directly in the destination buffer.

As for why exactly it's 16, I imagine that's just an arbitrary offset for testing purposes, but @daniel-scs might know better.

Ok, makes sense. I was a little bit concerned about this on nrf51dk which only has 16kB RAM but it seems to fit. You can check my PR

niklasad1 · 2018-01-21T19:56:48Z

boards/imix/src/aes_test.rs

+fn static_init_test() -> &'static mut Test<'static, Aes<'static>> {
+    unsafe {
+        let source = static_init!([u8; 4 * AES128_BLOCK_SIZE], [0; 4 * AES128_BLOCK_SIZE]);
+        let data = static_init!([u8; 6 * AES128_BLOCK_SIZE], [0; 6 * AES128_BLOCK_SIZE]);


Why is the data buffer 6 blocks instead of 4 blocks?

Again, I think these are some arbitrary sizes to facilitate testing the ability to encrypt a range into a different part of a slice, but I might be wrong.

bbbert · 2018-01-22T05:24:31Z

@niklasad1 Sure, thanks. Side note: my ccm branch was in turn based off of @daniel-scs's ccm branch. He wrote the nice new AES HIL, I only added CCM* on top of it.

Regarding the key and block size, good point. I think that associated constants look like a good idea.
I'm reading the old symmetric_encryption.rs userspace-facing capsule, which has since been updated to use the new Grant concept. I might have missed the discussion around Grants, but I think I understand it as essentially a list of apps. I noticed that the driver seems to make the assumption that there is only one app: it activates the AES driver on the first app with a request, but when the callback arrives, it copies it to the first app which has a data buffer waiting. Does this mean that theoretically an app could end up with the encrypted/decrypted result from a different app, or am I misunderstanding something? Would the solution be to save the previously-activated AppId?

niklasad1 · 2018-01-22T08:42:08Z

@bbbert

I think you summarized the issue very well.
This was a while ago when I didn't understand the Grants that well but yes it was meant for one app and we should change it to support several apps. Note, that the subscribe call assigns a function pointer to particular user-app so if the wrong application is identified then yes the result can be returned to the wrong application

Roughly the grant section is a process specific segment which the kernel only has access, to dynamic allocate memory. Again, as you said I think that best idea is to store all encryption data in the grant and the capsule itself stores the AppId and then it knows which Grant to go to and send the result to. We did something similar for the BLE driver but in this case we don't need virtual timers.

BTW,
I think the PR is close it will probably arrive today or in the week!

niklasad1 · 2018-01-23T15:05:55Z

kernel/src/hil/symmetric_encryption.rs

+    /// Call before `AES128::crypt()` to perform AES128CBC
+    fn set_mode_aes128cbc(&self, encrypting: bool);
+}
+


Sorry, I don't want to be annoying but I would like the see consistency in the naming of the traits.
I think it has been discussed earlier that the first letter should uppercase and the rest lowercase?

So, I suggest:

Aes128Ctr

Aes128Cbc

Thoughts?

niklasad1 · 2018-01-23T16:05:58Z

@bbbert @alevy @daniel-scs

I have been thinking about AES userspace interface a little bit in regard to to the new HIL.
I suggest the following (I think it is better with rust code over English) and yes I ignored the AES128Cbc and AES128Ctr trait bounds because then both need to be supported which they are not by NRF5x

use core::cell::Cell;
use kernel::{AppId, AppSlice, Callback, Driver, Grant, ReturnCode, Shared};
use kernel::common::take_cell::TakeCell;
use kernel::hil::symmetric_encryption::{Client, AES128, AES128Ctr, AES128CBC};

/// Syscall number
pub const DRIVER_NUM: usize = 0x40000;

/// This enum shall keep track of the state of the AESDriver
#[derive(Copy, Clone, Debug, PartialEq)]
enum AesState {
    Idle,
    Encrypt,
    Decrypt,
    SetKey,
    SetIv,
}

#[allow(dead_code)]
#[derive(Copy, Clone)]
enum ConfidentialityMode {
    ECB = 0,
    CBC,
    CFB,
    OFB,
    Ctr,
}

pub struct App {
    callback: Option<Callback>,
    key: Option<AppSlice<Shared, u8>>,
    input: Option<AppSlice<Shared, u8>>,
    output: Option<AppSlice<Shared, u8>>,
    iv: Option<AppSlice<Shared, u8>>,
    state: Option<AesState>,
}

impl Default for App {
    fn default() -> App {
        App {
            callback: None,
            key: None,
            input: None,
            output: None,
            iv: None,
            state: Some(AesState::Idle),
        }
    }
}

pub struct AesDriver<'a, A: AES128<'a> + 'a> {
    crypto: &'a A,
    apps: Grant<App>,
    kernel_key: TakeCell<'static, [u8]>,
    kernel_input: TakeCell<'static, [u8]>,
    kernel_output: TakeCell<'static, [u8]>,
    kernel_iv: TakeCell<'static, [u8]>,
    busy: Cell<bool>,
    current_app: Cell<Option<AppId>>,
}

impl<'a, A: AES128<'a> + 'a> AesDriver<'a, A> {
    pub fn new(crypto: &'a A,
               grant: Grant<App>,
               key: &'static mut [u8],
               input: &'static mut [u8],
               output: &'static mut [u8],
               iv: &'static mut [u8])
               -> AesDriver<'a, A> {
        AesDriver {
            crypto: crypto,
            apps: grant,
            kernel_key: TakeCell::new(key),
            kernel_input: TakeCell::new(input),
            kernel_output: TakeCell::new(output),
            kernel_iv: TakeCell::new(iv),
            busy: Cell::new(false),
            current_app: Cell::new(None),
        }
    }
}

impl<'a, A: AES128<'a>> Driver for AesDriver<'a, A> {
    fn allow(&self, appid: AppId, allow_num: usize, slice: AppSlice<Shared, u8>) -> ReturnCode {
        match allow_num {  
            // Allocate input buffer
            0 => ReturnCode::SUCCESS,
            
            // Allocate output buffer
            1 => unimplemented!(),

            // Allocate key buffer
            2 => unimplemented!(),

            // Allocate iv buffer (iv or initial counter)
            3 => unimplemented!(),

            _ => ReturnCode::EINVAL,

        }
    }

    fn subscribe(&self, subscribe_num: usize, callback: Callback) -> ReturnCode {
        
        // callback to user-space
        unimplemented!() 
    }

    fn command(&self, cmd: usize, sub_cmd: usize, _: usize, _: AppId) -> ReturnCode {
        match cmd {
            
            // Check if exists
            0 => ReturnCode::SUCCESS,
            
            // Set Confidentiality Mode
            1 => unimplemented!(),

            // Enable (maps direectly to the HIL)
            2 => unimplemented!(),

            // Disable (maps direectly to the HIL)
            3 => unimplemented!(),

            // Start message (maps directly to the HIL)
            4 => unimplemented!(),

            // Encrypt
            // here we need to set the busy flag i.e., we need mutual exclusion to the chip
            // and store AppId in shared region inorder to know which app to return the result to
            5 => unimplemented!(),

            // Decrypt
            // here we need to set the busy flag i.e., we need mutual exclusion to the chip
            // and store AppId in shared region inorder to know which app to return the result to
            6 => unimplemented!(),
            
            _ => ReturnCode::EINVAL,
        }
    }
}


impl<'a, A: AES128<'a>> Client<'a> for AesDriver<'a, A> {
    fn crypt_done(&'a self, source: Option<&'a mut [u8]>, dest: &'a mut [u8]) {
        unimplemented!()
    }
}

Thoughts?

* The CBC-test fails because it is not supported. I suggest to change the test structure and the trait bounds accordingly

* Because the AES tests depended on trait bounds on both AES128Cbc and AES128Ctr => both the traits needed to implemented * Since NRF5x only support CTR ended up in test failed * That's why I splitted up them (I have not verified that tests are correct though) * The test passed on NRF5x

Add nik's changes to ccm branch so that we can make sure everything works

alevy · 2018-01-29T09:30:02Z

Uh oh... I just merged #692, which reorganized some of the mac layer, cause a bunch of conflicts in this PR now. I don't think they look super hard to resolve, but might require moving changes between files.

…ranch

bbbert · 2018-01-30T11:44:24Z

@niklasad1 Your port looks great! start_message is meant to indicate that a new message is being started: the interface is designed so that you can encrypt non-contiguous ranges of buffers by calling crypt multiple times. Message boundaries matter for modes like CTR or CBC where the hardware is keeping track of either the counter or the last ciphertext block.

Everything seems to be working now, I'm holding off including the userspace facing driver in this PR because it's a significant change on its own.

@alevy Okay, I'm now super confused about where everything went. Is XMAC meant to be building on top of an existing MAC layer, or does it replace one interchangeably? I could distribute all my changes across those files by matching old code, but I have no guarantees that it'll even work.

alevy · 2018-01-30T16:24:54Z

boards/nrf52dk/src/aes_test.rs

+}
+
+fn static_init_test() -> &'static mut TestAes128Ctr<'static, AesECB<'static>> {
+    unsafe {


This is minor, since it's in the test, but when using unsafe:

Use it inside the function body, if the block exposes completely safe behavior: i.e., if there is no way to call the function in a way that could violate type-safety (for exmaple, by calling a function twice that allocates a value in static memory).

Use it to mark a function unsafe if the function needs to do unsafe things and it doesn't necessarily expose a totally safe API.

I think these initializers in the test harnesses fall into the second category, so should be

unsafe fn static_init_test() -> &'static mut TestAes128Ctr<'static, AesECB<'static>> {

and

pub unsafe fn run()

they are then called from the board's reset_handler, which is also unsafe and we're good.

That’s great rules but it is still a grey area with mutating static buffers and dereferencing memory mapped I/O (data races)

Is it ”your rules” or idiomatic Rust?

I think I haven’t followed these rules to 100% to be honest!

niklasad1 · 2018-01-30T21:41:11Z

@bbbert

Thanks.
It's better handle the userspace driver in separate PR as @alevy suggested but when this is PR merged we should add an issue and maybe discuss how to design/implement it!

hudson-ayers · 2018-02-06T23:32:02Z

kernel/src/hil/symmetric_encryption.rs

+
+/// The number of bytes used for AES block operations.  Keys and IVs must have this length,
+/// and encryption/decryption inputs must be have a multiple of this length.
+pub const AES128_BLOCK_SIZE: usize = 16;


I noticed that despite commenting that you would add a constant for key size 16 days ago in response to @niklasad1 's suggestion, it appears you still assume all key sizes will be 128 bits. It doesn't look like this ended up happening, though I suppose that since all of the constant names imply this implementation is only for AES128 that is fine?

hudson-ayers · 2018-02-07T00:33:42Z

kernel/src/hil/symmetric_encryption.rs

-    fn set_client(&self, client: &'static Client);
+/// Implement this trait and use `set_client()` in order to receive callbacks from an `AES128`
+/// instance.
+pub trait Client<'a> {


Seems like this should be "AESClient" if the related trait defined below is called "CCMClient". Just "Client" seems a little vague

hudson-ayers · 2018-02-07T00:50:39Z

boards/imix/src/main.rs

    );
+    aes_ccm.set_client(mac_device);


Just to clarify: There shouldn't be any problem calling this function from a "Thread" capsule to set the client as the Thread control plane for instances when MLE messages are being sent and the link layer is not encrypting messages, correct? It would obviously need a reference to the mac_device as well so that it could set the client back once the MLE process was complete. I could see this being an issue in cases where one app is running Thread and another is simply trying to communicate using encrypted 15.4 - somehow there would need to be a mechanism to ensure that the mac_device is always set as the client when link-layer encryption is needed, while also allowing Thread to be set as the client when above-link-layer encryption is needed for MLE messages.
That seems like the only potential issue which could hinder the 6lowpan/Thread development down the road, and even if it is something that will eventually have to change I think the current implementation is fine and will not hinder the current 6lowpan/Thread development as I expect getting CCM* working on MLE messages will be one of the final steps towards a SED

hudson-ayers

I think that the current implementation looks generally good, and should not hinder the current development towards 6lowpan/Thread. I am not sure I entirely understand the rules governing how the client of AES/CCM can be assigned and I suspect this could eventually need to be addressed to allow for a Thread SED, as described in one of my comments, but regardless I think it is okay to merge this implementation for the 1.0 release.

alevy · 2018-01-30T16:25:57Z

boards/nrf52dk/src/main.rs

@@ -380,6 +366,9 @@ pub unsafe fn reset_handler() {
        &mut PROCESSES,
        FAULT_RESPONSE,
    );
+
+    aes_test::run();


We don't want to do this by default, do we?

daniel-scs and others added 16 commits October 6, 2017 17:35

AES: formatting

22fe7f1

AES: remove userland tests for now

85b28bc

AES: fix some doc text; fix rustfmt pragmas

c183a9b

AES: set/take source buffer; copy key/iv

19d7972

AES: move test to capsules::test::

6a55872

It compiles. This is what the capsule would have to look like, and it…

841610f

…'s not pretty.

Objections to AES interface

dc70c7a

Make AESCCM a trait

6b7bce5

Merge branch 'cbc' into ccm

f250449

AES: pass bufs to crypt(), return at crypt_done()

08d63b5

This changes the interface once more so that there is no opportunity for confusion over the state of buffer ownership.

Merge branch 'cbc' into ccm

4596980

Work with improved AES interface

478f054

Use aes_ccm module for link-layer encryption in MacDevice

168c7bc

Add tests for AES CCM

0602b2f

daniel-scs mentioned this pull request Nov 21, 2017

AES: mods as necessary for 802.15.4 work #642

Closed

3 tasks

niklasad1 reviewed Jan 21, 2018

View reviewed changes

niklasad1 reviewed Jan 23, 2018

View reviewed changes

bbbert and others added 7 commits January 25, 2018 17:41

Use aes_ccm module for link-layer encryption in MacDevice

8db7555

Add tests for AES CCM

24cd1f7

NRF5x port to new symmetric_encryption HIL

a2807b7

* The CBC-test fails because it is not supported. I suggest to change the test structure and the trait bounds accordingly

cleanup unused code and added tests for nRF51-DK

1a7af3e

Merge branch 'ccm' into ccm

9ff0ec5

Merge pull request #1 from niklasad1/ccm

616225e

Add nik's changes to ccm branch so that we can make sure everything works

bbbert added 2 commits January 29, 2018 11:41

Fix errors (duplicated/old code) introduced when merging niklasad's b…

98066ac

…ranch

Add support for CCM* with or without confidentiality

e5309a6

bbbert added 2 commits January 30, 2018 03:49

Make formatall

4db174e

Merge branch 'master' into ccm

37f948f

bbbert changed the title ~~[ieee802154] CCM* implementation~~ [ieee802154] CCM* implementation and new symmetric encryption HIL Jan 30, 2018

alevy requested review from alevy and hudson-ayers January 30, 2018 16:12

alevy reviewed Jan 30, 2018

View reviewed changes

Declare test functions unsafe

0122f40

hudson-ayers reviewed Feb 6, 2018

View reviewed changes

hudson-ayers reviewed Feb 7, 2018

View reviewed changes

hudson-ayers approved these changes Feb 7, 2018

View reviewed changes

alevy and others added 2 commits February 7, 2018 18:31

Merge branch 'master' into ccm

b6c0c9b

Cleanup

a664a97

alevy approved these changes Feb 8, 2018

View reviewed changes

alevy merged commit 267df6b into tock:master Feb 8, 2018

alevy mentioned this pull request Feb 8, 2018

Remove extraneous AES test #743

Merged

1 task

This was referenced Feb 22, 2021

Tock 2.0 switchover tock/libtock-c#177

Merged

Tracking: AES system call driver #2453

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ieee802154] CCM* implementation and new symmetric encryption HIL #684

[ieee802154] CCM* implementation and new symmetric encryption HIL #684

bbbert commented Nov 14, 2017 •

edited

bbbert commented Nov 14, 2017

niklasad1 commented Nov 14, 2017

niklasad1 commented Nov 14, 2017

bradjc commented Nov 27, 2017

niklasad1 commented Jan 16, 2018 •

edited

bbbert commented Jan 20, 2018

niklasad1 commented Jan 21, 2018

niklasad1 Jan 21, 2018 •

edited

bbbert Jan 22, 2018

niklasad1 Jan 22, 2018

niklasad1 Jan 21, 2018

bbbert Jan 22, 2018

bbbert commented Jan 22, 2018

niklasad1 commented Jan 22, 2018 •

edited

niklasad1 Jan 23, 2018 •

edited

niklasad1 commented Jan 23, 2018

alevy commented Jan 29, 2018

bbbert commented Jan 30, 2018

alevy Jan 30, 2018

niklasad1 Jan 30, 2018

niklasad1 commented Jan 30, 2018

hudson-ayers Feb 6, 2018

hudson-ayers Feb 7, 2018

hudson-ayers Feb 7, 2018

hudson-ayers left a comment

alevy Jan 30, 2018

[ieee802154] CCM* implementation and new symmetric encryption HIL #684

[ieee802154] CCM* implementation and new symmetric encryption HIL #684

Conversation

bbbert commented Nov 14, 2017 • edited

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

bbbert commented Nov 14, 2017

niklasad1 commented Nov 14, 2017

niklasad1 commented Nov 14, 2017

bradjc commented Nov 27, 2017

niklasad1 commented Jan 16, 2018 • edited

bbbert commented Jan 20, 2018

niklasad1 commented Jan 21, 2018

niklasad1 Jan 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbbert commented Jan 22, 2018

niklasad1 commented Jan 22, 2018 • edited

niklasad1 Jan 23, 2018 • edited

Choose a reason for hiding this comment

niklasad1 commented Jan 23, 2018

alevy commented Jan 29, 2018

bbbert commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hudson-ayers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbbert commented Nov 14, 2017 •

edited

niklasad1 commented Jan 16, 2018 •

edited

niklasad1 Jan 21, 2018 •

edited

niklasad1 commented Jan 22, 2018 •

edited

niklasad1 Jan 23, 2018 •

edited