Skip to content
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

Add encrypted images support #330

Merged
merged 9 commits into from Oct 12, 2018

Conversation

@utzig
Copy link
Member

commented Sep 6, 2018

This allows storing encrypted images in slot1, that are automatically decrypted when copying to slot0 and re-encrypted when copying from slot0 to slot1.

The encryption works by applying AES-128-CTR on the image blocks (excluding the header and TLVs) using a random key. This random key is itself encrypted using either RSA-2048-OAEP or AES-128-KW (AES keywrap as defined by RFC3394), and appended to the image as newly defined TLVs.

AES-128-CTR was chosen primarily for having stream cipher properties, which basically means that any block being encrypted/decrypted does not depend on any other previous blocks results.

The TLV adds about 256 bytes to the image in RSA-OAEP mode, and 24 bytes in AES-128-KW mode. Resulting sizes for a Mynewt generated mcuboot (frdm-k64f):

  • swap mode and no signing: 12KB
  • adding encryption with RSA-OAEP: 28KB
  • adding encryption with AES-KW: 20KB

Some extra comments:

  • AES-128-KW requires a fairly new mbedtls with nist_kw support.
  • An alternative methods which could be added later are ECIES.
  • Key-wrapping seems easy enough to implement using just standard AES-ECB mode that it should be straight-forward to also add support to tinycrypt.

@utzig utzig requested review from d3zd3z, nvlsianpu and carlescufi Sep 6, 2018

@utzig

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

@d3zd3z Would be glad if you could take a look at the simulator changes. They are failing on the encrypted swap test (as expected) because I added some temp code to clean the global state variables in bootutil. One unrelated change is that the header struct was fixed to match bootutil, and some FIXME were not removed because they are not matching some other part of the code. Please review specifically this commit 19c14f8.

@utzig

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

The status right now is that overwrite-only mode is fully supported but swap mode does not support interruption (because it's still lacking the code to find the TLV containing the key, which will be added next). imgtool can generate encrypted images but does not yet generate the code structures from pem files.

@carlescufi

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2018

I probably won't have time to look at the PR this week, so I'm copying a couple of Nordic engineers that might have more time.

CC @oyvindronningstad @lemrey @frkv

@oyvindronningstad

This comment has been minimized.

Copy link

commented Sep 7, 2018

Since the RSA-OAEP part is so heavy, can it be optional?

@utzig

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2018

@oyvindronningstad As long as another key encrypting mechanism is present, sure. The threat model here is that whatever is already flashed inside the chip is to be considered secure. So, this PR is mostly useful to transport images, from some place to the final HW and also because there is need to protect FW when slot 1 happens to be an external SPI chip (a real case). The main reason to have used RSA-OAEP was its existing availability in libraries, but later this PR can easily be extended to add KEK or EICES. KEK is available for python's cryptography (used by imgtool) and mbed-TLS already, EICES is more complicated! ;-)

@oyvindronningstad

This comment has been minimized.

Copy link

commented Sep 7, 2018

@utzig How about using AES for the key encryption as well?

Also, have you looked into COSE_Encrypt?

@utzig

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2018

@utzig How about using AES for the key encryption as well?

Sorry, that's what I meant by KEK. Should have used key-wrap, but yes, was referring to the AES key encrypt AES key...

Also, have you looked into COSE_Encrypt?

I have not!

@oyvindronningstad

This comment has been minimized.

Copy link

commented Sep 7, 2018

Ok, cool :).

Sorry, that's what I meant by KEK.

Right, sorry. My first google search for KEK didn't turn up much so I just assumed it was another asymmetric algorithm :P.

Also, have you looked into COSE_Encrypt?

I have not!

https://datatracker.ietf.org/doc/rfc8152/

It's a serialization format that hopefully could cover the needs of this feature. It's also very relevant for IETF SUIT.

@utzig utzig force-pushed the utzig:encrypted-images branch from d0043a8 to af24c6b Sep 7, 2018

@utzig

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

