ICP: Fix null pointer dereference and use after free #9659
Conversation
In gcm_mode_decrypt_contiguous_blocks(), if vmem_alloc() fails, bcopy is called with a NULL pointer destination and a length > 0. This results in undefined behavior. Further ctx->gcm_pt_buf is freed but not set to NULL, leading to a potential write after free and a double free due to missing return value handling in crypto_update_uio(). The code as is may write to ctx->gcm_pt_buf in gcm_decrypt_final() and may free ctx->gcm_pt_buf again in aes_decrypt_atomic(). The fix is to slightly rework error handling and check the return value in crypto_update_uio(). Signed-off-by: Attila Fülöp <[email protected]>
There was a problem hiding this comment.
Thanks for running down this failure. The fix here makes good sense, my only question is do you know the size and flags passed to the failed vmem_alloc()? I ask because normallly these allocations are not allowed to fail. Did this issue manifest itself as a hang?
|
Well, I found this bug while reading the code, never managed to trigger it. And I agree that it is mostly academic for in-kernel use, but user space consumers of libicp may be able to hit this. This, btw also applies to #9660. |
Codecov Report
@@ Coverage Diff @@
## master #9659 +/- ##
==========================================
+ Coverage 79.32% 79.38% +0.05%
==========================================
Files 418 418
Lines 123544 123548 +4
==========================================
+ Hits 98003 98078 +75
+ Misses 25541 25470 -71
Continue to review full report at Codecov.
|
|
The failing resilver test is most likely not related to this change and code coverage can't be increased without making |
|
Illumos issue 12047 |
behlendorf
added
Status: Accepted
Motivation and Context
Bugfix.
Description
In
gcm_mode_decrypt_contiguous_blocks(), ifvmem_alloc()fails,bcopy()is called with a null pointer destination and a length > 0. This results in undefined behavior. Further ctx->gcm_pt_buf is freed but not set toNULL, leading to a potential write after free and a double free due to missing return value handling incrypto_update_uio(). The code as is may write toctx->gcm_pt_bufin gcm_decrypt_final() and may freectx->gcm_pt_bufagain in aes_decrypt_atomic().The fix is to slightly rework error handling in
gcm_mode_decrypt_contiguous_blocks()and check the return value incrypto_update_uio().How Has This Been Tested?
As part of another branch this survived 6 hours of
zloop. Currentlyzloopfails with unmodified master within one hour, so can't usezloopto test this PR, sorry.Types of changes
Checklist:
Signed-off-by.