gh-ARMmbed/mbed-crypto
Watch
1
0
Fork
You've already forked mbed-crypto
 0
Code Issues 36 Pull requests 1 Releases 7 Activity

Multipart AEAD buffer output sizes #102

Merged
gilles-peskine-arm merged 14 commits from psa-aead_multipart-delay into psa-api-1.0-beta 2019-05-16 07:34:22 -05:00
Conversation 11 Commits 14 Files changed 8 +410 -126
gilles-peskine-arm commented 2019-05-06 09:07:56 -05:00 (Migrated from github.com)
  • Allow delayed output in AEAD for all algorithms. Although CCM and GCM allow the data to be encrypted or decrypted as a stream cipher, some hardware implementations can only process a full block at a time apart from the last block. Fix ARMmbed/psa-crypto#130
  • Add output buffer parameter for psa_aead_verify. It needs it as much as psa_aead_finish.
  • Add buffer size macros for multipart AEAD functions.
  • Add a flag for built-from-block-cipher in AEAD algorithm encoding.
  • Declare and implement ChaCha20 and ChaCha20-Poly1305 algorithms.

Out of scope: testing the new algorithms beyond smoke tests; testing the new macros; testing multipart AEAD.

* Allow delayed output in AEAD for all algorithms. Although CCM and GCM allow the data to be encrypted or decrypted as a stream cipher, some hardware implementations can only process a full block at a time apart from the last block. Fix ARMmbed/psa-crypto#130 * Add output buffer parameter for `psa_aead_verify`. It needs it as much as `psa_aead_finish`. * Add buffer size macros for multipart AEAD functions. * Add a flag for built-from-block-cipher in AEAD algorithm encoding. * Declare and implement ChaCha20 and ChaCha20-Poly1305 algorithms. Out of scope: testing the new algorithms beyond smoke tests; testing the new macros; testing multipart AEAD.
athoelke (Migrated from github.com) requested changes 2019-05-07 09:55:38 -05:00
athoelke (Migrated from github.com) left a comment

overall approach looks sound, still need to double check the buffer computation logic but on first viewing it is broadly correct.

However, there are a few problems - see individual comments.

overall approach looks sound, still need to double check the buffer computation logic but on first viewing it is broadly correct. However, there are a few problems - see individual comments.
include/psa/crypto_sizes.h
athoelke (Migrated from github.com) commented 2019-05-07 09:40:51 -05:00

copy paste bug? - plaintext_length -> ciphertext_length

copy paste bug? - `plaintext_length` -> `ciphertext_length`
athoelke (Migrated from github.com) commented 2019-05-07 09:43:25 -05:00
  1. parameter mismatch: plaintext_length -> input_length
  2. Result for AEAD on block cipher is not rounded, but scaled. Presume missing ( ... ) * PSA_MAX_BLOCK_CIPHER_BLOCK_SIZE
1. parameter mismatch: `plaintext_length` -> `input_length` 2. Result for AEAD on block cipher is not rounded, but scaled. Presume missing `( ... ) * PSA_MAX_BLOCK_CIPHER_BLOCK_SIZE`
athoelke (Migrated from github.com) commented 2019-05-07 09:43:33 -05:00

parameter mismatch: plaintext_length -> input_length

parameter mismatch: `plaintext_length` -> `input_length`
athoelke (Migrated from github.com) commented 2019-05-07 09:45:58 -05:00

Just double checking that you intended to refer to the finishing function here, or should this be psa_aead_update()?

