Hi
I've written a valgrind plugin called secretgrind [0] that uses taint tracking to find leftovers of sensitive data in RAM. I presented this at RWC last Jan [1].
I've run secretgrind with the following command as a test:
./gnupg-2.0.28/g10/gpg2 --local-user XXXXXXX --output hello-rsa.sig --sign hello.txt.
The passphrase used to protect the key was empty in my test. I understand that does not reflect the majority of use cases, since most people do set a passphrase.
But I thought I'd send you this email with a few findings anyway.
1. functions fwrite/fread()/fgets()/... internally cache data so sensitive data read/written thru those function may lead data not be erased properly -- usually multiples of a page size. Using write/read() is preferable. I mentioned this in the presentation at RWC [1].
2. parse_key() in file parse-packet.c: this uses sk->skey[i] = mpi_read(inp, &n, 0 ); at various locations.
This was flagged by Secretgrind in:
```
[...]
for(i=0; i < npkey; i++ ) {
n = pktlen;
sk->skey[i] = mpi_read(inp, &n, 1 );
[...]
```
and in:
```
for(i=npkey; i < nskey; i++ ) {
[...]
n = pktlen;
sk->skey[i] = mpi_read(inp, &n, 0 );
[...]
```
If the password is empty, then mpi_read returns the unprotected key in memory tagged as non-secure. So using secure=1 is preferable as:
sk->skey[i] = mpi_read(inp, &n, 1 );
I understand that if the key is stored on disk with no passphrase, then it's probably easier for an attacker to get access
to the disk ... so what I'm suggesting is really just a small incremental improvement...
3. _gcry_sexp_find_token() in file sexp.c:
WARNING: this is part of libgcrypt, but has implications for GPG:
The function _gcry_sexp_find_token creates a new list of s-expr thru a call to
xtrymalloc. This gets called when gpg parses a key file. If the password is empty, then the memory
will not be erased. Instead, a call too a secure malloc (eg _gcry_malloc_secure) would be preferable.
I know _gcry_sexp_find_token() can be called with non-sensitive data too; so forcing an erase on those
will inccur a performance hit. Maybe the solution is to have two different functions. As the maintainer of gnupg, you
you are in a better position than me to suggest this to the developers of libgcrypt. So I've not contacted them directly.
4. same as above but with function _gcry_sexp_cdr(), _gcry_sexp_nth(), etc in file sexp.c
More generally, many functions probably need duplicates for a "secure" version, ie to erase their memory thoroughly.
5. Secretgrind also reported problems on functions read_rest() and read_protected_v3_mpi()
that use xmalloc, and not xmalloc_secure. These functions are called on non-sensitive data only
as far as I can tell, so I think these are false positives.
[0] https://github.com/lmrs2/secretgrind
[1] https://www.cl.cam.ac.uk/~lmrs2/publications/erasing_ram_rwc17.pdf