Safely allocate attachment buffers in PouchDB/Node.js

[micaksica]

Due to Buffer (number) is unsafe in Node and PouchDB's usage of the now-deprecated Buffer(number) constructor for attachments, PouchDB attachment buffers allocate potentially uninitialized memory via malloc from the Node.js process. These buffers may contain data left over from GC'd / freed heap memory from other parts of the Node.js process, and may lead to exploitable memory corruption if Buffer() types are not properly checked in PouchDB.

The PouchDB.put API does not validate the type coming in from Node, and it from my review of some of the PouchDB code, it is only saved because crypto does not accept the Number primitive as part of its digest.

As an example of something that could've gone wrong (but didn't):

db.put({
    _id: 'memory_leak_test',
    _attachments: {
        'allocate_with_primitive': {
            content_type: 'text/plain',
            data: 1024
        }
    });

is invalid, and data should be checked by PouchDB to make sure it is of string, Blob or Buffer data types; instead, PouchDB actually only checks if string is the datatype and otherwise passes the type through for allocation.

If this Number primitive were to succeed, the attachment for memory_leak_test would contain 1024 bytes (1KB) of uninitialized memory from the Node.js allocator that may lead to information leakage about other parts of the Node.js process. Thankfully, it does not due to the function binaryMd5 which uses crypto.createHash('md5').update(data, 'binary') on line 4; the .update method throws a TypeError which does not make this possible. PouchDB did not validate this data on its end.

Buffers should be zerofilled in LTS, or Buffer.alloc used in Node 6.x to prevent use of uninitialized memory. At the very least these Buffers may lead to unexpected internal behavior, and are unsafe to use in general (per the issue above, or the potential compromise due to improper type checking.)

I suspect the easiest option would be to extend Buffer to SafeBuffer or the like, which has a constructor that prefers Buffer.alloc, or uses new Buffer(len).fill(0) in cases where the incoming type is Number. Doing this would harden PouchDB against potential memory corruption / use of uninitialized memory attacks.

[nolanlawson]

Nice catch. We should switch to safe-buffer, yes.

[micaksica]

Would it be worthwhile if I submitted a pull request fixing this one?

[nolanlawson]

Absolutely. :)

[daleharvey]

Fixed in 3a44b63