Just double checking that you intended to refer to the finishing function here, or should this be `psa_aead_update()`?
Patater (Migrated from github.com) reviewed 2019-05-07 12:03:23 -05:00
include/psa/crypto.h
@ -2495,12 +2495,24 @@ psa_status_t psa_aead_update_ad(psa_aead_operation_t *operation,
* - In particular, do not copy the output anywhere but to a
Patater (Migrated from github.com) commented 2019-05-07 12:03:11 -05:00

Is this asking too much of implementations (for accelerators or secure elements)?

Is this asking too much of implementations (for accelerators or secure elements)?
include/psa/crypto.h
Patater (Migrated from github.com) commented 2019-05-07 11:54:13 -05:00

"This is the remaining from previous calls"

I think we are missing a word. Suggestion: "This is the remaining plaintext from previous calls..."

"This is the remaining from previous calls" I think we are missing a word. Suggestion: "This is the remaining plaintext from previous calls..."
include/psa/crypto_sizes.h
Patater (Migrated from github.com) commented 2019-05-07 11:48:58 -05:00

Looks like a pre-existing bug. Good to add a commit to fix that.

Looks like a pre-existing bug. Good to add a commit to fix that.
Patater (Migrated from github.com) reviewed 2019-05-09 10:46:44 -05:00
Patater (Migrated from github.com) left a comment

Do we need a new error code returned from any of the AEAD functions in order to indicate "The hardware is busy/delayed right now and isn't ready to return anything yet. Try again later."?

Do we need a new error code returned from any of the AEAD functions in order to indicate "The hardware is busy/delayed right now and isn't ready to return anything yet. Try again later."?
gilles-peskine-arm commented 2019-05-09 11:30:01 -05:00 (Migrated from github.com)

Do we need a new error code returned from any of the AEAD functions in order to indicate "The hardware is busy/delayed right now

No. The API is fully synchronous. If the hardware is busy, the call blocks.

All calls consume all the input (or fail). What's implementation-defined is only how whether the output at each stage is the maximum that can be mathematically calculated or not. Is this something that needs to be clarified in the documentation of specific functions?

> Do we need a new error code returned from any of the AEAD functions in order to indicate "The hardware is busy/delayed right now No. The API is fully synchronous. If the hardware is busy, the call blocks. All calls consume all the input (or fail). What's implementation-defined is only how whether the output at each stage is the maximum that can be mathematically calculated or not. Is this something that needs to be clarified in the documentation of specific functions?
👍 1
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-14 07:56:51 -05:00
include/psa/crypto.h
@ -2495,12 +2495,24 @@ psa_status_t psa_aead_update_ad(psa_aead_operation_t *operation,
* - In particular, do not copy the output anywhere but to a
gilles-peskine-arm (Migrated from github.com) commented 2019-05-14 07:56:51 -05:00

The point of this PR is precisely to avoid asking too much. But the wording could use some improvements. What did you think this is asking?

The point of this PR is precisely to avoid asking too much. But the wording could use some improvements. What did you think this is asking?
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-14 09:11:38 -05:00
include/psa/crypto.h
@ -2495,12 +2495,24 @@ psa_status_t psa_aead_update_ad(psa_aead_operation_t *operation,
* - In particular, do not copy the output anywhere but to a
gilles-peskine-arm (Migrated from github.com) commented 2019-05-14 09:11:38 -05:00

I've reworded this paragraph.

I've reworded this paragraph.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-14 09:12:45 -05:00
include/psa/crypto_sizes.h
gilles-peskine-arm (Migrated from github.com) commented 2019-05-14 09:12:45 -05:00

Fixed, and I modified the AEAD tests to use this macro.

Fixed, and I modified the AEAD tests to use this macro.
Patater (Migrated from github.com) reviewed 2019-05-15 04:22:58 -05:00
include/psa/crypto.h
@ -2498,1 +2503,4 @@
* provides sufficient input. The amount of data that can be delayed
* in this way is bounded by #PSA_AEAD_UPDATE_OUTPUT_SIZE.
*
* \param[in,out] operation Active AEAD operation.
Patater (Migrated from github.com) commented 2019-05-15 04:22:44 -05:00

OK, this makes it more clear that an implementation doesn't have to store and delay if it doesn't want to, but it has the option to. Good.

OK, this makes it more clear that an implementation doesn't have to store and delay if it doesn't want to, but it has the option to. Good.
tests/suites/test_suite_psa_crypto.function
Patater (Migrated from github.com) commented 2019-05-15 04:20:43 -05:00

Do we expect the length to be correct for errors other than PSA_ERROR_INVALID_ARGUMENT? If not, should we check for expected_result == PSA_SUCCESS?

Do we expect the length to be correct for errors other than PSA_ERROR_INVALID_ARGUMENT? If not, should we check for expected_result == PSA_SUCCESS?
gilles-peskine-arm (Migrated from github.com) reviewed 2019-05-15 04:33:10 -05:00
tests/suites/test_suite_psa_crypto.function
gilles-peskine-arm (Migrated from github.com) commented 2019-05-15 04:33:10 -05:00

The length is expected to be correct except if the algorithm is invalid. This includes cases where the operation might fail for other reasons, at least INVALID_SIGNATURE. It would be better to have properly separate tests for the macro and for the operation, but that's a much bigger task than this PR (it applies to all the other size macros).

The length is expected to be correct except if the algorithm is invalid. This includes cases where the operation might fail for other reasons, at least `INVALID_SIGNATURE`. It would be better to have properly separate tests for the macro and for the operation, but that's a much bigger task than this PR (it applies to all the other size macros).
Patater (Migrated from github.com) approved these changes 2019-05-15 05:16:28 -05:00
Patater commented 2019-05-16 07:33:57 -05:00 (Migrated from github.com)

CI failure is ABI job (known to fail until https://github.com/ARMmbed/mbedtls/pull/2636 lands in the development branch) and USE_PSA_CRYPTO, which doesn't work on the API branch.

CI failure is ABI job (known to fail until https://github.com/ARMmbed/mbedtls/pull/2636 lands in the development branch) and `USE_PSA_CRYPTO`, which doesn't work on the API branch.
Sign in to join this conversation.
Reviewers
No reviewers
athoelke
Patater
Labels
Clear labels   
api-spec
Issue or PR about the PSA specifications

  
bug
Something isn't working

  
CLA requested

   
CLA valid

   
compliance
Discrepancy between the library and the PSA Crypto API specification

  
DO NOT MERGE
The PR is not intended to be merged (yet). Usually used for a review of worked in progress.

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
feature branch
This issue or PR is for a short-term feature branch which doesn't have its own label.

  
good first issue
Good for newcomers

  
HEADER FORMATTING ISSUE

   
help wanted
Extra attention is needed

  
Information

   
invalid
This doesn't seem right

  
Jira status: AWAITING REVIEW

   
Jira status: CLOSED

   
Jira status: DONE

   
Jira status: IN PROGRESS

   
Jira status: IN REVIEW

   
Jira status: OPEN

   
Jira status: REOPENED

   
Jira status: RESOLVED

   
mirrored

   
mirror internally

   
needs: backports
Needs backports to Mbed TLS branches

  
needs: ci
Needs a passing full CI run

  
needs: design review

   
needs: preceding PR
Requires another PR to be merged first

  
needs: review
The pull request is ready for review. This generally means that it has no known issues.

  
needs: work
The pull request needs rework before it can be merged.

  
notabug
The bug is not considered to be a defect, but by design.

  
open for community contribution

   
Product Backlog

   
question
Further information is requested

  
ready for merge
Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only.

  
security
Security expertise requested in review

  
stale
This PR is not being actively worked on. It can be considered abandoned work.

  
type: bug

   
type: enhancement

   
type: question

   
web pages
The /docs directory in the master branch is shown on https://armmbed.github.io/mbed-crypto

  
wontfix
The cost of fixing the bug or managing it in the bug list is not justified.

  
writing
Documentation wording and formatting

No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
Milestone
Clear milestone
No items
No milestone
api-1.0-beta-3
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-ARMmbed/mbed-crypto!102
No description provided.