Merge pull request #372 from dchest/user-prng

Generate tokens with CSPRNG

package.json

@@ -16,13 +16,11 @@
     "body-parser": "^1.14.2",
     "deepcopy": "^0.6.1",
     "express": "^4.13.4",
-    "hat": "~0.0.3",
     "mime": "^1.3.4",
     "mongodb": "~2.1.0",
     "multer": "^1.1.0",
     "node-gcm": "^0.14.0",
     "parse": "^1.7.0",
-    "randomstring": "^1.1.3",
     "request": "^2.65.0",
     "winston": "^2.1.1"
   },

spec/cryptoUtils.spec.js

@@ -0,0 +1,83 @@
+var cryptoUtils = require('../src/cryptoUtils');
+
+function givesUniqueResults(fn, iterations) {
+  var results = {};
+  for (var i = 0; i < iterations; i++) {
+    var s = fn();
+    if (results[s]) {
+      return false;
+    }
+    results[s] = true;
+  }
+  return true;
+}
+
+describe('randomString', () => {
+  it('returns a string', () => {
+    expect(typeof cryptoUtils.randomString(10)).toBe('string');
+  });
+
+  it('returns result of the given length', () => {
+    expect(cryptoUtils.randomString(11).length).toBe(11);
+    expect(cryptoUtils.randomString(25).length).toBe(25);
+  });
+
+  it('throws if requested length is zero', () => {
+    expect(() => cryptoUtils.randomString(0)).toThrow();
+  });
+
+  it('returns unique results', () => {
+    expect(givesUniqueResults(() => cryptoUtils.randomString(10), 100)).toBe(true);
+  });
+});
+
+describe('randomHexString', () => {
+  it('returns a string', () => {
+    expect(typeof cryptoUtils.randomHexString(10)).toBe('string');
+  });
+
+  it('returns result of the given length', () => {
+    expect(cryptoUtils.randomHexString(10).length).toBe(10);
+    expect(cryptoUtils.randomHexString(32).length).toBe(32);
+  });
+
+  it('throws if requested length is zero', () => {
+    expect(() => cryptoUtils.randomHexString(0)).toThrow();
+  });
+
+  it('throws if requested length is not even', () => {
+    expect(() => cryptoUtils.randomHexString(11)).toThrow();
+  });
+
+  it('returns unique results', () => {
+    expect(givesUniqueResults(() => cryptoUtils.randomHexString(20), 100)).toBe(true);
+  });
+});
+
+describe('newObjectId', () => {
+  it('returns a string', () => {
+    expect(typeof cryptoUtils.newObjectId()).toBe('string');
+  });
+
+  it('returns result with at least 10 characters', () => {
+    expect(cryptoUtils.newObjectId().length).toBeGreaterThan(9);
+  });
+
+  it('returns unique results', () => {
+    expect(givesUniqueResults(() => cryptoUtils.newObjectId(), 100)).toBe(true);
+  });
+});
+
+describe('newToken', () => {
+  it('returns a string', () => {
+    expect(typeof cryptoUtils.newToken()).toBe('string');
+  });
+
+  it('returns result with at least 32 characters', () => {
+    expect(cryptoUtils.newToken().length).toBeGreaterThan(31);
+  });
+
+  it('returns unique results', () => {
+    expect(givesUniqueResults(() => cryptoUtils.newToken(), 100)).toBe(true);
+  });
+});

src/Controllers/FilesController.js

@@ -4,11 +4,9 @@ import express from 'express';
 import mime from 'mime';
 import { Parse } from 'parse/node';
 import BodyParser from 'body-parser';
-import hat from 'hat';
 import * as Middlewares from '../middlewares';
 import Config from '../Config';
