Closed bencemali closed 4 months ago
Thanks for the PR! Yes, we could use OPENSSL_cleanse
when available.
A couple of minor things to make our CI checks pass:
Hi @bencemali, thanks for the PR. It's correct that liboqs
could call out to the OPENSSL_cleanse
when built against OpenSSL. I'm not an expert on OpenSSL inner workings, but it looks to me as if OPENSSL_cleanse
is implemented using the same "trick" (to avoid being optimized out) that we currently have in place: https://github.com/openssl/openssl/blob/openssl-3.3.0/crypto/mem_clr.c. On the other hand, memset_s
is (as I understand it) guaranteed to not be optimized out by a standard-compliant compiler: no tricks required. I agree that we should probably call out to OPENSSL_cleanse
if memset_s
is not available, but I'm not sure we should prefer OPENSSL_cleanse
over memset_s
if the latter is available.
Hi @bencemali, thanks for the PR. It's correct that
liboqs
could call out to theOPENSSL_cleanse
when built against OpenSSL. I'm not an expert on OpenSSL inner workings, but it looks to me as ifOPENSSL_cleanse
is implemented using the same "trick" (to avoid being optimized out) that we currently have in place: https://github.com/openssl/openssl/blob/openssl-3.3.0/crypto/mem_clr.c. On the other hand,memset_s
is (as I understand it) guaranteed to not be optimized out by a standard-compliant compiler: no tricks required. I agree that we should probably call out toOPENSSL_cleanse
ifmemset_s
is not available, but I'm not sure we should preferOPENSSL_cleanse
overmemset_s
if the latter is available.
Following up on this more concretely: could you please rework the logic so that we only use OPENSSL_cleanse
if neither memset_s
nor explicit_bzero
is available (similarly for SecureZeroMemory
on Windows).
OpenSSL guarantees that OPENSSL_cleanse
always works and is not optimized out, it is an industry-wide used, trusted, and reviewed solution. The motivation behind this pull request is to use that logic inside OpenSSL instead of platform-specific custom stuff implemented in liboqs.
If OPENSSL_cleanse
cannot be the first choice when available then we'd prefer to simply close this PR and have a custom patch in place where we use liboqs.
OpenSSL guarantees that OPENSSL_cleanse always works and is not optimized out, it is an industry-wide used, trusted, and reviewed solution.
For our "peace of mind", do you have a pointer documenting this guarantee, @bencemali ? https://www.openssl.org/docs/manmaster/man3/OPENSSL_cleanse.html seems a bit non-committal. What is true is that the openssl
code does not contain a single case of memset_s
but a myriad uses of memset
, including an interesting one here indicating some deliberation as to what function to be sensible in which case. Conceptually, I agree, though, if OQS relies on/is built using openssl
it would be prudent to use the same --particularly security-- mechanisms openssl
uses.
My two cents:
OpenSSL guarantees that
OPENSSL_cleanse
always works and is not optimized out, it is an industry-wide used, trusted, and reviewed solution.The motivation behind this pull request is to use that logic inside OpenSSL instead of platform-specific custom stuff implemented in liboqs. IfOPENSSL_cleanse
cannot be the first choice when available then we'd prefer to simply close this PR and have a custom patch in place where we use liboqs.
I understand that OpenSSL is a ubiquitous, trusted library. However, liboqs
doesn't default to wrapping OpenSSL code whenever possible. Notably, we don't use OpenSSL's SHA3 implementation by default.
For our "peace of mind", do you have a pointer documenting this guarantee, @bencemali ? https://www.openssl.org/docs/manmaster/man3/OPENSSL_cleanse.html seems a bit non-committal. What is true is that the
openssl
code does not contain a single case ofmemset_s
but a myriad uses ofmemset
, including an interesting one here indicating some deliberation as to what function to be sensible in which case. Conceptually, I agree, though, if OQS relies on/is built usingopenssl
it would be prudent to use the same --particularly security-- mechanismsopenssl
uses.
I agree that we could rely on OpenSSL here and wash our hands of the matter, but to me it seems like a regression: our fallback code is, as far as I can tell, implemented in exactly the same way as OPENSSL_cleanse
. (I actually wonder if we copied directly from OpenSSL back in the day?) We've made a conscious decision in the past to prefer memset_s
or explicit_bzero
over the generic C approach. I was following that precedent above when I suggested to also prefer memset_s
etc. over OPENSSL_cleanse
. To break with it requires a stronger rationale than "prefer OpenSSL whenever possible" (which we don't do in general).
I'm open to the idea to defaulting to OPENSSL_cleanse
whenever possible for simplicity. However, if we prefer OPENSSL_cleanse
over memset_s
etc., then we should prefer its approach when we're not building with OpenSSL. Hence, we should also remove the logic to rely on SecureZeroMemory
/ explicit_bzero
/ memset_s
and simply fall back on the generic volatile
function pointer code when OpenSSL is not available.
IMO, what we decide here sets precedent for common (especially OpenSSL-related) code decisions across the project, and we should be able to consistently apply whatever rationale we use for future decisions.
However, if we prefer OPENSSL_cleanse over memset_s etc., then we should prefer its approach when we're not building with OpenSSL.
Good points. This indeed then feels like a regression, particularly considering
our fallback code is, as far as I can tell, implemented in exactly the same way as OPENSSL_cleanse.
First, might it be worth while getting the OpenSSLs team's opinion on your "non-OpenSSL" cleansing code?
Second, this of course implies we accepted OPENSSL_cleanse
to be a "good" way to do things -- "just" with the drawback that OQS will not benefit automatically from further improvements to this as & if the code in openssl
for this routine changes -- which again speaks for using OPENSSL_cleanse
.
This argument does not sound as strong, though:
However, liboqs doesn't default to wrapping OpenSSL code whenever possible. Notably, we don't use OpenSSL's SHA3 implementation by default.
Well, there's been a reason for that and I hope it's not "just" what's documented (?):
These default choices have been made in order to optimize the default performance of all algorithms. Changing them implies performance penalties.
This kind of seems to imply OQS values performance over security. This may be intended but should be clearly documented.
For our "peace of mind", do you have a pointer documenting this guarantee, @bencemali ? https://www.openssl.org/docs/manmaster/man3/OPENSSL_cleanse.html seems a bit non-committal. What is true is that the
openssl
code does not contain a single case ofmemset_s
but a myriad uses ofmemset
, including an interesting one here indicating some deliberation as to what function to be sensible in which case. Conceptually, I agree, though, if OQS relies on/is built usingopenssl
it would be prudent to use the same --particularly security-- mechanismsopenssl
uses.
OpenSSL API docs about OPENSSL_cleanse
:
OPENSSL_cleanse() fills ptr of size len with a string of 0's. Use OPENSSL_cleanse() with care if the memory is a mapping of a file. If the storage controller uses write compression, then it's possible that sensitive tail bytes will survive zeroization because the block of zeros will be compressed. If the storage controller uses wear leveling, then the old sensitive data will not be overwritten; rather, a block of 0's will be written at a new physical location.
As this function is in OpenSSL's publicly available API and says that it will fill the buffer with zeros, I'd assume it's a good enough that it works. If it would not, it would be treated as a large security issue within OpenSSL itself.
As this function is in OpenSSL's publicly available API and says that it will fill the buffer with zeros, I'd assume it's a good enough that it works. If it would not, it would be treated as a large security issue within OpenSSL itself.
ACK. It's also the reason why OQS apparently also bases its implementation on this as per the statements above.
So we're down to deciding the question by @SWilson4 :
I'm open to the idea to defaulting to OPENSSL_cleanse whenever possible for simplicity. However, if we prefer OPENSSL_cleanse over memset_s etc., then we should prefer its approach when we're not building with OpenSSL. Hence, we should also remove the logic to rely on SecureZeroMemory / explicit_bzero / memset_s and simply fall back on the generic volatile function pointer code when OpenSSL is not available.
IMO, what we decide here sets precedent for common (especially OpenSSL-related) code decisions across the project, and we should be able to consistently apply whatever rationale we use for future decisions.
--> Do we want to follow the OpenSSL approach throughout? Who's going to change the code "in the rest of" liboqs
accordingly? I'd argue that it should be the OQS team (by way of a new issue) for the non-OpenSSL code path as this PR does it correctly for the case with OpenSSL used. Opinions, @dstebila @SWilson4 ? We could also label such issue as a non-urgent, "non-product" issue assuming we decide at some point that OQS must only be used in productive settings with OQS_USE_OPENSSL set to ON.
Assuming this is the way we go, I approve.
Do we want to follow the OpenSSL approach throughout?
Just because we use OpenSSL for some things, doesn't mean we have to use them for everything nor agree with every choice they make. As far as I know, there are good reasons to use the dedicated functions SecureZeroMemory / explicit_bzero / memset_s
when available.
Who's going to change the code "in the rest of" liboqs accordingly?
I don't understand what the "rest of" is referring to here.
If this is an issue with no clear "best" solution, it could be added as a choice for users. There is an option whether to use OpenSSL for SHA3 called OQS_USE_SHA3_OPENSSL
, this could be similar if there would be an OQS_USE_CLEANSE_OPENSSL
option as well.
it could be added as a choice for users.
Doing this would a) create more (too much?) configurability and b) make us shirk a decision on a good default -- that I think a trustworthy security software library should be able to make for its users -- that by and large (should be allowed to) know less than the developers of the library.
As far as I know, there are good reasons to use the dedicated functions SecureZeroMemory / explicit_bzero / memset_s when available.
Can you please elaborate on them & particularly why they are apparently not used by OpenSSL?
I don't understand what the "rest of" is referring to here.
I meant all code requiring secure memory clearing in the absence of OpenSSL. If there is none or if the macro is already used throughout the code base, I agree this may be the empty set.
it could be added as a choice for users.
Doing this would a) create more (too much?) configurability and b) make us shirk a decision on a good default -- that I think a trustworthy security software library should be able to make for its users -- that by and large (should be allowed to) know less than the developers of the library.
I agree, I don't think this level of configurability is necessary.
As far as I know, there are good reasons to use the dedicated functions SecureZeroMemory / explicit_bzero / memset_s when available.
Can you please elaborate on them & particularly why they are apparently not used by OpenSSL?
For these specific functions, the compilers promise that they will not optimize the call away. As far as I know, the volatile trick that we use as a backup (and that OpenSSL uses as a default) is just that -- a trick, which we think that compilers will not optimize away, but there's no formal guarantee that compilers won't optimize it away.
I don't understand what the "rest of" is referring to here.
I meant all code requiring secure memory clearing in the absence of OpenSSL. If there is none or if the macro is already used throughout the code base, I agree this may be the empty set.
This should be the only place that the change is needed in our code base -- the rest of the code in liboqs is meant to be calling OQS_MEM_cleanse
. We have some CI tests complaining about bare free
calls (instead of calls to OQS_MEM_secure_free
or OQS_MEM_insecure_free
). Although I don't think we have any CI tests scanning for memset's, which I speculate would have many false positives.
This should be the only place that the change is needed in our code base
Thanks for that assessment. In that case, I'd suggest accepting the PR as-is and consider the issue sufficiently dealt with. OK, @dstebila @SWilson4 ?
OK, I just took a closer look through the OpenSSL repo and found what appear to be assembly implementations of OPENSSL_cleanse
. I had missed these originally because they're contained in Perl scripts. (I assume that the scripts are used to generate code somehow; I'm not familiar with the OpenSSL build process.)
With this knowledge, I'm OK with this PR as-is. What I said above about it being a regression does not apply, as the OpenSSL implementation apparently does not simply default to the approach that we use as a fallback.
I'll approve once CI is green (a rebase on main
will hopefully clear up the failing style check). Sorry for holding this up: I should have figured that OpenSSL would have assembly implementations for something as basic (and easily optimized out) as zeroing memory.
This should be the only place that the change is needed in our code base
Thanks for that assessment. In that case, I'd suggest accepting the PR as-is and consider the issue sufficiently dealt with. OK, @dstebila @SWilson4 ?
Okay with me.
Okay with me.
Thanks @dstebila .
@bencemali Please note the CI failure: It seems to be relevant.
Thanks @bencemali for the fix in the latest commit! CI passes. Now a second review/approval please @SWilson4 @dstebila so we can merge this finally.
Thanks for the changes @bencemali!
The OPENSSL_cleanse function could be used when OQS_USE_OPENSSL is set.