code for encrypting sensitive tcmalloc metadata

code for encrypting sensitive tcmalloc metadata


BUG=NONE
TEST=NONE

[jschuh, 2011-09-13 16:37:12]

http://codereview.chromium.org/7833003/diff/5006/base/allocator/allocator.gyp
File base/allocator/allocator.gyp (right):

http://codereview.chromium.org/7833003/diff/5006/base/allocator/allocator.gyp...
base/allocator/allocator.gyp:14: 'defines' : ["TCMALLOC_SPAN_ENCRYPT"],
You'll probably want a flag in a main TCMalloc header that combines all these
options for simplicity. Something like TCMALLOC_HARDENED.

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
File third_party/tcmalloc/chromium/src/metadata_encrypt.h (right):

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt.h:43: #include
"metadata_encrypt_generic.h"
I'm assuming this is where the OS-specific ifdefs will go?

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
File third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h (right):

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:1: // Copyright (c)
2011, Google Inc.
I'd rename this metadata_encrypt_posix.h. Also, I don't think /dev/urandom is
posix. So, maybe you should test for /dev/urandom first and fall back to
/dev/random (which I think is posix). Or, you might be able to just switch based
on a compile-time macro.

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:45: static bool
initialized_ = false;
You don't need a separate bool for initialized. Just check that key_ isn't zero.

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:53: initialized_ ==
true;
Never mark something as "true" until you've actually done it. ;)

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:61: size_t
num_bytes = read(fd, reinterpret_cast<void *>(&key_)+num_read_bytes,
Don't read directly into key. Better to store it in a local stack variable until
the operation is complete. then you can verify that your seed is non-zero (and
retry if needed). A related question: do we have a guarantee (or expectation)
that we're single threaded at this point, or should we CAS the key in?

[jar (doing other things), 2011-09-13 18:15:18]

As a security guy, the name "encryption" is surprising, and seemingly
misleading.

Encryption has a technical meaning.  I'd be hard pressed to (for instance) call
a system which allows a person to see plain and cipher text, and instantly
surmise a key, "encryption."

Given that you're currently doing simple XOR coding, perhaps calling it a
metadata_coder would be reasonable.

You also need to think about performance.  This may involve rethinking how the
de/coding API is written (consider the cost or repeated loads of the random
value when traversing a linked list), and/or reconsidering where you store the
random data, so that it is pulled in with a cache line involved in TCMalloc
processing.

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
File third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h (right):

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:51: // Ensure this
function only executes once.
You also want assurances that this is only executed on one thread... presumably
during initialization before multi-threading has begun.

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:53: initialized_ ==
true;
+1  Very good coding style.

On 2011/09/13 16:37:12, Justin Schuh wrote:
> Never mark something as "true" until you've actually done it. ;)

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:63: if (num_bytes <
0) {
Since num_bytes is unsigned, this is a very strange test (always false).

In addition, if num_bytes were signed, I doubt that we would want to continue
with the loop.

[bxx, 2011-09-13 19:36:08]

These are in response to Justin's comments.

Also, it looks like cl picked up on some old changes i made for my DLL
implementation when I updated this patch, sorry about that.  I must not have had
the DLL code pulled to my branch before I started working on the pointer
encryption.

..looking at Jim's comments next.

http://codereview.chromium.org/7833003/diff/5006/base/allocator/allocator.gyp
File base/allocator/allocator.gyp (right):

http://codereview.chromium.org/7833003/diff/5006/base/allocator/allocator.gyp...
base/allocator/allocator.gyp:14: 'defines' : ["TCMALLOC_SPAN_ENCRYPT"],
On 2011/09/13 16:37:12, Justin Schuh wrote:
> You'll probably want a flag in a main TCMalloc header that combines all these
> options for simplicity. Something like TCMALLOC_HARDENED.

Done.

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
File third_party/tcmalloc/chromium/src/metadata_encrypt.h (right):

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt.h:43: #include
"metadata_encrypt_generic.h"
On 2011/09/13 16:37:12, Justin Schuh wrote:
> I'm assuming this is where the OS-specific ifdefs will go?

Yes, this is where they go once they exist.

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
File third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h (right):

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:1: // Copyright (c)
2011, Google Inc.
On 2011/09/13 16:37:12, Justin Schuh wrote:
> I'd rename this metadata_encrypt_posix.h. Also, I don't think /dev/urandom is
> posix. So, maybe you should test for /dev/urandom first and fall back to
> /dev/random (which I think is posix). Or, you might be able to just switch
based
> on a compile-time macro.

Done.

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:45: static bool
initialized_ = false;
On 2011/09/13 16:37:12, Justin Schuh wrote:
> You don't need a separate bool for initialized. Just check that key_ isn't
zero.

Done.

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:53: initialized_ ==
true;
On 2011/09/13 16:37:12, Justin Schuh wrote:
> Never mark something as "true" until you've actually done it. ;)

Done.

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:61: size_t
num_bytes = read(fd, reinterpret_cast<void *>(&key_)+num_read_bytes,
On 2011/09/13 16:37:12, Justin Schuh wrote:
> Don't read directly into key. Better to store it in a local stack variable
until
> the operation is complete. then you can verify that your seed is non-zero (and
> retry if needed). A related question: do we have a guarantee (or expectation)
> that we're single threaded at this point, or should we CAS the key in?
The initialization is done when TCMallocGuard() is constructed.  Chrome creates
this before main() and any treads are created, according to the comments.  Given
my experience of trying to find a good spot to perform initialization, I am
willing to believe that it is not multithreaded at this point.
http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/tcmalloc/chromiu...

[bxx, 2011-09-13 20:06:58]

Submitted a patch responding to Jim's comments.

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
File third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h (right):

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:51: // Ensure this
function only executes once.
On 2011/09/13 18:15:18, jar wrote:
> You also want assurances that this is only executed on one thread...
presumably
> during initialization before multi-threading has begun.
Initialization is done when TCMallocGuard() is constructed, this is something
that is called before multi-threading begins.  This should fail if called by
multiple threads (see next set of patches)

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:51: // Ensure this
function only executes once.
On 2011/09/13 18:15:18, jar wrote:
> You also want assurances that this is only executed on one thread...
presumably
> during initialization before multi-threading has begun.
It gets initialized when TCMallocGuard() is constructed.  This is done before
multi-threading begins.  Wouldn't the static variables defined here be global
across all threads?

http://codereview.chromium.org/7833003/diff/5006/third_party/tcmalloc/chromiu...
third_party/tcmalloc/chromium/src/metadata_encrypt_generic.h:63: if (num_bytes <
0) {
On 2011/09/13 18:15:18, jar wrote:
> Since num_bytes is unsigned, this is a very strange test (always false).
> 
> In addition, if num_bytes were signed, I doubt that we would want to continue
> with the loop.

The man pages tell me read() returns -1 when there is an error.  I see chromium
also checks for negative values when using read().  But now I see that it
returns ssize_t, not size_t.
http://www.google.com/codesearch#hfE6470xZHk/base/file_util_posix.cc&q=readfr...

[bxx_google.com, 2011-09-16 04:03:50]

I patched this code a bunch of times since the last review and I want to
explain my last change and see if you have any advice-- on a different
branch I patched the free lists so the pointers are encrypted  and found
that the current metadata_encode_posix.h implementation with inlined
functions wasn't going to cut it.  The problem is that we ended up with
multiple copies of the key we were encrypting against and in only one
location does key get set as a random value.  Unfortunately I couldn't think
of a clean way of keeping encryption/decryption inline functions when they
need access to a global variable.  We can still use inline functions when we
use Window's EncodePointer and I wanted to take advantage that.  My solution
to all of this is in the last set of patches.  The posix implementation is a
bit ugly and proof-of-concepty at the moment.  Let me know if you have any
ideas of how to make it nicer.  I did it the way I did so I still can
control which implementation gets used purely from the metadata_encryption.h
file.

The good news is that I do have encrypted pointers on the free lists
working, the bad news is that it is all dependent on this changeset so I am
holding off from posting it for code review at the moment.

_bx