openssh unix dev - Jan 2016 - Use |mprotect()| to secure key data ? / was: Re: Proposal: always handle keys in separate process

On Thu, Jan 14, 2016 at 8:12 PM, Alexander Wuerstlein <arw at cs.fau.de>
wrote:
> Hello,
>
> in light of the recent CVE-2016-0777, I came up with the following idea,
> that would have lessened its impact. Feel free to ignore or flame me,
> maybe its stupid or I missed something :)
>
> - private key material should only ever be handled in a separate process
>   from the SSH client. ssh-agent (maybe slightly extended) seems the
>   logical choice.
[snip]

While this *might* be a bit of an overkill (think about embedded
systems where the extra >= 500kb for a separate process actually hurt)
... what about using the existing facilities like |mprotect()| to
secure the key data, e.g. like this:
-- snip --
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <errno.h>
#include <sys/mman.h>

#define MYMSG(str) { puts((str)) ; fflush(stdout); fflush(stderr); }
#define SECRET_BUFF_SIZE (8192) /* should be page size */
char *secret_buff = NULL;

void write_buff(const char *in_str)
{
    mprotect(secret_buff, SECRET_BUFF_SIZE, PROT_WRITE);
    strncpy(secret_buff, in_str, SECRET_BUFF_SIZE);
    mprotect(secret_buff, SECRET_BUFF_SIZE, PROT_NONE);
}

void read_buff(void)
{
    mprotect(secret_buff, SECRET_BUFF_SIZE, PROT_READ);
    printf("|%s|\n", secret_buff);
    mprotect(secret_buff, SECRET_BUFF_SIZE, PROT_NONE);
}

void thief_read_buff(void)
{
    printf("|%s|\n", secret_buff);
}

int main(int ac, char *av[])
{
    secret_buff = mmap(NULL, SECRET_BUFF_SIZE*2, PROT_READ|PROT_WRITE,
MAP_ANON|MAP_PRIVATE, -1, 0);
    if ((secret_buff == (void *)-1) || (secret_buff == NULL))
    {
        perror("mmap() failed");
        return EXIT_FAILURE;
    }

    MYMSG("# writing something into the secret buffer");
    write_buff("hello, this string is secret: vem_iam_1983");

    MYMSG("# authorised secret buffer reader...");
    read_buff();

    MYMSG("# ALERT ALERT thieves are trying to use the buffer...");
    thief_read_buff();

    return EXIT_SUCCESS;
}
-- snip --

This example goes nicely "boom" with a SIGSEGV in function
|thief_read_buff()| when it tries to access the data in |secret| which
have neither |PROT_READ| nor |PROT_WRITE| at that point:
-- snip --
$ gdb ./a.out
[snip]
Reading symbols from /home/test001/tmp/a.out...done.
(gdb) run
Starting program: /home/test001/tmp/a.out
# writing something into the secret buffer
# authorised secret buffer reader...
|hello, this string is secret: vem_iam_1983|
# ALERT ALERT thieves are trying to use the buffer...

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7ac3e44 in _IO_vfprintf_internal (s=<value optimized out>,
format=<value optimized out>, ap=<value optimized out>) at
vfprintf.c:1593
1593    vfprintf.c: No such file or directory.
        in vfprintf.c
(gdb) where
#0  0x00007ffff7ac3e44 in _IO_vfprintf_internal (s=<value optimized
out>, format=<value optimized out>, ap=<value optimized out>) at
vfprintf.c:1593
#1  0x00007ffff7acb34a in __printf (format=<value optimized out>) at
printf.c:35
#2  0x0000000000400853 in thief_read_buff () at secretbuffer.c:27
#3  0x000000000040093d in main (ac=1, av=0x7fffffffde08) at secretbuffer.c:46
-- snip --

Granted this way is not elegant but it prevents unintended data
leakage with a minimum of resource overhead (erm... yes, we still
fiddle at MMU level which is expensive from an OS point of view but
it's still a few hundred times more lightwheight than having a
separate process around).

The other idea from the <sys/mman.h> area would be to store secret
data in unlinked temporary files, e.g. we only keep the fd's around
and |mmap()| and |munmap()| them on demand to store/read data in
dedicated functions.

Note that this only works for memory (file mapped or |MAP_ANON|)
obtained via |mmap()|, so using the stack or global vars is not
recommended (POSIX and SUS do not allow it but some OSes support it
anyway...).

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix
programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)

That won't work when the data was recovered because it was read inside
a stdio buffer which was not overwritten before being freed.

On Tue, Jan 19, 2016 at 11:53 PM, ?ngel Gonz?lez <keisial at gmail.com>
wrote:
> That won't work when the data was recovered because it was read inside
> a stdio buffer which was not overwritten before being freed.
Why is stdio used in such a security-sensitive area anyway ? Is there
any performance impact if the code is switched to plain { |open()|,
|read()|, ... } (with sufficient wrappers for |EINTR| handling) ?

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix
programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)