Skip to content

Wallet package fixes and code hardening - #297

Open
popenta wants to merge 4 commits into
mainfrom
wallet-package-fixes
Open

popenta wants to merge 4 commits into
mainfrom
wallet-package-fixes

Conversation

@popenta

@popenta popenta commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Wallet hardening: fixes from the security review of multiversx_sdk.wallet

The changes stay within multiversx_sdk/wallet. Public signatures change only by adding optional parameters, but stricter validation now rejects some inputs that were accepted before. See Behaviour changes.

Changes

Key derivation

  • NFKD normalization.
    • core.mnemonic_to_bip39seed() normalizes the mnemonic and the passphrase to NFKD before PBKDF2. Mnemonic.check() already normalized internally, but the seed was computed from the raw text, so some accepted mnemonics derived non-standard addresses.
    • Mnemonic.__init__ stores the normalized text.
    • UserWallet.from_mnemonic() encrypts the normalized text.
  • Address index validation. core.bip39seed_to_secret_key() validates the address index: an integer (not bool) with 0 ≤ index < 2^31, otherwise InvalidAddressIndexError. The partial check in Mnemonic.derive_key() was removed in favour of this one.
  • from_wallet index. UserSigner.from_wallet(path, password, address_index=None) passes the index through to UserWallet.load_secret_key().
  • Default index. UserWallet.load_secret_key() defaults the index to 0 only when it is None (before: address_index or 0), so falsy values of the wrong type (False, 0.0) reach the validation above instead of silently selecting account 0.

Keystore files

  • Identity checks. UserWallet.decrypt_secret_key():

    • requires the address and bech32 fields
    • requires the decrypted data to be 32 or 64 bytes (before, it was padded or truncated)
    • checks the embedded public key, address (hex) and bech32 against the derived public key; the bech32 comparison is by public key, so any HRP is accepted

    A tampered IV now gets caught as well, since it changes the decrypted key. This mirrors mx-sdk-go's LoadPrivateKeyFromJsonFile.

  • Scrypt bounds. decryptor.decrypt() checks the scrypt parameters before running the KDF:

    • n a power of 2 in [2, 2^18]
    • r in [1, 8]
    • p in [1, 4]
    • dklen == 32
    • all of them int
    • the IV must be 16 bytes

    The worst case allowed is about 256 MiB of memory. New constants are in crypto/constants.py.

  • Constant-time MAC. The MAC is checked with HMAC.verify() (constant-time) instead of !=.

PEM files

  • Strict parsing.
    • PemEntry.from_text_all() groups each entry by its BEGIN and END lines instead of splitting on -----. It checks that the labels match, rejects empty entries and text outside entries, and decodes base64 with validate=True.
    • Errors raise InvalidPemError(ValueError). The underlying error is not chained, since it could reveal bits of the secret content.
    • to_text() rejects labels that would break the file structure (line breaks, -----) and labels that would not survive a save / load cycle (ending with -, or with leading or trailing whitespace). The parser stays lenient about the trailing delimiter and surrounding whitespace.
  • Identity checks. UserPEM checks that the embedded public key and a bech32 label match the secret key. Labels that aren't addresses are still accepted.
  • Index checks. UserPEM and ValidatorPEM reject negative and out-of-range indices with IndexError (assert_index_is_valid).

BLS (validator keys)

  • Zero key and key length. ValidatorSecretKey rejects the all-zero key. BLSFacade.generate_public_key() and compute_message_signature() return b"" for any private key that is not exactly 32 bytes or is all-zero, instead of calling into the native library. The zero key panics in Go and kills the process. Older builds of the library (e.g. libbls.so) only read the first 32 bytes of the key, so longer keys starting with 32 zero bytes crashed as well, and other longer keys were accepted. libbls is not modified.
  • Signing errors. ValidatorSecretKey.sign() raises CannotSignError, and generate_public_key() raises InvalidSecretKeyError, when the native library returns an empty result.
  • Error type. ValidatorPublicKey raises InvalidPublicKeyLengthError instead of InvalidSecretKeyLengthError.

User keys

  • UserSecretKey raises TypeError for anything other than bytes or bytearray, and copies the buffer.
  • UserSecretKey.__eq__ uses hmac.compare_digest.

New errors in wallet/errors.py: InvalidKeystoreFileError, InvalidPemError, InvalidSecretKeyError.

Behaviour changes

Area Before After
Mnemonic with NBSP / ideographic space / fullwidth letters Non-standard address Standard BIP-39 address (same as js-core)
Secret-key keystore without address / bech32 Loaded InvalidKeystoreFileError
Keystore whose fields don't match the key Loaded (wrong account) InvalidKeystoreFileError
Keystore scrypt params out of bounds, IV ≠ 16 bytes Ran as given InvalidKeystoreFileError
PEM index -1 Last entry IndexError
PEM label my-validator myvalidator my-validator
Saving a PEM whose label ends with - or has surrounding whitespace Saved, label altered on reload InvalidPemError
Malformed PEM (unmatched BEGIN/END, empty entry, bad base64) Loaded or misaligned InvalidPemError (a ValueError)
BLS sign() failure (invalid key, empty message) b"" CannotSignError
Zero BLS secret key Process crash InvalidSecretKeyError
BLSFacade with a private key longer than 32 bytes On older libbls builds: process crash (zero-prefixed) or extra bytes ignored b"" on all platforms
Address index ≥ 2^31 / True struct.error / index 1 InvalidAddressIndexError
Address index False / 0.0 when loading a mnemonic keystore Account 0 InvalidAddressIndexError
ValidatorPublicKey wrong length InvalidSecretKeyLengthError InvalidPublicKeyLengthError

