Allow use of nodejs Buffer objects instead of Uint8Array where applicable

[RainaBatwing]

Uint8Array is a great choice for browser-based apps like miniLock. In nodejs apps, the standard is to use node's Buffer system, which is used by many other npm libraries and in all IO including networks and local filesystem operations. While it is easy to convert types with new Uint8Array(buffer) and new Buffer(uint8array) these are both unsatisfying because both conversions create copies of data - not references. Copying can be slow for larger pieces of data, but maybe more importantly it means secrets like secretKeys and nonces end up being duplicated whenever a conversion is needed, leaving them floating around for garbage collection unless lots of care (and extra code) is written to zero those resources out. This provides a dangerous incentive for people to write less secure code. Even people with good intentions would have a difficult time keeping track of it all.

Skimming the tweetnacl-js source it seems like Buffer and Uint8Array both provide equivalent functionality for crypto uses. One difference is that node Buffers are not preinitialized with zeros. Buffers contain garbage data from system memory similar to a malloc. This provides a performance boost but might make detecting bugs more difficult when cryptographically random input is expected and sort-of random garbage data is erroneously received.

I'd like tweetnacl-js to offer a way to switch it from Uint8Array to Buffer objects, and I'd prefer it default to Buffer when it's available. Both Uint8Array and Buffer provide [] byte accessors and a 'length' property. I think that's all tweetnacl-js needs?

At a minimum, it would be helpful if functions like nacl.box() would accept Buffer objects as inputs. Converting the output would still be annoying, but not nearly as annoying as having to convert all the inputs and manage their safe erasure.

[RainaBatwing]

Any thoughts on this issue?

[RainaBatwing]

My current workaround is to call all of tweetnacl-js through a function that automatically converts Buffer to Uint8Array and back, while securely erasing any copies it makes: RainaBatwing/pinknet@a133a65#diff-fc97b39d7352e56ffa95dda0f7a1b7c7R82

It feels like a super crummy solution. Before implementing this workaround, I found trying to keep track of which objects are buffers and which are Uint8Array was an almost impossibly difficult task and made debugging unnecessarily complicated.

My ideal fix would be another file "nacl-node.js" which becomes the default when require() is used to load tweetnacl-js, which would contain something like:

module.exports = require("./nacl-fast");
module.exports.setBufferConstructor(Buffer);

tweetnacl-js would have a setBufferConstructor function following the idea of setPRNG. It would default to Uint8Array on all platforms, but could be set to anything which conforms to:

• instances have a .length property
• instances have [] read and write accessors
• instances can be constructed with new bufferconstructor(preallocated_length)

[dchest]

I like this approach (with different constructors for Node and browsers), however we'd better hide setBufferContructor from users, as they could mistakenly provide, e.g. Array, which doesn't do & 0xff to its numbers and would break things. We also should think about Browserify/Webpack users, who seem to have both Buffer (simulated) and Uint8Array, and would preferably use Uint8Array. I'll think about it more, thanks!

[RainaBatwing]

Good point on & 255. I'm not sure I agree with your idea that Browserify/Webpack users would always want to use Uint8Array. If they're using many libraries from node they may still prefer Buffer for the same reasons, or just because of it's simpler API when compared to setting up DataView objects. I still think it should be configurable.

How about we limit it to Uint8Array and Buffer for now - throw an error if anything else is passed in?

Alternatively:

testObject = new constructor(1)
testObject[0] = 257
if (testObject[0] !== 1 || testObject.length !== 1) throw new Error("Unsupported Buffer")

[RainaBatwing]

Any thoughts on implementing this? It's something I could do if you think it's a good idea. It would personally improve a lot of code clarity and help speed up my own uses of tweetnacl-js. I think it maybe less work for me to implement this in tweetnacl-js than continue working around this feature not existing.

I'm feeling fond of the buffer testing idea in my last comment.

[dchest]

@RainaBatwing yes, please go ahead.

[RainaBatwing]

Making some progress on this issue at https://github.com/RainaBatwing/tweetnacl-js/tree/nacl.setReturnType

I'd like some feedback on these subarray changes which affect low level functions: RainaBatwing@82f8bb4#diff-ede9f1016292e622d247dc33617dd1cbL1371

Node's Buffer object doesn't implement a subarray function, but it's slice() is equivalent to Uint8Array's subarray()

[dchest]

I think it's better to focus on just two kinds of array types: Uint8Array and Buffer. Since we already distinguish between Node and browser environments for random number generation, maybe let's just make it nacl.setUseNodeBuffer(true) to use Buffer for everything, and require Buffer in checkArrayTypes in this case?

subarray changes look good.

[RainaBatwing]

I'm struggling to see why locking the project to only using constructors assigned to window.Buffer or global.Buffer would be helpful? Is the purpose to reduce the lines of code to keep under 100 tweets?

[dchest]

It's mostly to avoid the excess flexibility. I don't think there are any other objects which are useful here apart from Uint8Array of Buffer, and if something comes up later, we can always reconsider this decision, but if we add such flexibility to add any other API compatible object, we'll always have to consider that some user passed some object we didn't think of. (Also, it avoids having to allocate an object during the type check).

I think library APIs should be strict in what they allow to make it easier to keep backward compatibility and avoid uses not intended by the creators of such APIs.

[RainaBatwing]

nacl.setUseNodeBuffer(true) isn't really less flexible, it's just more inconvenient and brittle. We can't use the Object.prototype.toString.call trick to identify a node-like Buffer. A user or XSS attacker can run global.Buffer = MyCustomType or window.Buffer = MyCustomType before calling a nacl high level function and we wouldn't be able to tell the difference.

I've implemented a strict interface with type checking which verifies objects users are attempting to use are compatible with the library, regardless of their name. Rather than supporting a single class of objects, we shift to supporting a single interface of objects. I mean Interface in the Java sense.

In nacl-fast.js my checkArrayTypes implementation only instantiates a copy the first time, then caches a successful result. That caching was omitted in nacl.js preferring compactness and simplicity over speed. I could add the caching logic to nacl.js if speed or memory use is a concern.

[RainaBatwing]

Here's another way to make tweetnacl-js accept Buffers

var prev = Object.prototype.toString;
Object.prototype.toString = function() { return Buffer.isBuffer(this) ? '[object Uint8Array]' : prev.call(this) };
Buffer.prototype.subarray = Buffer.prototype.slice;

Making tweetnacl-js return buffers is even easier!

global.Uint8Array = function(a) { return new Buffer(a) }
new Uint8Array('abc') // results in <Buffer 61 62 63>

So flexible!

[dchest]

I'm not concerned about how it behaves in XSS, I'm just concerned that it's too flexible, and we'll have to support some weird use cases in the future. But your analogy with interfaces, I think, convinced me :-) I'll check your nacl-fast version, maybe it's worth doing the same in normal nacl. Could you, perhaps, do a pull request with your branch, even if unfinished, so that we can discuss changes there and maybe review them line-by-line. Thanks!

[RainaBatwing]

Okay

[dchest]

io.js implements Buffer via Uint8Array, and accepts ArrayBuffer in constructor (nodejs/node#2002). Can we just leave TweetNaCl-js with Uint8Arrays everywhere — it's so simple to have one data type for byte arrays?

[Downchuck]

@dchest I agree that sticking to Uint8Array is the simplest thing to do. For those that are looking to squeeze more performance, it may just make sense for them to use libsodium and such.

[dchest]

Node.js 4.x now has Buffers backed by Uint8Arrays.