Hello,
while playing with compiler optimization and reading the norm to write a
nice wipe_memory(char* mem, size_t len) function, I stumbled upon issue
23[1]. It seems that libgcrypt can now provide secure memory to
application running as non-root. What about using this in libotr instead
of a custom implementation?
I pushed a patch on the `secmem` branch, that you can find attached to
this mail.
Cheers,
1. https://bugs.otr.im/issues/23
From 576a33bed3d89e61d02a44795aa5c191de9fa529 Mon Sep 17 00:00:00 2001
From: jvoisin <julien.voi...@dustri.org>
Date: Sun, 12 Apr 2015 00:34:35 +0200
Subject: [PATCH] Use libgcrypt secure memory
---
src/mem.c | 138 +++---------------------------------------------------------
src/mem.h | 2 -
src/proto.c | 4 +-
3 files changed, 7 insertions(+), 137 deletions(-)
diff --git a/src/mem.c b/src/mem.c
index f703606..6b1b927 100644
--- a/src/mem.c
+++ b/src/mem.c
@@ -20,24 +20,13 @@
/* Memory allocation routines for libgcrypt. All of the session key
* information gets allocated through here, so we can wipe it out when
- * it's free()d. We don't use the built-in secmem functions of
- * libgcrypt because you need to declare a fixed amount of it when you
- * start up.
+ * it's free()d. We use the built-in secmem functions of
+ * libgcrypt because since Linux 2.6.9, you don't need to be root anymore
+ * to use use mlock and other goodies. And if you're not running Linux,
+ * libgcrypt will do its best to make the memory "safe" anyway.
*
- * Because "secure" and "insecure" allocations from libgcrypt will get
- * handled the same way (since we're not going to be running as root,
- * and so won't actually have pinned memory), pretend all allocated
- * memory (but just from libgcrypt) is requested secure, and wipe it on
- * free(). */
-
-/* Uncomment the following to add a check that our free() and realloc() only
- * get called on things returned from our malloc(). */
-/* #define OTRL_MEM_MAGIC 0x31415926 */
+ */
-/* system headers */
-#ifdef OTRL_MEM_MAGIC
-#include <stdio.h>
-#endif
#include <stdlib.h>
/* libgcrypt headers */
@@ -46,123 +35,6 @@
/* libotr headers */
#include "mem.h"
-static size_t header_size;
-
-static void *otrl_mem_malloc(size_t n)
-{
- void *p;
- size_t new_n = n;
- new_n += header_size;
-
- /* Check for overflow attack */
- if (new_n < n) return NULL;
- p = malloc(new_n);
- if (p == NULL) return NULL;
-
- ((size_t *)p)[0] = new_n; /* Includes header size */
-#ifdef OTRL_MEM_MAGIC
- ((size_t *)p)[1] = OTRL_MEM_MAGIC;
-#endif
-
- return (void *)((char *)p + header_size);
-}
-
-static int otrl_mem_is_secure(const void *p)
-{
- return 1;
-}
-
-static void otrl_mem_free(void *p)
-{
- void *real_p = (void *)((char *)p - header_size);
- size_t n = ((size_t *)real_p)[0];
-#ifdef OTRL_MEM_MAGIC
- if (((size_t *)real_p)[1] != OTRL_MEM_MAGIC) {
- fprintf(stderr, "Illegal free!\n");
- return;
- }
-#endif
-
- /* Wipe the memory (in the same way the built-in deallocator in
- * libgcrypt would) */
- memset(real_p, 0xff, n);
- memset(real_p, 0xaa, n);
- memset(real_p, 0x55, n);
- memset(real_p, 0x00, n);
-
- free(real_p);
-}
-
-static void *otrl_mem_realloc(void *p, size_t n)
-{
- if (p == NULL) {
- return otrl_mem_malloc(n);
- } else if (n == 0) {
- otrl_mem_free(p);
- return NULL;
- } else {
- void *real_p = (void *)((char *)p - header_size);
- void *new_p;
- size_t old_n = ((size_t *)real_p)[0];
-#ifdef OTRL_MEM_MAGIC
- size_t magic = ((size_t *)real_p)[1];
-#endif
- size_t new_n = n;
- new_n += header_size;
-
- /* Check for overflow attack */
- if (new_n < n) return NULL;
-
-#ifdef OTRL_MEM_MAGIC
- if (magic != OTRL_MEM_MAGIC) {
- fprintf(stderr, "Illegal realloc!\n");
- return NULL;
- }
-#endif
-
- if (new_n < old_n) {
- /* Overwrite the space we're about to stop using */
- void *p = (void *)((char *)real_p + new_n);
- size_t excess = old_n - new_n;
- memset(p, 0xff, excess);
- memset(p, 0xaa, excess);
- memset(p, 0x55, excess);
- memset(p, 0x00, excess);
-
- /* We don't actually need to realloc() */
- new_p = real_p;
- } else {
- new_p = realloc(real_p, new_n);
- if (new_p == NULL) return NULL;
- }
-
- ((size_t *)new_p)[0] = new_n; /* Includes header size */
- return (void *)((char *)new_p + header_size);
- }
-}
-
-void otrl_mem_init(void)
-{
- header_size = 8;
-#ifdef OTRL_MEM_MAGIC
- if (header_size < 2*sizeof(size_t)) {
- header_size = 2*sizeof(size_t);
- }
-#else
- if (header_size < sizeof(size_t)) {
- header_size = sizeof(size_t);
- }
-#endif
-
- gcry_set_allocation_handler(
- otrl_mem_malloc,
- otrl_mem_malloc,
- otrl_mem_is_secure,
- otrl_mem_realloc,
- otrl_mem_free
- );
-}
-
/* Compare two memory blocks in time dependent on the length of the
* blocks, but not their contents. Returns 1 if they differ, 0 if they
* are the same. */
diff --git a/src/mem.h b/src/mem.h
index 0601200..864ba1d 100644
--- a/src/mem.h
+++ b/src/mem.h
@@ -23,8 +23,6 @@
#include <stdlib.h>
-void otrl_mem_init(void);
-
/* Compare two memory blocks in time dependent on the length of the
* blocks, but not their contents. Returns 1 if they differ, 0 if they
* are the same. */
diff --git a/src/proto.c b/src/proto.c
index f3fb10c..817e2fa 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -70,8 +70,8 @@ gcry_error_t otrl_init(unsigned int ver_major, unsigned int ver_minor,
otrl_api_version = api_version;
}
- /* Initialize the memory module */
- otrl_mem_init();
+ /* Initialize 32k of secure memory */
+ gcry_control (GCRYCTL_INIT_SECMEM, 32768, 0);
/* Initialize the DH module */
otrl_dh_init();
--
2.1.0
_______________________________________________
OTR-dev mailing list
OTR-dev@lists.cypherpunks.ca
http://lists.cypherpunks.ca/mailman/listinfo/otr-dev