Testing

  • New and extended tests:
    • mnemonic_test.py: NFKD cases, Trezor BIP-39 vectors, passphrase normalization, address index bounds, largest index cross-checked against ed25519-hd-key
    • user_test.py: keystore identity checks, PEM identity and index checks, UserSecretKey type, copy and equality, UserSigner.from_wallet index, address index of the wrong type (including falsy values)
    • validator_test.py: zero key, signing failures, key altered after construction (zero and longer zero-prefixed buffers), PEM indices, ValidatorPublicKey length error
    • pem_entry_test.py: malformed structure, bad encoding, labels (including labels that would not survive a save / load cycle)
    • libraries/bls_facade_test.py: the zero key and longer zero-prefixed keys, run in a subprocess, no longer crash; keys of the wrong length are rejected
    • new crypto/decryptor_test.py: scrypt bounds, IV length, MAC
  • Each fix was checked against a copy of the code with that fix removed: the corresponding new tests fail there.
  • The BLS crash with longer zero-prefixed keys was reproduced against libbls.so (Linux amd64, in Docker) and libbls.dylib (x86_64); the wallet tests pass on Linux amd64 with the fix.
  • pytest -m "not networkInteraction and not mainnet": 761 passed, 12 skipped.
  • flake8 and pyright are clean on the changed files.

@popenta popenta self-assigned this Oct 30, 2026
@github-actions

github-actions Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  multiversx_sdk/abi
  bigint_value.py
  bool_value.py
  code_metadata_value.py
  enum_value.py
  interface.py
  option_value.py
  optional_value.py
  small_int_values.py
  multiversx_sdk/core
  address.py
  errors.py
  interfaces.py
  transaction.py
  transaction_computer.py
  multiversx_sdk/network_providers
  errors.py
  interface.py
  proxy_network_provider.py
  resources.py
  transaction_awaiter.py
  multiversx_sdk/testutils
  mock_network_provider.py
  multiversx_sdk/wallet
  errors.py
  interfaces.py
  user_pem.py
  user_signer.py
  validator_keys.py
  validator_signer.py
Project Total  

The report is truncated to 25 files out of 68. To see the full report, please visit the workflow summary page.

This report was generated by python-coverage-comment-action

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The BLS zero-key guard can be bypassed by longer inputs with a zeroed 32-byte prefix, allowing the native library to abort the process.

3 open issues
What changed in this PR

Hardens the wallet package’s key derivation, keystore and PEM loading, and signing behavior through stricter validation and identity checks.

Changes:

  • Normalize BIP-39 inputs and validate address indices.
  • Strengthen wallet-file parsing, secret-key handling and BLS error handling, with expanded regression coverage.
  • Bump the SDK to 3.1.0 and pin cryptography to 50.0.2.
File Description
requirements.txt Updates the cryptography pin.
pyproject.toml Updates the SDK version and dependency pin.
multiversx_sdk/​wallet/​validator_test.py Tests validator-key validation and signing failures.
multiversx_sdk/​wallet/​validator_pem.py Validates PEM entry indices.
multiversx_sdk/​wallet/​validator_keys.py Validates secret-key buffers and native-library results.
multiversx_sdk/​wallet/​user_wallet.py Normalizes stored mnemonics and verifies keystore identity.
multiversx_sdk/​wallet/​user_test.py Covers wallet, PEM, signer and user-key hardening.
multiversx_sdk/​wallet/​user_signer.py Adds optional wallet address-index forwarding.
multiversx_sdk/​wallet/​user_pem.py Checks entry indices and account identity.
multiversx_sdk/​wallet/​user_keys.py Copies key buffers and uses constant-time equality.
multiversx_sdk/​wallet/​pem_entry.py Adds structured parsing, strict decoding and label checks.
multiversx_sdk/​wallet/​pem_entry_test.py Tests malformed PEM entries and label handling.
multiversx_sdk/​wallet/​mnemonic.py Stores normalized mnemonic text.
multiversx_sdk/​wallet/​mnemonic_test.py Tests normalization, reference vectors and index bounds.
multiversx_sdk/​wallet/​libraries/​bls_facade.py Adds a zero-key guard before native public-key generation.
multiversx_sdk/​wallet/​libraries/​bls_facade_test.py Adds subprocess coverage for zero-key crash prevention.
multiversx_sdk/​wallet/​errors.py Defines keystore, PEM and secret-key errors.
multiversx_sdk/​wallet/​crypto/​decryptor.py Bounds scrypt parameters, validates IV length and verifies MACs.
multiversx_sdk/​wallet/​crypto/​decryptor_test.py Tests decryption validation and MAC failures.
multiversx_sdk/​wallet/​crypto/​constants.py Defines accepted scrypt parameter bounds.
multiversx_sdk/​wallet/​core.py Normalizes BIP-39 inputs and validates derivation indices.

Review effort: Max

Comment on lines +29 to +30
if private_key == bytes(VALIDATOR_SECRETKEY_LENGTH):
return b""

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The facade now rejects any private key that isn't exactly 32 non-zero bytes before calling the native library, for both public key generation and signing.

Comment on lines +99 to +101
# Only the delimiters are removed, so that dashes within the label are kept.
# The trailing delimiter is handled leniently (e.g. "---" instead of "-----"), as before: such files exist in the wild.
return line[len(prefix) :].rstrip("-").strip()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BEGIN my-key- / END my-key equivalence is by design, since both lines parse to the same label.

secret_key = UserWallet.load_secret_key(path, password)
def from_wallet(cls, path: Path, password: str, address_index: Optional[int] = None) -> "UserSigner":
# "address_index" is only used for keystore files holding a mnemonic (same as "UserWallet.load_secret_key").
secret_key = UserWallet.load_secret_key(path, password, address_index)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_secret_key() now defaults the index only when it is None, so False, 0.0 and other falsy values raise InvalidAddressIndexError.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants