Use |mprotect()| to secure key data ? / was: Re: Proposal: always handle keys in separate process

[Roland Mainz]

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

[Roland Mainz]

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

[Ángel González]

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

[Ángel González]

Post by Roland Mainz

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) ?

Probably not, and in fact I would favor changing it.

I was just pointing out that the private key leak was not in OpenSSH buffers,
which were properly zeroed, but from things like the use of stdio buffers.

Best regards

[Ángel González]

Post by Roland Mainz

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) ?

Probably not, and in fact I would favor changing it.

I was just pointing out that the private key leak was not in OpenSSH buffers,
which were properly zeroed, but from things like the use of stdio buffers.

Your proposal may be an hardening oportunity, but is not a final solution.
For that, a different process would be preferable.

Best regards

[Roland Mainz]

Post by Ángel González

Post by Roland Mainz

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) ?

Probably not, and in fact I would favor changing it.
I was just pointing out that the private key leak was not in OpenSSH
buffers,
which were properly zeroed, but from things like the use of stdio buffers.
Your proposal may be an hardening oportunity, but is not a final solution.
For that, a different process would be preferable.

Well, I am not happy with the solution because it adds *lots* of extra
overhead (not noticeable on today's multi-GHz desktop machines but on
small embedded machines this bites back).

What about the idea of storing "valuable" data in unlinked temp files
and |mmap()| then only on demand ? That would keep them out of the
claws of *other* users (obviously same user can use /proc/$pid/fd/$fd
to |open()| such files, but then the same user could just attach
gdb/dbx and dissect the ssh/sshd/ssh_secure_storage processes and even
inject random code) ...

----

Bye,
Roland

[Stephen Harris]

Post by Roland Mainz
Well, I am not happy with the solution because it adds *lots* of extra
overhead (not noticeable on today's multi-GHz desktop machines but on
small embedded machines this bites back).

Serious question: how many small embedded machines that can't afford
another process are running openssh vs dropbear server?

[Daniel Kahn Gillmor]

Post by Roland Mainz
What about the idea of storing "valuable" data in unlinked temp files
and |mmap()| then only on demand ? That would keep them out of the
claws of *other* users (obviously same user can use /proc/$pid/fd/$fd
to |open()| such files, but then the same user could just attach
gdb/dbx and dissect the ssh/sshd/ssh_secure_storage processes and even
inject random code) ...

depending on the filesystem used, this could mean writing this sensitive
data to the underlying storage medium, which sounds like a worse failure
than anything this proposal would fix.

--dkg

[Gert Doering]

Hi,

Post by Roland Mainz

Post by Roland Mainz
any performance impact if the code is switched to plain { |open()|,
|read()|, ... } (with sufficient wrappers for |EINTR| handling) ?

Well, I am not happy with the solution because it adds *lots* of extra
overhead (not noticeable on today's multi-GHz desktop machines but on
small embedded machines this bites back).

Actually, "open(), read(full file size), close()" is *less* overhead than
mixing in an extra stdio layer. Of course what you don't want to do is
single-byte-read()s then...

gert