Conversation
…d OpenSSL structs and update the implementation to not use those functions
|
No worries. I'll give it a proper review! Thanks for starting with this anyway! |
pemensik
left a comment
pemensik
left a comment
There was a problem hiding this comment.
I have found some things to change a bit. But overall great work so far, thank for you that!
| #endif /* ! HAVE_B32_PTON */ | ||
|
|
||
| void | ||
| ldns_swap_bytes(unsigned char *buf, size_t len) |
There was a problem hiding this comment.
I would propose to implement endian swapping memcpy. I think all uses of this function first use memcpy, then do swap_bytes on the same data. Could it perhaps swap bytes into target buffer instead and use memcpy if that is not necessary? That way it would not require temp variable usage.
But I am not sure, should it use instead BN_lebin2bn or BN_bin2bn ?
| offset += SHA_DIGEST_LENGTH; | ||
|
|
||
| memcpy(P, key+offset, length); | ||
| ldns_swap_bytes(P, length); |
| params[0] = | ||
| OSSL_PARAM_construct_BN(OSSL_PKEY_PARAM_FFC_Q, Q, SHA_DIGEST_LENGTH); | ||
|
|
||
| params[1] = OSSL_PARAM_construct_BN(OSSL_PKEY_PARAM_FFC_P, P, length); |
There was a problem hiding this comment.
This may have used OSSL_PARAM_set_BN with combination of BN_bin2bn IMO. Is there a reason it were not used for it and own swap implemented?
There was a problem hiding this comment.
Yes. This is an unfortunate requirement for OpenSSL3, but all OSSL_PARAMs must be stored in native byte order instead of network byte order. BIGNUMs created by BN_bin2bn and similar functions store the data in network byte order. This effectively makes OSSL_PARAM_set_BN difficult to use on little-endian systems.
I suppose it would be possible to call BN_lebin2bn to create a backwards BIGNUM on little endian systems, but it's a bit strange because you're calling a function designed to take little endian data on big endian data. It's also weird because you'd have to conditionally call the function based on the endianness of the system.
However, I recall trying this when doing work for libssh2 and finding that it didn't work properly. The only solution that worked for me at the time was swapping the buffer and calling OSSL_PARAM_construct_BN.
|
I would propose to reuse existing solution from unbound code. There seems to be used OSSL_PARAM_BLD_push_BN and BN_bin2bn and dropped sldns_key_buf2dsa_raw function for newer OpenSSL. Anyone else had found any time to review this proposal? |
|
Okay, I've started review and pushed some fixes (to make the tests work) on the review/Michael-Panic-openssl-3 branch. @Michael-Panic I have created a PR #1 on your branch for you to review and merge the updates and fixes |
3 participants
A few days ago, I volunteered to contribute an OpenSSL 3 implementation that doesn't use any functions or types deprecated by OpenSSL 3 in #243. I realized as I was finishing this that I don't actually have a good way of testing this, so it probably shouldn't be merged as-is.
I tried running ./tests/test-all.sh, but many of the tests failed, even without my changes. I suspect my computer isn't set up to properly run the tests, plus there were some comments in there about the tests really only working on openbsd.
But I'm willing to help however I can to figure out how to get this all tested and merged.