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.

Sign up for GitHub

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

Jump to bottom

ECPKeypair::ECDSA_sign_hash should be deprecated since not supported by security-critical APIs in Java #18430

Closed
tcarmelveilleux opened this issue May 13, 2022 · 1 comment · Fixed by #20078
Closed

ECPKeypair::ECDSA_sign_hash should be deprecated since not supported by security-critical APIs in Java #18430

tcarmelveilleux opened this issue May 13, 2022 · 1 comment · Fixed by #20078
Assignees
@tcarmelveilleux
Labels
android security V1.0

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

  • Spec only defines Crypto_Sign(privateKey, message) primitive
  • CASE and all other areas where signatures are generated according to the spec use the message variant
  • The sign_hash operation is a low-level consideration of crypto APIs and is sometimes not exposed by platform crypto APIs. For example, in Java's java.security.Signature, including on Android, there is no way to get a signature by providing a pre-computed digest. The API expects digesting to be done by the signing operation.

Overall, replacement of this primitive is trivial wherever used, and would reduce reliance on native implementation that prevent going to hardware/OS-aided key storage, especially where Java is concerned.

Proposed Solution

  • Mark ECDSA_sign_hash as deprecated
  • Replace usages of ECDSA_sign_hash with ECDSA_sign_message where possible
@tcarmelveilleux tcarmelveilleux self-assigned this May 13, 2022
@bluebin14
Copy link
Contributor

ECDSA_sign_hash and ECDSA_validate_hash_signature are useful in various scenarios such as signing non-contiguous memory blocks without the need to concatenate them all into one, e.g. during firmware update. The Java equivalent is NONEwithECDSA.

tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Jun 28, 2022
@tcarmelveilleux
Remove duplicate P256Keypair::ECDSA_sign_hash code
2a3b0b3 
- The ECDSA_sign_hash method is a near identical copy of of ECDSA_sign_msg,
  that takes a raw hash.
- This is problematic since some platforms, like Android, cannot directly sign
  a pre-computed hash with OS-aided APIs, and overall this is not consistent
  with signature APIs that work on messages, and where a digest is an internal
  implementation detail.
- Overall, the method adds little value and prevents easy transition to different
  signing algorithms over time if the hash assumption is kept

Fixes project-chip#18430

This PR:

- Removes the sign_hash API
- Replaces its usage throughout the SDK
- Updates all tests
- Leaves the ECDSA_verify_hash_signature (since it's only used in one place,
  already in native code, and always against raw public keys)

Testing done:

- Cert tests still pass, including device attestation during commissioning
- Unit tests still pass including updated unit tests
@tcarmelveilleux tcarmelveilleux mentioned this issue Jun 28, 2022
Merged
tcarmelveilleux added a commit that referenced this issue Jun 29, 2022
@tcarmelveilleux @restyled-commits
Remove duplicate P256Keypair::ECDSA_sign_hash code (#20078)
81c7f2a 
* Remove duplicate P256Keypair::ECDSA_sign_hash code

- The ECDSA_sign_hash method is a near identical copy of of ECDSA_sign_msg,
  that takes a raw hash.
- This is problematic since some platforms, like Android, cannot directly sign
  a pre-computed hash with OS-aided APIs, and overall this is not consistent
  with signature APIs that work on messages, and where a digest is an internal
  implementation detail.
- Overall, the method adds little value and prevents easy transition to different
  signing algorithms over time if the hash assumption is kept

Fixes #18430

This PR:

- Removes the sign_hash API
- Replaces its usage throughout the SDK
- Updates all tests
- Leaves the ECDSA_verify_hash_signature (since it's only used in one place,
  already in native code, and always against raw public keys)

Testing done:

- Cert tests still pass, including device attestation during commissioning
- Unit tests still pass including updated unit tests

* Restyled by clang-format

* Remove missed removals

* Apply review comments

Co-authored-by: Restyled.io <commits@restyled.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tcarmelveilleux tcarmelveilleux

Labels
android security V1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove duplicate P256Keypair::ECDSA_sign_hash code tcarmelveilleux/connectedhomeip
2 participants
@tcarmelveilleux @bluebin14