Skip to content

KVStore: Fix buffer overrun when device key size doesn't match #12875

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

Merged

Conversation

ccli8
Copy link
Contributor

@ccli8 ccli8 commented Apr 28, 2020

Summary of changes

This PR tries to fix #12822, where buffer may overrun when injected device key is 32-byte but read as 16-byte.


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

@ciarmcom
Copy link
Member

@ccli8, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

0xc0170
0xc0170 previously approved these changes Apr 28, 2020
@mergify mergify bot added needs: CI and removed needs: review labels Apr 28, 2020
SeppoTakalo
SeppoTakalo previously approved these changes Apr 28, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 28, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 28, 2020

Test run: FAILED

Summary: 1 of 6 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 28, 2020

There are lot of failed related test, please review

@ccli8 ccli8 force-pushed the nuvoton_kvstore_devicekey_buffer_overrun branch from 4901f11 to f01773a Compare April 29, 2020 09:07
@ccli8
Copy link
Contributor Author

ccli8 commented Apr 29, 2020

This PR adds check for device key size and will catch key size mismatch cases, which are missed before. #12823 can have similar issues. To address them, I make the modifications:

  1. Chery-pick Allow Devicekey::generate_root_of_trust() to define key size. #12823. This enables specifying key size in generate_root_of_trust(...) and default key size changes from 32 bytes to 16 bytes to match most known code, like:

    int os_ret = devkey.generate_derived_key(salt_buf, strlen(salt), encrypt_key, DEVICE_KEY_16BYTE);

    And:

    int os_ret = devkey.generate_derived_key(salt_buf, strlen(salt), auth_key, DEVICE_KEY_16BYTE);

  2. Fix test code to specify key size explicitly in the generate_root_of_trust(...) call when necessary.

@mergify mergify bot added needs: CI and removed needs: work labels Apr 29, 2020
@mergify mergify bot dismissed stale reviews from 0xc0170 and SeppoTakalo April 29, 2020 12:32

Pull request has been modified.

ccli8 added 2 commits May 4, 2020 09:11
This change fixes buffer overrun when injected device key is 32-byte but read as 16-byte.
@ccli8 ccli8 force-pushed the nuvoton_kvstore_devicekey_buffer_overrun branch from f01773a to 405ee47 Compare May 4, 2020 03:15
@ccli8
Copy link
Contributor Author

ccli8 commented May 4, 2020

Do rebase to base on #12823

@mbed-ci
Copy link

mbed-ci commented May 4, 2020

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 2
Build artifacts

@0xc0170
Copy link
Contributor

0xc0170 commented May 8, 2020

Will be merged as soon as we have 6.0.0 beta1 released

@0xc0170 0xc0170 merged commit 96c0e9c into ARMmbed:master May 12, 2020
@mergify mergify bot removed the ready for merge label May 12, 2020
@ccli8 ccli8 deleted the nuvoton_kvstore_devicekey_buffer_overrun branch May 13, 2020 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KVStore: Potential buffer overrun in TDBStore
5 participants