Conversation
- Replaced python-ecdsa dependency with cryptography (>=43.0.0) - Rewrote secp256k1_ecdsa.py to use cryptography's EC module - CVE-2024-23342 is a Minerva timing attack in python-ecdsa that affects all versions. The maintainers stated they won't fix it as it's inherent to pure Python implementations - The cryptography library provides constant-time implementations that eliminate this vulnerability - All existing tests pass with the new implementation - Bumped version to 0.12.0 (breaking change due to dependency change) - Updated mypy.ini to remove ecdsa configuration
- Remove unused imports (hashlib, serialization) - Reorder imports per isort - Apply black formatting
There was a problem hiding this comment.
Pull request overview
This pull request migrates from the python-ecdsa library to the cryptography library to fix CVE-2024-23342, a Minerva timing attack vulnerability in ECDSA signature generation. The python-ecdsa maintainers have stated they will not fix this vulnerability as it's inherent to pure Python implementations. The cryptography library provides constant-time implementations that eliminate this security concern.
Changes:
- Replaced
ecdsadependency withcryptography>=43.0.0in pyproject.toml - Completely rewrote secp256k1_ecdsa.py to use the cryptography library's EC module
- Removed ecdsa-related mypy configuration
- Updated CHANGELOG.md with breaking change notice
- Bumped version to 0.12.0
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updated dependency from ecdsa to cryptography>=43.0.0 and bumped version to 0.12.0 |
| poetry.lock | Updated lock file with cryptography packages (versions 43.0.3 and 45.0.7 for different Python versions) |
| mypy.ini | Removed ecdsa-specific mypy configuration that's no longer needed |
| aptos_sdk/secp256k1_ecdsa.py | Complete rewrite using cryptography library while maintaining the same API surface for backward compatibility |
| CHANGELOG.md | Added 0.12.0 release notes documenting the security fix and breaking change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Use deterministic ECDSA (RFC 6979) with SHA3-256 | ||
| signature_der = self.key.sign(data, ec.ECDSA(hashes.SHA3_256())) |
There was a problem hiding this comment.
The comment on line 95 states that deterministic ECDSA (RFC 6979) is being used, but this should be verified. In the cryptography library, the default ec.ECDSA() algorithm uses RFC 6979 for deterministic signatures, which is correct. However, it would be good to add a test that verifies the signature is deterministic by checking that signing the same message multiple times produces the same signature.
| signature = cast(Signature, signature) | ||
| self.key.verify(signature.data(), data) | ||
| sig_bytes = signature.data() | ||
|
|
There was a problem hiding this comment.
The verify method assumes signature bytes are exactly 64 bytes (32 for r, 32 for s) but doesn't validate the length before slicing at lines 188-189. If a signature with incorrect length is passed, this could lead to incorrect parsing. Consider adding a length check similar to what's done in Signature.deserialize.
| # Ensure the raw signature is exactly 64 bytes (32 bytes for r, 32 bytes for s) | |
| if len(sig_bytes) != Signature.LENGTH: | |
| raise ValueError("Signature length mismatch") |
2 participants
Description
Test Plan
Related Links