[Bug 2474] Enabling ECDSA in PKCS#11 support for ssh-agent

[bugzilla-daemon at bugzilla.mindrot.org]

https://bugzilla.mindrot.org/show_bug.cgi?id=2474

--- Comment #14 from Dmitry S. <dsavints at gmail.com> ---
(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.