RE: Secure storage of private (RSA) keys

2014-04-15 Thread Salz, Rich
In our haste to help, the secure memory allocation patch we posted last week 
had two issues. First, it wasn’t easy to use. We knew that, and tried to set 
expectations accordingly. Second, it wasn’t really secure enough. We didn’t 
know that, and we thank everyone who brought it to our attention. For example, 
it only protected keys that came in via ASN.1 (which addressed our use-case but 
wasn’t made explicit) and, much worse, it failed to protect all the necessary 
key parameters and values used in intermediate calculations.
 
We are working on new code that will  address both issues, and hope to post the 
next revision in a couple of days.  The heap is more like OpenSSL coding style, 
and protects all BIGNUM's.  If you are interested and able to help out before 
we post it, please contact me directly.
 
/r$

--  
Principal Security Engineer
Akamai Technology
Cambridge, MA


RE: Secure storage of private (RSA) keys

2014-04-11 Thread Salz, Rich
> Have you thought about mprotecting the guard pages with
> mprotect(PROT_NONE) so the application crashes in case of a stray memory 
> access?

Yes, rats.  My message implied that we do that.  And I then posted the wrong 
version of the code. :(

Here's the right version of cmm_init.

/r$ 

--  
Principal Security Engineer
Akamai Technology
Cambridge, MA

void *
cmm_init(int size, int mem_min_unit, int overrun_bytes)
{
int i;
size_t pgsize = (size_t)sysconf(_SC_PAGE_SIZE);
size_t aligned = (pgsize + size + (pgsize - 1)) & ~(pgsize - 1);

mem_arena_size = size;
Mem_min_unit   = mem_min_unit,
Overrun_bytes  = overrun_bytes;
/* make sure mem_arena_size and Mem_min_unit are powers of 2 */
assert(mem_arena_size > 0);
assert(mem_min_unit > 0);
assert(0 == ((mem_arena_size-1)&mem_arena_size));
assert(0 == ((Mem_min_unit-1)&Mem_min_unit));

cmm_bittable_size = (mem_arena_size/Mem_min_unit) * 2;

i = cmm_bittable_size;
cmm_max_free_lists = -1;
while(i) {
i>>=1;
cmm_max_free_lists++;
}

cmm_free_list = malloc(cmm_max_free_lists * sizeof(void *));
assert(cmm_free_list);
memset(cmm_free_list, 0, cmm_max_free_lists*sizeof(void *));

cmm_bittable = malloc(cmm_bittable_size>>3);
assert(cmm_bittable);
memset(cmm_bittable, 0, cmm_bittable_size>>3);

cmm_bitmalloc = malloc(cmm_bittable_size>>3);
assert(cmm_bitmalloc);
memset(cmm_bitmalloc, 0, cmm_bittable_size>>3);

cmm_arena = mmap(NULL, pgsize + mem_arena_size + pgsize, 
PROT_READ|PROT_WRITE,
 MAP_ANON|MAP_PRIVATE, 0, 0);
assert(MAP_FAILED  != cmm_arena);
mprotect(cmm_arena, pgsize, PROT_NONE);
mprotect(cmm_arena + aligned, pgsize, PROT_NONE);
set_bit(cmm_arena, 0, cmm_bittable);
cmm_add_to_list(&cmm_free_list[0], cmm_arena);

/* first bit means that table is in use, multi-arena management */
/* SETBIT(cmm_bittable, 0); */

return cmm_arena;
}


Re: Secure storage of private (RSA) keys

2014-04-11 Thread Hannes Frederic Sowa
Hello!

On Fri, Apr 11, 2014 at 01:22:21PM -0400, Salz, Rich wrote:
> Akamai Technologies is pleased to offer the following patch to OpenSSL. It 
> adds a "secure arena" that is used to store RSA private keys.  This arena is 
> mmap'd, with guard pages before and after so pointer over- and under-runs 
> won't wander into it. It's also locked into memory so it doesn't appear on 
> disk, and when possible it's also kept out of core files.  This patch is a 
> variant of what we've been using to help protect customer keys for a decade.

Have you thought about mprotecting the guard pages with
mprotect(PROT_NONE) so the application crashes in case of a stray
memory access?

Thanks,

  Hannes

__
OpenSSL Project http://www.openssl.org
User Support Mailing Listopenssl-users@openssl.org
Automated List Manager   majord...@openssl.org


Secure storage of private (RSA) keys

2014-04-11 Thread Salz, Rich
Akamai Technologies is pleased to offer the following patch to OpenSSL. It adds 
a "secure arena" that is used to store RSA private keys.  This arena is mmap'd, 
with guard pages before and after so pointer over- and under-runs won't wander 
into it. It's also locked into memory so it doesn't appear on disk, and when 
possible it's also kept out of core files.  This patch is a variant of what 
we've been using to help protect customer keys for a decade.



This should really be considered more of a proof of concept than something that 
you want to put directly into production. It slides into the ASN1 code rather 
than adding a new API (OPENSSL_secure_allocate et al), the overall code isn't 
portable, and so on. If there is community interest, we would be happy to help 
work on addressing those issues.  Let me restate that: *do not just take this 
patch and put it into production without careful review.*



OpenSSL is important to us, and this is the first of what we hope will be 
several significant contributions in the near future.



Thanks.



/r$


--
Principal Security Engineer
Akamai Technology
Cambridge, MA


diff -uNr -x'*.[oas]' openssl-1.0.1g.orig/crypto/Makefile 
openssl-1.0.1g/crypto/Makefile
--- openssl-1.0.1g.orig/crypto/Makefile 2014-04-10 13:11:56.0 -0400
+++ openssl-1.0.1g/crypto/Makefile  2014-04-10 13:02:39.0 -0400
@@ -35,14 +35,16 @@
 LIB= $(TOP)/libcrypto.a
 SHARED_LIB= libcrypto$(SHLIB_EXT)
 LIBSRC=cryptlib.c mem.c mem_clr.c mem_dbg.c cversion.c ex_data.c 
cpt_err.c \
-   ebcdic.c uid.c o_time.c o_str.c o_dir.c o_fips.c o_init.c fips_ers.c
+   ebcdic.c uid.c o_time.c o_str.c o_dir.c o_fips.c o_init.c fips_ers.c \
+   secure_malloc.c buddy_allocator.c
 LIBOBJ= cryptlib.o mem.o mem_dbg.o cversion.o ex_data.o cpt_err.o ebcdic.o \
-   uid.o o_time.o o_str.o o_dir.o o_fips.o o_init.o fips_ers.o $(CPUID_OBJ)
+   uid.o o_time.o o_str.o o_dir.o o_fips.o o_init.o fips_ers.o 
$(CPUID_OBJ) \
+   secure_malloc.o buddy_allocator.o
 
 SRC= $(LIBSRC)
 
 EXHEADER= crypto.h opensslv.h opensslconf.h ebcdic.h symhacks.h \
-   ossl_typ.h
+   ossl_typ.h secure_malloc.h
 HEADER=cryptlib.h buildinf.h md32_common.h o_time.h o_str.h o_dir.h 
$(EXHEADER)
 
 ALL=$(GENERAL) $(SRC) $(HEADER)
diff -uNr -x'*.[oas]' openssl-1.0.1g.orig/crypto/asn1/tasn_dec.c 
openssl-1.0.1g/crypto/asn1/tasn_dec.c
--- openssl-1.0.1g.orig/crypto/asn1/tasn_dec.c  2014-03-17 12:14:20.0 
-0400
+++ openssl-1.0.1g/crypto/asn1/tasn_dec.c   2014-04-10 16:32:23.0 
-0400
@@ -169,6 +169,11 @@
int otag;
int ret = 0;
ASN1_VALUE **pchptr, *ptmpval;
+
+int ak_is_rsa_key  = 0; /* Are we parsing an RSA key? */
+int ak_is_secure_field = 0; /* should this field be allocated from the 
secure arena? */
+int ak_is_arena_active = 0; /* was the secure arena already activated? 
*/
+
if (!pval)
return 0;
if (aux && aux->asn1_cb)
@@ -407,6 +412,11 @@
if (asn1_cb && !asn1_cb(ASN1_OP_D2I_PRE, pval, it, NULL))
goto auxerr;
 
+/* Watch out for this when OpenSSL is upgraded! */
+/* We have to be sure that it->sname will still be "RSA" */
+if (it->sname[0] == 'R' && it->sname[1] == 'S' && it->sname[2] 
== 'A' && it->sname[3] == 0)
+ak_is_rsa_key = 1;
+
/* Get each field entry */
for (i = 0, tt = it->templates; i < it->tcount; i++, tt++)
{
@@ -445,8 +455,30 @@
/* attempt to read in field, allowing each to be
 * OPTIONAL */
 
+ 
+/* Watch out for this when OpenSSL is upgraded! */
+/* We have to be sure that seqtt->field_name will 
still be */
+/* "d", "p", and "q" */
+ak_is_secure_field = 0;
+ak_is_arena_active = 0;
+if (ak_is_rsa_key)
+{
+/* ak_is_rsa_key is set for public keys too */
+/* however those don't have these variables */
+const char *f = seqtt->field_name;
+if ((f[0] == 'd' || f[0] == 'p' || f[0] == 
'q') && f[1] == 0)
+{
+ak_is_secure_field = 1;
+ak_is_arena_active = 
start_secure_allocation();
+}
+}
+
ret = asn1_template_ex_d2i(pseqval, &p, len,
seqtt, isopt, ctx);
+ 
+if (ak_is_secure_field && !ak_is_arena_active)
+stop_secure_allocation();
+