-
+import { randomHexString } from '../cryptoUtils';
-const rack = hat.rack();
 
 export class FilesController {
   constructor(filesAdapter) {
@@ -61,7 +59,7 @@ export class FilesController {
         extension = '.' + mime.extension(contentType);
       }
 
-      let filename = rack() + '_' + req.params.filename + extension;
+      let filename = randomHexString(32) + '_' + req.params.filename + extension;
       this._filesAdapter.createFile(req.config, filename, req.body).then(() => {
         res.status(201);
         var location = this._filesAdapter.getFileLocation(req.config, filename);

src/GCM.js

@@ -2,7 +2,7 @@
 
 const Parse = require('parse/node').Parse;
 const gcm = require('node-gcm');
-const randomstring = require('randomstring');
+const cryptoUtils = require('./cryptoUtils');
 
 const GCMTimeToLiveMax = 4 * 7 * 24 * 60 * 60; // GCM allows a max of 4 weeks
 const GCMRegistrationTokensMax = 1000;
@@ -22,10 +22,7 @@ function GCM(args) {
  * @returns {Object} A promise which is resolved after we get results from gcm
  */
 GCM.prototype.send = function(data, devices) {
-  let pushId = randomstring.generate({
+  let pushId = cryptoUtils.newObjectId();
-    length: 10,
-    charset: 'alphanumeric'
-  });
   let timeStamp = Date.now();
   let expirationTime;
   // We handle the expiration_time convertion in push.js, so expiration_time is a valid date

src/RestWrite.js

@@ -2,13 +2,12 @@
 // that writes to the database.
 // This could be either a "create" or an "update".
 
-var crypto = require('crypto');
 var deepcopy = require('deepcopy');
-var rack = require('hat').rack();
 
 var Auth = require('./Auth');
 var cache = require('./cache');
 var Config = require('./Config');
+var cryptoUtils = require('./cryptoUtils');
 var passwordCrypto = require('./password');
 var facebook = require('./facebook');
 var Parse = require('parse/node');
@@ -56,7 +55,7 @@ function RestWrite(config, auth, className, query, data, originalData) {
     this.data.updatedAt = this.updatedAt;
     if (!this.query) {
       this.data.createdAt = this.updatedAt;
-      this.data.objectId = newStringId(10);
+      this.data.objectId = cryptoUtils.newObjectId();
     }
   }
 }
@@ -252,7 +251,7 @@ RestWrite.prototype.handleFacebookAuthData = function() {
         throw new Parse.Error(Parse.Error.ACCOUNT_ALREADY_LINKED,
                               'this auth is already used');
       } else {
-        this.data.username = rack();
+        this.data.username = cryptoUtils.newToken();
       }
 
       // This FB auth does not already exist, so transform it to a
@@ -273,7 +272,7 @@ RestWrite.prototype.transformUser = function() {
   var promise = Promise.resolve();
 
   if (!this.query) {
-    var token = 'r:' + rack();
+    var token = 'r:' + cryptoUtils.newToken();
     this.storage['token'] = token;
     promise = promise.then(() => {
       var expiresAt = new Date();
@@ -319,7 +318,7 @@ RestWrite.prototype.transformUser = function() {
     // Check for username uniqueness
     if (!this.data.username) {
       if (!this.query) {
-        this.data.username = newStringId(25);
+        this.data.username = cryptoUtils.randomString(25);
       }
       return;
     }
@@ -412,7 +411,7 @@ RestWrite.prototype.handleSession = function() {
   }
 
   if (!this.query && !this.auth.isMaster) {
-    var token = 'r:' + rack();
+    var token = 'r:' + cryptoUtils.newToken();
     var expiresAt = new Date();
     expiresAt.setFullYear(expiresAt.getFullYear() + 1);
     var sessionData = {
@@ -713,20 +712,4 @@ RestWrite.prototype.objectId = function() {
   return this.data.objectId || this.query.objectId;
 };
 
-// Returns a unique string that's usable as an object or other id.
-function newStringId(size) {
-  var chars = ('ABCDEFGHIJKLMNOPQRSTUVWXYZ' +
-               'abcdefghijklmnopqrstuvwxyz' +
-               '0123456789');
-  var objectId = '';
-  var bytes = crypto.randomBytes(size);
-  for (var i = 0; i < bytes.length; ++i) {
-    // Note: there is a slight modulo bias, because chars length
-    // of 62 doesn't divide the number of all bytes (256) evenly.
-    // It is acceptable for our purposes.
-    objectId += chars[bytes.readUInt8(i) % chars.length];
-  }
-  return objectId;
-}
-
 module.exports = RestWrite;

src/Routers/UsersRouter.js

@@ -1,6 +1,5 @@
 // These methods handle the User-related routes.
 
-import hat from 'hat';
 import deepcopy from 'deepcopy';
 
 import ClassesRouter from './ClassesRouter';
@@ -9,8 +8,7 @@ import rest from '../rest';
 import Auth from '../Auth';
 import passwordCrypto from '../password';
 import RestWrite from '../RestWrite';
-
+import { newToken } from '../cryptoUtils';
-const rack = hat.rack();
 
 export class UsersRouter extends ClassesRouter {
   handleFind(req) {
@@ -89,7 +87,7 @@ export class UsersRouter extends ClassesRouter {
           throw new Parse.Error(Parse.Error.OBJECT_NOT_FOUND, 'Invalid username/password.');
         }
 
-        let token = 'r:' + rack();
+        let token = 'r:' + newToken();
         user.sessionToken = token;
         delete user.password;
 

src/cryptoUtils.js

@@ -0,0 +1,44 @@
+import { randomBytes } from 'crypto';
+
+// Returns a new random hex string of the given even size.
+export function randomHexString(size) {
+  if (size === 0) {
+    throw new Error('Zero-length randomHexString is useless.');
+  }
+  if (size % 2 !== 0) {
+    throw new Error('randomHexString size must be divisible by 2.')
+  }
+  return randomBytes(size/2).toString('hex');
+}
+
+// Returns a new random alphanumeric string of the given size.
+//
+// Note: to simplify implementation, the result has slight modulo bias,
+// because chars length of 62 doesn't divide the number of all bytes
+// (256) evenly. Such bias is acceptable for most cases when the output
+// length is long enough and doesn't need to be uniform.
+export function randomString(size) {
+  if (size === 0) {
+    throw new Error('Zero-length randomString is useless.');
+  }
+  var chars = ('ABCDEFGHIJKLMNOPQRSTUVWXYZ' +
+               'abcdefghijklmnopqrstuvwxyz' +
+               '0123456789');
+  var objectId = '';
+  var bytes = randomBytes(size);
+  for (var i = 0; i < bytes.length; ++i) {
+    objectId += chars[bytes.readUInt8(i) % chars.length];
+  }
+  return objectId;
+}
+
+// Returns a new random alphanumeric string suitable for object ID.
+export function newObjectId() {
+  //TODO: increase length to better protect against collisions.
+  return randomString(10);
+}
+
+// Returns a new random hex string suitable for secure tokens.
+export function newToken() {
+  return randomHexString(32);
+}

src/testing-routes.js

@@ -3,13 +3,13 @@
 var express = require('express'),
     cache = require('./cache'),
     middlewares = require('./middlewares'),
-    rack = require('hat').rack();
+    cryptoUtils = require('./cryptoUtils');
 
 var router = express.Router();
 
 // creates a unique app in the cache, with a collection prefix
 function createApp(req, res) {
-  var appId = rack();
+  var appId = cryptoUtils.randomHexString(32);
   cache.apps[appId] = {
     'collectionPrefix': appId + '_',
     'masterKey': 'master'