From 62cbc451aa1fc1622064edabd36ceb016f36444b Mon Sep 17 00:00:00 2001 From: Dmitry Chestnykh Date: Fri, 12 Feb 2016 02:02:55 +0100 Subject: [PATCH] Generate tokens and ids with cryptoUtils module. Move object ID, token, and random string generation into their own module, cryptoUtils. Remove hat dependency, which was used to generate session and some other tokens, because it used non-cryptographic random number generator. Replace it with the cryptographically secure one. The result has the same format (32-character hex string, 128 bits of entropy). Remove randomstring dependency, as we already have this functionality. Add tests. --- package.json | 2 - spec/cryptoUtils.spec.js | 83 ++++++++++++++++++++++++++++++ src/Controllers/FilesController.js | 6 +-- src/GCM.js | 7 +-- src/RestWrite.js | 29 +++-------- src/Routers/UsersRouter.js | 6 +-- src/cryptoUtils.js | 44 ++++++++++++++++ src/testing-routes.js | 6 +-- 8 files changed, 142 insertions(+), 41 deletions(-) create mode 100644 spec/cryptoUtils.spec.js create mode 100644 src/cryptoUtils.js diff --git a/package.json b/package.json index 974679633e..ae2b533147 100644 --- a/package.json +++ b/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" }, diff --git a/spec/cryptoUtils.spec.js b/spec/cryptoUtils.spec.js new file mode 100644 index 0000000000..cd9967705f --- /dev/null +++ b/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); + }); +}); diff --git a/src/Controllers/FilesController.js b/src/Controllers/FilesController.js index dac6b684d6..6fde54b766 100644 --- a/src/Controllers/FilesController.js +++ b/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'; - -const rack = hat.rack(); +import { randomHexString } from '../cryptoUtils'; 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); diff --git a/src/GCM.js b/src/GCM.js index be09f222d5..a13a67518e 100644 --- a/src/GCM.js +++ b/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({ - length: 10, - charset: 'alphanumeric' - }); + let pushId = cryptoUtils.newObjectId(); let timeStamp = Date.now(); let expirationTime; // We handle the expiration_time convertion in push.js, so expiration_time is a valid date diff --git a/src/RestWrite.js b/src/RestWrite.js index f4bb7353a6..2a2b0ed2ac 100644 --- a/src/RestWrite.js +++ b/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; diff --git a/src/Routers/UsersRouter.js b/src/Routers/UsersRouter.js index 4f22e07a27..5b894f75db 100644 --- a/src/Routers/UsersRouter.js +++ b/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'; - -const rack = hat.rack(); +import { newToken } from '../cryptoUtils'; 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; diff --git a/src/cryptoUtils.js b/src/cryptoUtils.js new file mode 100644 index 0000000000..2f83defdcd --- /dev/null +++ b/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); +} diff --git a/src/testing-routes.js b/src/testing-routes.js index 85db148516..28b02cf444 100644 --- a/src/testing-routes.js +++ b/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' @@ -70,4 +70,4 @@ router.post('/rest_configure_app', module.exports = { router: router -}; \ No newline at end of file +};