This report is for an instance of CWE-14 “Compiler Removal of Code to Clear Buffers” https://cwe.mitre.org/data/definitions/14.html
A cleanup
macro is defined here and is used in several places to clear local variables of secrets: https://github.com/janmojzis/tinyssh/blob/97dd9e05f52482e46d660af81547c7c02669c1a2/crypto/cleanup.h
For instance on my computer, gcc invokes clang:
~/tinyssh $ gcc -v
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 5.1 (clang-503.0.40) (based on LLVM 3.4svn)
Target: x86_64-apple-darwin13.4.0
And the result of the compilation of the function crypto_scalarmult_nistp256_tinynacl
, that invokes the cleanup
macro, is:
~/tinyssh $ otool -tV build/lib/libtinynacl.a | grep -A50 crypto_scalarmult_nistp256_tinynacl
_crypto_scalarmult_nistp256_tinynacl:
0000000000000000 pushq %r15
0000000000000002 pushq %r14
0000000000000004 pushq %r12
0000000000000006 pushq %rbx
0000000000000007 subq $0xc8, %rsp
000000000000000e movq %rsi, %r14
0000000000000011 movq %rdi, %rbx
0000000000000014 movq ___stack_chk_guard(%rip), %r12
000000000000001b movq _crypto_scalarmult_nistp256_tinynacl(%r12), %rax
000000000000001f movq %rax, 0xc0(%rsp)
0000000000000027 leaq 0x60(%rsp), %rdi
000000000000002c movq %rdx, %rsi
000000000000002f callq _gep256_frombytes
0000000000000034 testl %eax, %eax
0000000000000036 jne 0x5f
0000000000000038 leaq _crypto_scalarmult_nistp256_tinynacl(%rsp), %r15
000000000000003c leaq 0x60(%rsp), %rsi
0000000000000041 movq %r15, %rdi
0000000000000044 movq %r14, %rdx
0000000000000047 callq _gep256_scalarmult
000000000000004c movq %rbx, %rdi
000000000000004f movq %r15, %rsi
0000000000000052 callq _gep256_tobytes
0000000000000057 movl %eax, %ecx
0000000000000059 xorl %eax, %eax
000000000000005b testl %ecx, %ecx
000000000000005d je 0xa3
000000000000005f movq $_crypto_scalarmult_nistp256_tinynacl, 0x38(%rbx)
0000000000000067 movq $_crypto_scalarmult_nistp256_tinynacl, 0x30(%rbx)
000000000000006f movq $_crypto_scalarmult_nistp256_tinynacl, 0x28(%rbx)
0000000000000077 movq $_crypto_scalarmult_nistp256_tinynacl, 0x20(%rbx)
000000000000007f movq $_crypto_scalarmult_nistp256_tinynacl, 0x18(%rbx)
0000000000000087 movq $_crypto_scalarmult_nistp256_tinynacl, 0x10(%rbx)
000000000000008f movq $_crypto_scalarmult_nistp256_tinynacl, 0x8(%rbx)
0000000000000097 movq $_crypto_scalarmult_nistp256_tinynacl, _crypto_scalarmult_nistp256_tinynacl(%rbx)
000000000000009e movl $0xffffffff, %eax ## imm = 0xFFFFFFFF
00000000000000a3 movq _crypto_scalarmult_nistp256_tinynacl(%r12), %rcx
00000000000000a7 cmpq 0xc0(%rsp), %rcx
00000000000000af jne 0xc0
00000000000000b1 addq $0xc8, %rsp
00000000000000b8 popq %rbx
00000000000000b9 popq %r12
00000000000000bb popq %r14
00000000000000bd popq %r15
00000000000000bf ret
00000000000000c0 callq ___stack_chk_fail
00000000000000c5 nopw %cs:_crypto_scalarmult_nistp256_tinynacl(%rax,%rax)
_crypto_scalarmult_nistp256_tinynacl_base:
…
The above is the translation of the source code below, from crypto/crypto_scalarmult_nistp256.c
:
int crypto_scalarmult_nistp256_tinynacl(unsigned char *q, const unsigned char *n, const unsigned char *p) {
gep256 P, Q;
long long i;
int ret = -1;
if (gep256_frombytes(P, p) != 0) goto fail;
gep256_scalarmult(Q, P, n);
if (gep256_tobytes(q, Q) != 0) goto fail;
ret = 0;
goto cleanup;
fail:
for (i = 0; i < 64; ++i) q[i] = 0;
cleanup:
cleanup(P); cleanup(Q);
return ret;
}
The for (i = 0; i < 64; ++i) q[i] = 0;
was translated to the series of 8 movq
instructions. The code that follows is the canary check. The code cleanup(P); cleanup(Q);
was translated to nothing.
The real GCC, or any modern optimizing C compiler, will do the same and translate the invocations of cleanup()
on all local variables at the end of their scopes to nothing.
There exists no perfect solution for this problem. The C11 standard introduced memset_s
but it is the still the wrong idiom http://www.daemonology.net/blog/2014-09-06-zeroing-buffers-is-insufficient.html and GCC can still translate that to nothing: http://goo.gl/LDfPHG (gcc.godbolt.org link by Samuel Neves). Also not everyone is using a C11 compiler yet.
I found that if I simply convert the array passed as argument to the cleanup()
macro to a volatile
pointer, my compiler does generate the code to clean up the local arrays:
~/tinyssh $ git diff
diff --git a/crypto/cleanup.h b/crypto/cleanup.h
index 0566c59..efb95d3 100644
--- a/crypto/cleanup.h
+++ b/crypto/cleanup.h
@@ -1,6 +1,6 @@
#ifndef _CLEANUP_H____
#define _CLEANUP_H____
-#define cleanup(x) for (i = 0; i < sizeof(x); ++i) ((char *)x)[i] = 0;
+#define cleanup(x) for (i = 0; i < sizeof(x); ++i) ((volatile char *)x)[i] = 0;
#endif
~/tinyssh $ otool -tV build/lib/libtinynacl.a | grep -A50 crypto_scalarmult_nistp256_tinynacl
_crypto_scalarmult_nistp256_tinynacl:
0000000000000000 pushq %r15
0000000000000002 pushq %r14
0000000000000004 pushq %r12
0000000000000006 pushq %rbx
0000000000000007 subq $0xc8, %rsp
000000000000000e movq %rsi, %r14
0000000000000011 movq %rdi, %rbx
0000000000000014 movq ___stack_chk_guard(%rip), %r12
000000000000001b movq _crypto_scalarmult_nistp256_tinynacl(%r12), %rax
000000000000001f movq %rax, 0xc0(%rsp)
0000000000000027 leaq 0x60(%rsp), %rdi
000000000000002c movq %rdx, %rsi
000000000000002f callq _gep256_frombytes
0000000000000034 testl %eax, %eax
0000000000000036 jne 0x5f
0000000000000038 leaq _crypto_scalarmult_nistp256_tinynacl(%rsp), %r15
000000000000003c leaq 0x60(%rsp), %rsi
0000000000000041 movq %r15, %rdi
0000000000000044 movq %r14, %rdx
0000000000000047 callq _gep256_scalarmult
000000000000004c movq %rbx, %rdi
000000000000004f movq %r15, %rsi
0000000000000052 callq _gep256_tobytes
0000000000000057 movl %eax, %ecx
0000000000000059 xorl %eax, %eax
000000000000005b testl %ecx, %ecx
000000000000005d je 0xa3
000000000000005f movq $_crypto_scalarmult_nistp256_tinynacl, 0x38(%rbx)
0000000000000067 movq $_crypto_scalarmult_nistp256_tinynacl, 0x30(%rbx)
000000000000006f movq $_crypto_scalarmult_nistp256_tinynacl, 0x28(%rbx)
0000000000000077 movq $_crypto_scalarmult_nistp256_tinynacl, 0x20(%rbx)
000000000000007f movq $_crypto_scalarmult_nistp256_tinynacl, 0x18(%rbx)
0000000000000087 movq $_crypto_scalarmult_nistp256_tinynacl, 0x10(%rbx)
000000000000008f movq $_crypto_scalarmult_nistp256_tinynacl, 0x8(%rbx)
0000000000000097 movq $_crypto_scalarmult_nistp256_tinynacl, _crypto_scalarmult_nistp256_tinynacl(%rbx)
000000000000009e movl $0xffffffff, %eax ## imm = 0xFFFFFFFF
00000000000000a3 xorl %ecx, %ecx
00000000000000a5 xorl %edx, %edx
00000000000000a7 nopw _crypto_scalarmult_nistp256_tinynacl(%rax,%rax)
00000000000000b0 movb $_crypto_scalarmult_nistp256_tinynacl, 0x60(%rsp,%rdx)
00000000000000b5 incq %rdx
00000000000000b8 cmpq $0x60, %rdx
00000000000000bc jne 0xb0
00000000000000be nop
00000000000000c0 movb $_crypto_scalarmult_nistp256_tinynacl, _crypto_scalarmult_nistp256_tinynacl(%rsp,%rcx)
00000000000000c4 incq %rcx
00000000000000c7 cmpq $0x60, %rcx
00000000000000cb jne 0xc0
00000000000000cd movq _crypto_scalarmult_nistp256_tinynacl(%r12), %rcx
00000000000000d1 cmpq 0xc0(%rsp), %rcx
00000000000000d9 jne 0xea
00000000000000db addq $0xc8, %rsp
00000000000000e2 popq %rbx
00000000000000e3 popq %r12
00000000000000e5 popq %r14
00000000000000e7 popq %r15
00000000000000e9 ret
00000000000000ea callq ___stack_chk_fail
00000000000000ef nop
_crypto_scalarmult_nistp256_tinynacl_base:
…
Using the volatile
qualifier this way is not perfect either, but it at least tricks some current compilers into doing the right thing, which is somewhat better than the reliable elimination of dead stores that happens without it.