[PATCH] Crypto EC - avoids possible memset compiler optimisation

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Crypto EC - avoids possible memset compiler optimisation

DEV Nexen
Hi,

Here a little patch proposal which is usually relevant in cryptographics matters. Usually memset/bzero/... is used to clear private structures but the compiler can possibly optimize those calls but with this change we can unsure sensitive data is properly zero'ed using if possible native calls or memory fence.

Kind regards.

patch-jdk.crypto.ec.diff (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Crypto EC - avoids possible memset compiler optimisation

Adam Petcher
On 1/8/2018 10:13 AM, David CARLIER wrote:

> Hi,
>
> Here a little patch proposal which is usually relevant in
> cryptographics matters. Usually memset/bzero/... is used to clear
> private structures but the compiler can possibly optimize those calls
> but with this change we can unsure sensitive data is properly zero'ed
> using if possible native calls or memory fence.

SunEC doesn't really make an effort to zeroize sensitive data, and all
of the memset operations except for one (line 418) operate on memory
that is not sensitive. While the patch is a relatively simple change
that probably doesn't hurt anything, it doesn't seem to me like this
improvement is particularly valuable. Perhaps it would be more valuable
along with a larger improvement to make SunEC zeroize all intermediate
values. Though this would be a much larger undertaking, and it still may
not be useful on its own because the Java code in the provider also
holds some sensitive values.

>
> Kind regards.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Crypto EC - avoids possible memset compiler optimisation

Christopher Schultz
Adam and David,

On 1/8/18 11:30 AM, Adam Petcher wrote:

> On 1/8/2018 10:13 AM, David CARLIER wrote:
>
>> Hi,
>>
>> Here a little patch proposal which is usually relevant in
>> cryptographics matters. Usually memset/bzero/... is used to clear
>> private structures but the compiler can possibly optimize those calls
>> but with this change we can unsure sensitive data is properly zero'ed
>> using if possible native calls or memory fence.
>
> SunEC doesn't really make an effort to zeroize sensitive data, and all
> of the memset operations except for one (line 418) operate on memory
> that is not sensitive. While the patch is a relatively simple change
> that probably doesn't hurt anything, it doesn't seem to me like this
> improvement is particularly valuable. Perhaps it would be more valuable
> along with a larger improvement to make SunEC zeroize all intermediate
> values. Though this would be a much larger undertaking, and it still may
> not be useful on its own because the Java code in the provider also
> holds some sensitive values.
Also, if you want to "sanitize" memory, you ought to:

1. use explicit_bzero instead of bzero, as bzip may be optimized-away by
the compiler[1]

2. use memset instead of bzero, as memset is POSIX[2] and bzero is not[3]

3. use memset_s instead of memset, since memset_s is guaranteed not to
be optimized-away by the compiler[4]. Its presence is not guaranteed, so
use compiler macros to ensure you have a backup plan if it is not
available (e.g. use memset or manual memory-scrubbing).

4. On Windows, use SecureZeroMemory[5], for reasons similar to the above

-chris

[1] https://www.freebsd.org/cgi/man.cgi?query=explicit_bzero
[2] https://linux.die.net/man/3/memset
[3] https://linux.die.net/man/3/bzero
[4] http://en.cppreference.com/w/c/string/byte/memset
[5] https://msdn.microsoft.com/en-us/library/windows/desktop/aa366877.aspx


signature.asc (998 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Crypto EC - avoids possible memset compiler optimisation

Christopher Schultz
All,

Apologies... I had only read the top of the patch before replying.

All my comments had already been addressed in the actual patch.

Sorry for the noise.

-chris

On 1/12/18 6:00 PM, Christopher Schultz wrote:

> Adam and David,
>
> On 1/8/18 11:30 AM, Adam Petcher wrote:
>> On 1/8/2018 10:13 AM, David CARLIER wrote:
>>
>>> Hi,
>>>
>>> Here a little patch proposal which is usually relevant in
>>> cryptographics matters. Usually memset/bzero/... is used to clear
>>> private structures but the compiler can possibly optimize those calls
>>> but with this change we can unsure sensitive data is properly zero'ed
>>> using if possible native calls or memory fence.
>>
>> SunEC doesn't really make an effort to zeroize sensitive data, and all
>> of the memset operations except for one (line 418) operate on memory
>> that is not sensitive. While the patch is a relatively simple change
>> that probably doesn't hurt anything, it doesn't seem to me like this
>> improvement is particularly valuable. Perhaps it would be more valuable
>> along with a larger improvement to make SunEC zeroize all intermediate
>> values. Though this would be a much larger undertaking, and it still may
>> not be useful on its own because the Java code in the provider also
>> holds some sensitive values.
>
> Also, if you want to "sanitize" memory, you ought to:
>
> 1. use explicit_bzero instead of bzero, as bzip may be optimized-away by
> the compiler[1]
>
> 2. use memset instead of bzero, as memset is POSIX[2] and bzero is not[3]
>
> 3. use memset_s instead of memset, since memset_s is guaranteed not to
> be optimized-away by the compiler[4]. Its presence is not guaranteed, so
> use compiler macros to ensure you have a backup plan if it is not
> available (e.g. use memset or manual memory-scrubbing).
>
> 4. On Windows, use SecureZeroMemory[5], for reasons similar to the above
>
> -chris
>
> [1] https://www.freebsd.org/cgi/man.cgi?query=explicit_bzero
> [2] https://linux.die.net/man/3/memset
> [3] https://linux.die.net/man/3/bzero
> [4] http://en.cppreference.com/w/c/string/byte/memset
> [5] https://msdn.microsoft.com/en-us/library/windows/desktop/aa366877.aspx
>


signature.asc (998 bytes) Download Attachment