-
Notifications
You must be signed in to change notification settings - Fork 6.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Auth crash fix #6698
Auth crash fix #6698
Conversation
@@ -241,7 +241,6 @@ static int nss_aes_operation(CK_ATTRIBUTE_TYPE op, | |||
ret = PK11_DigestFinal(ectx, | |||
(unsigned char*)out_tmp.c_str()+written, &written2, | |||
out_tmp.length()-written); | |||
PK11_DestroyContext(ectx, PR_TRUE); | |||
if (ret != SECSuccess) { | |||
PK11_DestroyContext(ectx, PR_TRUE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch ! You could just remove this one instead so there is only one call ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dachary nop, PK11_DestroyContext() should be called before this function exit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dachary it's my fault that i misunderstand, you are right, just removing PK11_DestroyContext() is enough. I have just updated pull request.
Signed-off-by: Dunrong Huang <[email protected]>
In this case(e.g. user passes wrong key), attempts to call the CryptoKey.ckh will lead to a segfault. This patch fixes crash issue like following: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fffed10e700 (LWP 25051)] 0x00007ffff59896c6 in CryptoKey::encrypt (this=0x7fffed10d4f0, cct=0x555555829c30, in=..., out=..., error=0x7fffed10d440) at auth/cephx/../Crypto.h:110 110 return ckh->encrypt(in, out, error); (gdb) bt at auth/cephx/../Crypto.h:110 at auth/cephx/CephxProtocol.h:464 Signed-off-by: Dunrong Huang <[email protected]>
254d781
to
a7f520c
Compare
This passed a test run. I'd still like to see a simple test before merging it, though, since this failure/error path is very easy to break later. I think you can put a simple test case in, say, src/test/mon/misc.sh, and verify it by running the script with a ./vstart.sh environment (or via make check). |
Signed-off-by: Dunrong Huang <[email protected]>
hi @liewegas , I just add a test to detect the error as you suggested, and verify it in different case, could you have a look at and merge it if there is nothing wrong? |
auth: fix crash when bad keyring is passed Reviewed-by: Sage Weil <[email protected]>
@mtanski can you make a bug for backporting it? (reference this pr, set the status to "Pending Backport" and the backport field to infernalis (and maybe hammer too)) |
No description provided.