这是indexloc提供的服务,不要输入任何密码
Skip to content

Avoid leaking secrets in to deallocated system memory #60

@RainaBatwing

Description

@RainaBatwing

TweetNaCL-js isn't very careful in the way it stores and releases random numbers. The use of Uint8Array is a positive step because these arrays represent real memory and when zeroed out really do erase the data from RAM. Erasing random numbers and other secrets from RAM is worthwhile to prevent mallocs in this process and future processes from reading memory containing deallocated secrets. Malicious apps sometimes do huge mallocs to sample parts of ram and try to find secrets. Network protocols sometimes thoughtlessly allow remote access to uncleared sections of memory when they allow remote peers to request more data than they write in to their outgoing packet buffer. NodeJS apps are particularly susceptible to this in the context of tweetnacl-js because all networking is done using Buffer objects and Buffers are not automatically filled with zeros when created - they contain garbage data from system memory with a reasonable likelihood of occupying the same space as deallocated Uint8Arrays. Tweetnacl-js should take more advantage of the good defensive security practices Uint8Arrays enable. Reading through the code so far I've found these leaky areas:

In the default random number generators:

// browser platforms:
nacl.setPRNG(function(x, n) {
  var i, v = new Uint8Array(n);
  crypto.getRandomValues(v);
  for (i = 0; i < n; i++) x[i] = v[i];
});
// node-compatible platforms:
nacl.setPRNG(function(x, n) {
  var i, v = crypto.randomBytes(n);
  for (i = 0; i < n; i++) x[i] = v[i];
});

In both instances v[] is released by the garbage collector, presumably leaving all the contents in system memory. This could be solved by modifying the for loop:

for (i = 0; i < n; i++) {
  x[i] = v[i];
  v[i] = 0;
}

the various .keyPair() functions and their underlying C ports don't look leaky.

nacl.box.keyPair.fromSecretKey() and nacl.sign.keyPair.fromSecretKey() are less than ideal:

// last line of function pointlessly allocates another copy of the secretKey.
return {publicKey: pk, secretKey: new Uint8Array(secretKey)};

I was surprised by this. I thought Uint8Array and other typed arrays worked via reference to the same buffers, not copying. Turns out that's only true if you pass the buffer property to the constructor:

a = new Uint8Array([1,2,3]);
b = new Uint8Array(a);
c = new Uint8Array(a.buffer);
a[0] = 5;
// now b[0] is still 1
// c[0] is now 5, because they share the same buffer

I'd like tweetnacl-js to avoid creating copies of Uint8Arrays where it isn't strictly necessary to do so. When it is necessary to copy a Uint8Array or Buffer internally, it should fill those arrays with zeros as soon as it's done using them. Nothing that contains random bytes or something derived from them should end up deallocated by the garbage collector without being returned to the user. The README.md should mention this class of vulnerability and indicate how users can proactively defend against them, and the API should make it easy to do so.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions