https://bugzilla.mindrot.org/show_bug.cgi?id=2474
--- Comment #14 from Dmitry S. <[email protected]> --- (In reply to Mathias from comment #12) > Created attachment 3095 [details] > Sixth iteration > > I've updated my patch with the fix from Dmitry [...] Thanks Mathias - but I wonder if the fix was properly propagated. In my patch, I had the following in ssh-pkcs11.c function pkcs11_ecdsa_wrap: + /* identify key object on smartcard */ + k11->keyid_len = keyid_attrib->ulValueLen; + if (k11->keyid_len > 0) { + k11->keyid = xmalloc(k11->keyid_len); + } in your latest ("Sixth iteration") patch I see the statements in different order: + /* identify key object on smartcard */ + if (k11->keyid_len > 0) { + k11->keyid_len = keyid_attrib->ulValueLen; + k11->keyid = xmalloc(k11->keyid_len); + } Is it a typo or have you done it for a reason that I'm missing? Should not we first extract the value from keyid_attrib->ulValueLen and assign it to k11->keyid_len and only then use it in the if condiftion for zero check? This is how it is done in pkcs11_rsa_wrap in master ov V_7_6_p1: https://github.com/openssh/openssh-portable/blob/V_7_6_P1/ssh-pkcs11.c#L324-L325 I believe in your code the k11->keyid_len is uninitialized and therefore can take arbitrary values leading to undefined behavior. Please let me know if I'm missing something here. If it is a bug, I wonder if we could add a test to catch it, so it would fail on the current patch and succeed with a fix? -- You are receiving this mail because: You are watching the assignee of the bug. _______________________________________________ openssh-bugs mailing list [email protected] https://lists.mindrot.org/mailman/listinfo/openssh-bugs
