Sync to kth/0.81.0 + explicit_bzero on sensitive wallet wrappers#8
Sync to kth/0.81.0 + explicit_bzero on sensitive wallet wrappers#8fpelliccioni merged 1 commit intomasterfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 50 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis PR updates the Conan dependency version to kth/0.81.0 and systematically modifies Python C-API wrapper functions across chain and wallet modules to pass hash and struct objects by pointer instead of by value, adding secure memory zeroing in sensitive wallet operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fpelliccioni
force-pushed
the
feature/sync-kth-0.81
branch
from
7787a33 to
1e142ef
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/hd_private.cpp (1)
294-303:⚠️ Potential issue | 🟠 MajorDrop
constqualifier fromresultvariables to fix undefined behavior in secure-zero operations.Writing to
auto const resultobjects via cast-away-const pointers is undefined behavior per C++[dcl.type.cv]. The pattern appears in fourhd_private.cppgetters (lines 299, 310, 394, 414) andwallet_data.cpp::encrypted_seed(line 133): each declares a const local, then callskth_core_secure_zero((void*)&result, ...)to overwrite it—a UB violation that allows the compiler to optimize away the zeroing entirely, defeating the security guarantee.Change
auto const result = ...toauto result = ...in:
kth_py_native_wallet_hd_private_secret(line 299)kth_py_native_wallet_hd_private_to_hd_key(line 310)kth_py_native_wallet_hd_private_chain_code(line 394)kth_py_native_wallet_hd_private_point(line 414)kth_py_native_wallet_wallet_data_encrypted_seed(wallet_data.cpp, line 133)Suggested changes
- auto const result = kth_wallet_hd_private_secret(self_handle); + auto result = kth_wallet_hd_private_secret(self_handle);Apply identical change to the four other affected functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/hd_private.cpp` around lines 294 - 303, The local variables declared as "auto const result" in the HD-private getters are later passed to kth_core_secure_zero via a casted pointer, which is undefined behavior; change the declarations to non-const (e.g., replace "auto const result = ..." with "auto result = ...") in kth_py_native_wallet_hd_private_secret, kth_py_native_wallet_hd_private_to_hd_key, kth_py_native_wallet_hd_private_chain_code, kth_py_native_wallet_hd_private_point and similarly in wallet_data's encrypted_seed so the secure-zero call legally overwrites the stack object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/wallet/hd_private.cpp`:
- Around line 294-303: The local variables declared as "auto const result" in
the HD-private getters are later passed to kth_core_secure_zero via a casted
pointer, which is undefined behavior; change the declarations to non-const
(e.g., replace "auto const result = ..." with "auto result = ...") in
kth_py_native_wallet_hd_private_secret,
kth_py_native_wallet_hd_private_to_hd_key,
kth_py_native_wallet_hd_private_chain_code,
kth_py_native_wallet_hd_private_point and similarly in wallet_data's
encrypted_seed so the secure-zero call legally overwrites the stack object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3450e575-9d98-4279-9fab-0e7fc40e87a5
📒 Files selected for processing (18)
conanfile.pyinclude/kth/py-native/utils.hsrc/chain/double_spend_proof_spender.cppsrc/chain/get_blocks.cppsrc/chain/get_headers.cppsrc/chain/header.cppsrc/chain/output_point.cppsrc/chain/point.cppsrc/chain/script.cppsrc/chain/stealth_compact.cppsrc/chain/token_data.cppsrc/chain/transaction.cppsrc/wallet/ec_private.cppsrc/wallet/ec_public.cppsrc/wallet/hd_private.cppsrc/wallet/hd_public.cppsrc/wallet/payment_address.cppsrc/wallet/wallet_data.cpp
fpelliccioni
force-pushed
the
feature/sync-kth-0.81
branch
from
1e142ef to
35a96c8
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Picks up the kth 0.81.0 C-API change (#285) that passes value_struct params by `const*` in the safe variant. Regenerated py-native wrappers now forward `&local` to every `kth_xxx_t const*` entry point, and call `kth_core_secure_zero` on their stack-local value_struct buffers after the C-API has consumed them. Scrubbing covers both sides of the wallet API surface: - Input ctors / setters that memcpy key bytes into `kth_hash_t secret`, `kth_wif_*_t wif`, `kth_hd_key_t private_key`, `kth_encrypted_seed_t value` wipe those locals on the way out. - Getters returning value_struct (`ec_private.secret()`, `hd_private.to_hd_key()`, `wallet_data.encrypted_seed()`, ...) build the Python bytes, scrub `result`, then return. - Non-ctor methods that take a sensitive param (today: `chain.script.create_endorsement`) also scrub the wrapper's stack-local after the C-API call. Sensitivity is detected per-param via name + type heuristics (`_is_sensitive_value_struct`) so methods like `create_endorsement` in a non-wallet class still get scrubbed. Builds the `kth_py_native_borrowed_parent_dtor` declaration is unchanged; the previously-local `kth_py_native_explicit_bzero` helper was removed in favour of the C-API-side `kth_core_secure_zero` so every high-level binding shares the same portable primitive. No functional change for non-wallet classes; 171 tests green.
fpelliccioni
force-pushed
the
feature/sync-kth-0.81
branch
from
1f4cbec to
8570ba0
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
1 participant
Summary
kth/0.81.0to pick up the C-API change that passes value_struct params byconst*in the safe variant (refactor(c-api): value_struct params by const* in the safe variant kth#285).&localto everykth_xxx_t const*entry point.kth_py_native_explicit_bzero(void*, size_t)helper (explicit_bzero on glibc/BSD/macOS, SecureZeroMemory on Windows, volatile-loop fallback).ec_private,hd_private,wallet_datanow scrub their stack-local value_struct buffers after the C-API has consumed them — both input ctors/setters and output getters.Dependency
kth/0.81.0published to the conan remote before CI can resolve the dep.Test plan
kth/0.81.0package.kth/0.81.0hits the remote.Summary by CodeRabbit
Release Notes
Dependencies
Security
Documentation