@oyvindronningstad Just added another commits that adds the KEK support. Using a freedom-k64f with Mynewt, the numbers I get for the bootloader with no sign (no mbedtls dependecy) is around 12KB, with encryption using RSA-OAEP it is around 28KB, and using AES-128-KEK around 20KB. The TLV was reduced from 256 bytes to 24 bytes. Unfortunately this requires a very new release of mbedtls (I have a PR for Mynewt here: apache/mynewt-core#1393), and I hope to update the simulator's bundled mbedtls very soon to be able to test this.

@utzig utzig force-pushed the utzig:encrypted-images branch from d86499a to c0c982b Sep 13, 2018

@mkiiskila
Copy link
Collaborator

left a comment

Looks pretty good so far.

/*
* Indicates that this image should be loaded into RAM instead of run
* directly from flash. The address to load should be in the
* ih_load_addr field of the header.
*/
#define IMAGE_F_RAM_LOAD 0x00000020
#define IMAGE_F_RAM_LOAD 0x00000020 //FIXME

This comment has been minimized.

Copy link
@mkiiskila

mkiiskila Sep 13, 2018

Collaborator

Why the comment?

This comment has been minimized.

Copy link
@utzig

utzig Sep 13, 2018

Author Member

Yup, unfortunately I can't remember the exact issue, but I think I was trying to find out the reason for the gap in the bitmask, and while looking for other places that define this (simulator, imgtool, newt and v1 header in mynewt) the values didn't match!

So newt for v2 format has (ENCRYPTED on my PR only!):

IMAGE_F_PIC          = 0x00000001
IMAGE_F_NON_BOOTABLE = 0x00000002 /* non bootable image */
IMAGE_F_ENCRYPTED    = 0x00000004 /* encrypted image */

imgtool has:

IMAGE_F = {
        'PIC':                   0x0000001,
        'NON_BOOTABLE':          0x0000010,
        'ENCRYPTED':             0x0000004,
}

Here already NON_BOOTABLE does not match.

The simulator has:

pub enum TlvFlags {
    PIC = 0x01,
    NON_BOOTABLE = 0x02,
    ENCRYPTED = 0x04,
    RAM_LOAD = 0x20,
}

And mcuboot itself:

#define IMAGE_F_PIC                      0x00000001 /* Not supported. */
#define IMAGE_F_NON_BOOTABLE             0x00000002 /* Split image app. */
#define IMAGE_F_ENCRYPTED                0x00000004
#define IMAGE_F_RAM_LOAD                 0x00000020

In the end I think the policy was to try not to reuse defines that clash with those from v1, but maybe we could have just started with new values, since the magic already differentiates the header versions, and instead of jumping over bits just increase them in the usual sequential way. I will remove this now when squashing the commits to cleanup and open an issue to discuss it.

const struct flash_area *fap;
int rc;

rc = flash_area_open(FLASH_AREA_IMAGE_0, &fap);

This comment has been minimized.

Copy link
@mkiiskila

mkiiskila Sep 13, 2018

Collaborator

Given the function parameters, should this be flash_area_open(slot,...)?

This comment has been minimized.

Copy link
@utzig

utzig Sep 17, 2018

Author Member

@mkiiskila Nope, the temporary keys are written to either slot0 or scratch (during swap procedure). The slot parameter is here because both keys are saved temporarily and the correct one is indexed by slot.

assert(enc->valid == 1);
for (i = 0; i < sz; i++) {
if (i == 0 || blk_off == 0) {
mbedtls_aes_crypt_ecb(&enc->aes, MBEDTLS_AES_ENCRYPT, nonce, blk);

This comment has been minimized.

Copy link
@mkiiskila

mkiiskila Sep 13, 2018

Collaborator

This is ok for me. We can add hw accel later (if even necessary).

blk_off = (off - hdr->ih_hdr_size) & 0xf;
}
boot_encrypt(fap, (off + idx) - hdr->ih_hdr_size, blk_sz - idx,
blk_off, &tmp_buf[idx]);

This comment has been minimized.

Copy link
@mkiiskila

mkiiskila Sep 13, 2018

Collaborator

This might be simpler if you generated the encrypting stream such that offset 0 from the beginning of image is 0, but don't start xor'ing that stream to turn the image into ciphertext until it reaches of image header.
The way you have it here is fine too.
I.e. I believe you could say here: boot_encrypt(fap, off, blk_sz - idx, blk_off, &tmp_buf[idx]);
blk_off computation would change slightly too. blk_off = off & 0xf, or something like that.

This comment has been minimized.

Copy link
@utzig

utzig Sep 17, 2018

Author Member

Yeah, it would be simpler. I'm not sure I would like to change how it is done because this has the nice property that the counter for AES-CTR starts at 0 and increments by 1. All tooling (imgtool, newt for the moment being) need to agree on how the image is to be encrypted and even though this slightly complicates the calculation it makes it super easy for anyone to create, encrypt and decrypt images (knowing the secret key, of course!). Also it does not require me to change newt and imgtool now! :-)

So I think it should stay this way at least for now, and we could maybe discuss it again in the near future.

static int
boot_erase_sector(int flash_area_id, uint32_t off, uint32_t sz)
static inline int
boot_erase_sector(const struct flash_area *fap, uint32_t off, uint32_t sz)

This comment has been minimized.

Copy link
@mkiiskila

mkiiskila Sep 13, 2018

Collaborator

Good.

}

#ifndef MCUBOOT_OVERWRITE_ONLY
static inline int
boot_status_init_by_id(int flash_area_id, const struct boot_status *bs)
boot_status_init(const struct flash_area *fap, const struct boot_status *bs)

This comment has been minimized.

Copy link
@mkiiskila

mkiiskila Sep 13, 2018

Collaborator

I like this change too.

@mkiiskila

This comment has been minimized.

Copy link
Collaborator

commented Sep 13, 2018

I think this is great. One thing which I'd want to be able to do also is to use encrypted images with serial bootloader, but that should probably be addressed within the serial bootloader code itself. And probably does not need to be part of this PR.

Currently boot_serial always writes the image it receives to slot 0. I think for encrypted images it must write it to slot 1, and then copy it over to slot0 (as the key would not be available otherwise).

@utzig

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

@mkiiskila Ha, I actually had thought about this before and my idea was to add an option to newt and imgtool to allow them to insert the encryption header and TLV while at the same time not actually encrypting the generated image, which would allow it to be sent over boot_serial! Of course, this adds an extra layer of complexity because now the user must know which is which, and also it allows for "leaking" an non-encrypted image (and reverse engineering, etc). Other option would be to allow mcumgr/newtmgr to un-encrypt on the fly, but that also requires a private key to be shared with users, so in the end it might just be a better idea to make boot_serial write it to offset 1 and "upgrade" on the first boot.

@utzig utzig force-pushed the utzig:encrypted-images branch 6 times, most recently from ffee671 to 07a48c3 Sep 17, 2018

@utzig

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2018

I cleaned up the git history now.

Some things are missing on the Zephyr side, like encryption with AES-128-KW and I also did not add support for generating images using AES keys to imgtool. I haven't done it because mbedtls was too old on Zephyr, but seems like it was updated to latest last week!

@d3zd3z would be nice to get some security related review of this.

@utzig utzig force-pushed the utzig:encrypted-images branch from 07a48c3 to 9cddcae Sep 28, 2018

#endif

#define BOOT_ENC_KEY_SIZE 16
#define BOOT_ENC_KEY_SIZE_BITS 128

This comment has been minimized.

Copy link
@nvlsianpu

nvlsianpu Sep 28, 2018

Collaborator

BOOT_ENC_KEY_SIZE * 8

enckey_type = IMAGE_TLV_ENC_KW;
break;
}
#endif

This comment has been minimized.

Copy link
@nvlsianpu

nvlsianpu Sep 28, 2018

Collaborator

In your convenience:

uint8_t enckey_type = 0;
(...)
#if defined(MCUBOOT_ENCRYPT_xxx)
        if (tlv.it_type == IMAGE_TLV_ENC_xxx) {
            if (tlv.it_len != TLV_ENC_KW_xxx) {
                return -1;
            }

            enckey_type = IMAGE_TLV_ENC_xxx;
        }
#endif 
(...)

if (enckey_type) {
            rc = flash_area_read(fap, off + sizeof(tlv), buf, tlv.it_len);
            if (rc) {
                return rc;
            }
            break;
}
@utzig

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

@d3zd3z Docs added!

@utzig utzig force-pushed the utzig:encrypted-images branch 2 times, most recently from 0624951 to 8b593d8 Oct 1, 2018

@d3zd3z

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2018

I've reviewed the design (from the docs), and nothing is standing out to me as a flaw from a security perspective.

COSE_Encrypt would add a lot of complexity to this (and a lot of code). It does support a separate payload, so could be placed after the image. I'm not sure there is really much benefit to this, either.

I would want to revisit the security when adding any other key wrapping method. The complexity in RSA-OAEP and and AES-128-KW are to address specific security concerns, and we would want to make sure any other key wrapping operation also provides sufficient security. I think ECIES should also provide the needed security.

@d3zd3z
d3zd3z approved these changes Oct 12, 2018
utzig added 9 commits Aug 23, 2018
Add imgtool support for encrypted image creation
Adds a new flag to imgtool, -E/--encrypt which accepts a public rsa-2048
key file that will be used to encrypt the image.

The encryption method uses AES-128-CTR to encrypt the image data (ignores
the header and TLVs), using a random key that is itself encrypted using
RSA-2048-OAEP and added to the generated image as a new TLV.

Signed-off-by: Fabio Utzig <utzig@apache.org>
Add Zephyr support for encrypted images
Signed-off-by: Fabio Utzig <utzig@apache.org>
Add sample encryption keys
This adds both RSA (pub + private) keys and a base64 encoded AES-128
secret key, which should be used as sample keys in encrypted image tests.

Signed-off-by: Fabio Utzig <utzig@apache.org>
Enable encrypted image tests to run on travis-ci
Signed-off-by: Fabio Utzig <utzig@apache.org>
Add bootutil support for encrypted images
This allows storing encrypted images in slot1, that are automatically
decrypted when copying to slot0 and re-encrypted when copying from slot0
to slot1.

The encryption works by applying AES-CTR-128 on the image blocks
(excluding the header and TLVs) using a random key. This random key
is itself encrypted using either RSA-OAEP-2048 or AES-KW-128 (AES keywrap
as defined by RFC3394), and appended to the image as newly defined TLVs.

AES-CTR-128 was chosen primarily for having stream cipher proporties,
which basically means that any block being encrypted/decrypted does not
depend on any other previous blocks results.

The TLV adds about 256 bytes to the image in RSA-OAEP-2048 mode, and 24
bytes in AES-KW-128 mode. Resulting sizes for a Mynewt generated mcuboot
(frdm-k64f):

- swap mode and no signing: 12KB
- adding encryption with RSA-OAEP-2048: 28KB
- adding encryption with AES-KW-128: 20KB

Some extra comments:

- AES-KW-128 requires a fairly new mbedtls with nist_kw support.
- An alternative methods which could be added later are ECIES.
- Key-wrapping seems easy enough to implement using just standard
  AES-ECB mode that it should be straight-forward to also add support to
  tinycrypt.

Signed-off-by: Fabio Utzig <utzig@apache.org>
Add Mynewt config for encrypted images
Signed-off-by: Fabio Utzig <utzig@apache.org>
Add encrypted image support on sim
This adds new cargo features to allow running tests of encrypted
images with both RSA-OAEP and AES-128-KW.

When installing images on the simulated flash, both a plain and an
encrypted images are created. When encrypted image support is enabled,
verification of images in slot1 match against the encrypted image,
otherwise plain images are used.

PS: Also fixes ImageHeader to match bootutil definition.

Signed-off-by: Fabio Utzig <utzig@apache.org>
Add mbedtls config with nist_kw enabled
This adds a mbedtls config that will enable the simulator to run tests
for the kw based encrypted images.

Signed-off-by: Fabio Utzig <utzig@apache.org>
Add encrypt images design/usage documentation
Signed-off-by: Fabio Utzig <utzig@apache.org>

@utzig utzig force-pushed the utzig:encrypted-images branch from 8b593d8 to 466a91f Oct 12, 2018

@utzig utzig merged commit cdfa11a into JuulLabs-OSS:master Oct 12, 2018

1 check passed

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

@utzig utzig deleted the utzig:encrypted-images branch Oct 12, 2018

@JonasNorling

This comment has been minimized.

Copy link

commented Oct 13, 2018

Hey, I think this just broke imgtool (or I'm just doing something terribly stupid). The constructor of class Image doesn't take an encrypt argument, but gets sent one at imgtool.py line 145. It fails like this (I'm trying to build samples/zephyr):

../../scripts/imgtool.py sign --key ../../root-rsa-2048.pem --header-size 0x200 --align 8 --version 1.2 --slot-size 0x60000 /home/jonas/src/mcuboot/samples/zephyr/build/nrf52_pca10040/hello1/zephyr/zephyr.bin signed-hello1.bin
Traceback (most recent call last):
  File "../../scripts/imgtool.py", line 194, in <module>
    imgtool()
  File "/home/jonas/.local/lib/python3.6/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/home/jonas/.local/lib/python3.6/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/home/jonas/.local/lib/python3.6/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/jonas/.local/lib/python3.6/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/jonas/.local/lib/python3.6/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "../../scripts/imgtool.py", line 145, in sign
    encrypt=encrypt)
  File "/home/jonas/src/mcuboot/scripts/imgtool/image.py", line 94, in load
    obj = cls(**kwargs)
TypeError: __init__() got an unexpected keyword argument 'encrypt'
@utzig

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2018

@JonasNorling Thanks for reporting! Must have messed up when rebasing after the endiannes PR; fix submitted #347.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.