crypto: Fix reading keyfile in bd_crypto_luks_change_key #1158
Conversation
📝 WalkthroughWalkthroughFixes buffer usage in LUKS key-change code when the new key context is a KEYFILE (reads from new-key buffers and frees the correct buffer) and adds tests exercising passphrase- and keyfile-based key changes and opens/closes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/crypto_test.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/crypto.c (1)
1736-1749: Fix critical buffer cleanup bug inbd_crypto_luks_change_keyerror path (ncontext KEYFILE read failure).When
crypt_keyfile_device_read()fails for the new context (line 1737), the error handler incorrectly checkscontext->typeto freenkey_buf, which:
- Never frees
key_bufwhen the old context is KEYFILE (memory and key material leak)- Uses the wrong condition for
nkey_bufcleanupThis diverges from the correct pattern in the success path (lines 1765-1768) and matches the bug pattern in
bd_crypto_luks_add_key(lines 1534-1535).Proposed fix
if (ret != 0) { g_set_error (&l_error, BD_CRYPTO_ERROR, BD_CRYPTO_ERROR_KEYFILE_FAILED, "Failed to load key from file '%s': %s", ncontext->u.keyfile.keyfile, strerror_l (-ret, c_locale)); crypt_free (cd); - if (context->type == BD_CRYPTO_KEYSLOT_CONTEXT_TYPE_KEYFILE) - crypt_safe_free (nkey_buf); + if (context->type == BD_CRYPTO_KEYSLOT_CONTEXT_TYPE_KEYFILE) + crypt_safe_free (key_buf); + crypt_safe_free (nkey_buf); bd_utils_report_finished (progress_id, l_error->message); g_propagate_error (error, l_error); return FALSE; }
🧹 Nitpick comments (1)
tests/crypto_test.py (1)
634-657: Add explicitcrypto_luks_closeafter opening withkctxto keep the test self-contained.The new assertions improve coverage for KEYFILE
ncontext, but the finalBlockDev.crypto_luks_open(..., kctx)isn’t closed in-test (relies on cleanup), which can leave state behind if later assertions are added or failures occur mid-test.Proposed tweak
# keyfile should work succ = BlockDev.crypto_luks_open(self.loop_devs[0], "libblockdevTestLUKS", kctx) self.assertTrue(succ) + + succ = BlockDev.crypto_luks_close("libblockdevTestLUKS") + self.assertTrue(succ)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/plugins/crypto.ctests/crypto_test.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/crypto_test.py (1)
src/python/gi/overrides/BlockDev.py (2)
crypto_luks_open(315-316)CryptoKeyslotContext(284-300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (42)
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_udisks
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_blivet
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: symbols
- GitHub Check: Analyze (python)
- GitHub Check: compilation (clang-17)
- GitHub Check: blivet-tests
- GitHub Check: compilation (clang-18)
- GitHub Check: compilation (clang-16)
- GitHub Check: compilation (gcc-10)
- GitHub Check: compilation (clang-15)
- GitHub Check: compilation (gcc-12)
- GitHub Check: compilation (gcc-11)
- GitHub Check: udisks-build
- GitHub Check: compilation (clang-14)
- GitHub Check: compilation (gcc-14)
- GitHub Check: compilation (gcc-13)
- GitHub Check: Analyze (cpp)
- GitHub Check: csmock
Copy-paste bug, we need to make sure save the new passphrase into the correct buffer.
vojtechtrefny
force-pushed
the
master_change-key-fix
branch
from
6c29e6b to
5b02aaf
Compare
1 participant
Copy-paste bug, we need to make sure save the new passphrase into the correct buffer.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.