[trivial] clear stack variable by function

isle2983 commented at 6:08 pm on January 26, 2017: none

The bare bits = 0; raises a clang static analysis issue.

0--------------------------------------------------------------------------------
1An issue has been found in ./src/ecmult_gen_impl.h:153:5
2Type:         Dead assignment
3Description:  Value stored to 'bits' is never read
4
50: ./src/ecmult_gen_impl.h:153:5 - Value stored to 'bits' is never read
6--------------------------------------------------------------------------------

The line was introduced in 2f6c8019114743e46a741fc6a01f9c6600ccdb5d (when secp256k1_ecmult_gen() lived in src/ecmult_impl.h) with the purpose of clearing stack variables that might have secret info.

However, without that context, the purpose for clearing isn’t obvious from reading the code. Aside from making clang happy, the function name should help it be understood.

gmaxwell commented at 10:33 pm on January 27, 2017: contributor

It’s a fine structural change, but if you’re going to break this out, can you perhaps make it a generic unsigned char clear that takes a point and length (which you’d use sizeof() on here)? — and try to get the compiler to not optimize it out? Edit: on second thought, we may prefer a clear function per type, even if all they’re doing is wrapping a generic one.

(As a matter of principle, we should never silence a warning which is potentially hiding an actual problem. In this case the problem is that the compiler will simply optimize out this assignment).

Example 1 - avoid optimizing out stack variable clearing by using an __attribute__ 72c0ebbea4

Example 2 - avoid optimizing out stack variable clearing by using the 'volatile' keyword cdd46814c4

isle2983 force-pushed on Jan 29, 2017

isle2983 commented at 10:10 pm on January 29, 2017: none

The new branch has two commits with two tentative examples for comment. They focus just on the bits variable for the purpose of discussion even though the problem extends to the other *_clear() functions.

In the first example commit 72c0ebbea413a3d05847a849d629082e5408232e, __attribute__((optimize("O0"))) achieves what we want and the call to secp256k1_secure_clear() can be seen at the bottom of the -03 assembly for secp256k1_ecmult_gen():

 0.LBE2707:
 1	.loc 6 135 0
 2	cmpq	$64, %rdi	#, ivtmp.241
 3	je	.L451	#,
 4	salq	$34, %rdi	#, D.9544
 5	movl	%r11d, 104(%rsp)	# D.9544, %sfp
 6	movl	%ebp, %r14d	# D.9544, D.9552
 7	shrq	$37, %rdi	#, D.9544
 8	movl	%r8d, 112(%rsp)	# D.9544, %sfp
 9	movl	%r12d, %r15d	# D.9544, D.9552
10	movl	416(%rsp,%rdi,4), %edi	# gnb.d, D.9552
11	movl	%r10d, 96(%rsp)	# D.9544, %sfp
12	movl	%ecx, %r11d	# D.9544, D.9552
13	movl	%esi, 88(%rsp)	# D.9544, %sfp
14	movl	%edx, 80(%rsp)	# D.9544, %sfp
15	movl	%r9d, %r13d	# D.9544, D.9552
16	movl	%eax, 48(%rsp)	# D.9544, %sfp
17	jmp	.L452	#
18.L451:
19.LVL2579:
20.LBB2708:
21.LBB2709:
22	.loc 2 222 0
23	leaq	412(%rsp), %rdi	#, tmp5277
24.LVL2580:
25	movl	$4, %esi	#,
26	call	secp256k1_secure_clear	#
27.LVL2581:
28.LBE2709:
29.LBE2708:
30	.loc 6 156 0
31	addq	$1144, %rsp	#,

(As was assumed, it was indeed being optimized out before, but I didn’t quote it here to save space)

However, compiling the same with clang, it throws a warning and doesn’t do what we want:

0$ clang -I. -g -O3 -Wall -Wextra -Wno-unused-function -c src/gen_context.c -fverbose-asm -S
1In file included from src/gen_context.c:13:
2src/group_impl.h:217:28: warning: unknown attribute 'optimize' ignored
3      [-Wunknown-attributes]
4static void __attribute__((optimize("O0"))) secp256k1_secure_clear(void ...
5                           ^
61 warning generated.

So, this approach looks like it is not very universal. ugh.

Taking a step back and looking at OpenSSL, they clear stack variables with a function named BN_clear_free() in (openssl)/crypto/bn/bn_lib.c which calls OPENSSL_cleanse() in (openssl)/crypto/mem_clr.c which is implemented with the volatile keyword:

 0/*
 1 * Pointer to memset is volatile so that compiler must de-reference
 2 * the pointer and can't assume that it points to any function in
 3 * particular (such as memset, which it then might further "optimize")
 4 */
 5typedef void *(*memset_t)(void *,int,size_t);
 6
 7static volatile memset_t memset_func = memset;
 8
 9void OPENSSL_cleanse(void *ptr, size_t len)
10{
11    memset_func(ptr, 0, len);
12}

So, this approach is also tried in cdd46814c4abcd7bc9ce73d616347964ac7ac9be. The assembly in -03 turns out a bit different, but still achieves the aim:

 0.LBE2372:
 1	.loc 6 135 0
 2	cmpq	$64, %rax	#, ivtmp.263
 3	je	.L424	#,
 4	salq	$34, %rax	#, D.9409
 5	shrq	$37, %rax	#, D.9409
 6	movl	400(%rsp,%rax,4), %r9d	# gnb.d, D.9415
 7	jmp	.L425	#
 8.L424:
 9.LVL2528:
10.LBB2373:
11.LBB2374:
12.LBB2375:
13	.loc 2 222 0
14	leaq	396(%rsp), %rdi	#, tmp5505
15.LVL2529:
16	movq	secure_memset(%rip), %rax	# secure_memset, D.9410
17	movl	$4, %edx	#,
18	xorl	%esi, %esi	#
19	call	*%rax	# D.9410
20.LVL2530:
21.LBE2375:
22.LBE2374:
23.LBE2373:
24	.loc 6 156 0
25	addq	$1176, %rsp	#,

This works perfectly with clang too, so this is definitely more viable.

Also, for C++ there is discussion on the internet around a new memset_s() call designed for this purpose but it is only introduced in C++11. In the long run, if memset_s() makes it to C, that will probably be the preferred way to do this.

Questions:

What kind of benchmarking needs to be done to quantify the performance impacts?
Should we look at applying this to all stack vars? or should the focus be a subset of functions that touch private keys?
Any cosmetic suggestions for function names/interface/presentation?

sipa commented at 10:14 pm on January 29, 2017: contributor

memset_s is an optional function in C11 (not C++11), and it doesn’t seem like compilers are eager to implement it.

gmaxwell commented at 0:33 am on February 11, 2017: contributor

10:59 < gmaxwell> glibc 2.25 has explicit_bzero 11:01 < gmaxwell> void 11:01 < gmaxwell> explicit_bzero (void s, size_t len) 11:01 < gmaxwell> { 11:01 < gmaxwell> memset (s, ‘\0’, len); 11:01 < gmaxwell> / Compiler barrier. */ 11:01 < gmaxwell> asm volatile ("" ::: “memory”); 11:01 < gmaxwell> }

isle2983 cross-referenced this on Mar 15, 2017 from issue Clear sensitive memory without getting optimized out. by isle2983

isle2983 commented at 4:16 pm on March 15, 2017: none

closing in favor of #448

isle2983 closed this on Mar 15, 2017

[trivial] clear stack variable by function #438