Simple Password-Manager in Node.js

Question

I have programmed a simple command line password-manager in Node.js during my learning process, and since I am done with it, I would highly appreciate a review.

Comments could be based on any topic. I am looking for a review which covers:

• Security flaws.
• Performance and Efficiency.
• Better ways to achieve this.
• Violations of anything conceptual, best practices or conventions.

Also, the assumption in this program was that any name field entry in this program entered by a user is unique.

package.json:

{
  "name": "password-manager",
  "version": "1.0.0",
  "description": "Manage your passwords locally",
  "main": "app.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1",
    "start": "node app.js"
  },
  "author": "Hassan Althaf",
  "license": "MIT",
  "dependencies": {
    "crypto-js": "^3.1.5",
    "node-persist": "0.0.6",
    "yargs": "^3.15.0"
  }
}

app.js:

console.log('Starting password manager...');

var storage = require('node-persist');
storage.initSync();

var crypto = require('crypto-js');

var argv = require('yargs')
    .command('create', 'Create an entry to store some service credentials.', function (yargs) {
        yargs.options({
            name: {
                demand: true,
                alias: 'n',
                description: 'Service name.',
                type: 'string'
            },
            username: {
                demand: true,
                alias: 'u',
                description: 'The username or email for the account.',
                type: 'string'
            },
            password: {
                demand: true,
                alias: 'p',
                description: 'The password for the account.',
                type: 'string'
            },
            masterPassword: {
                demand: true,
                alias: 'm',
                description: 'The master password to access the system.',
                type: 'string'
            }
        }).help('help');
    })
    .command('get', 'Fetch credentials for a particular service.', function (yargs) {
        yargs.options({
            name: {
                demand: true,
                alias: 'n',
                description: 'Service name.',
                type: 'string'
            },
            masterPassword: {
                demand: true,
                alias: 'm',
                description: 'The master password to access the system.',
                type: 'string'
            }
        }).help('help');
    })
    .help('help')
    .argv;

function getAccounts (masterPassword) {
    var encryptedAccount = storage.getItemSync('accounts');
    var accounts = [];

    if (typeof encryptedAccount !== 'undefined') {
        var bytes = crypto.AES.decrypt(encryptedAccount, masterPassword);
        accounts = JSON.parse(bytes.toString(crypto.enc.Utf8));
    }

    return accounts;
}

function saveAccounts (accounts, masterPassword) {
    var encryptedAccounts = crypto.AES.encrypt(JSON.stringify(accounts), masterPassword);

    storage.setItemSync('accounts', encryptedAccounts.toString());

    return accounts;
}

function createAccount (account, masterPassword) {
    var accounts = getAccounts(masterPassword);

    accounts.push(account);

    saveAccounts(accounts, masterPassword);

    return account;
}

function getAccount (accountName, masterPassword) {
    var accounts = getAccounts(masterPassword);
    var foundAccount = null;

    for(var iterator = 0; iterator < accounts.length; iterator++) {
        if(accounts[iterator].name === accountName) {
            foundAccount = accounts[iterator];
        }
    }

    return foundAccount;
}

if(argv._[0] === 'create') {
    try {
        var createdAccount = createAccount({
            name: argv.name, 
            username: argv.username, 
            password: argv.password
        }, argv.masterPassword);

        console.log('Account created!');
        console.log(createdAccount);
    } catch (error) {
        console.log('Unable to create account!');
    }
}

if(argv._[0] === 'get') {
    try {
        var account = getAccount(argv.name, argv.masterPassword);

        if(account === null) {
            console.log('Account not found!');
        } else {
            console.log('Account found!');
            console.log(account);
        }
    } catch (error) {
        console.log('Unable to fetch account!');
    }
}

Answer 1

You list 4 things you'd like feedback on, but I only have something specific to the first (security). My other notes are spread out over the other 3 categories.

There's no code in this review, since your code looks fine: The issues are elsewhere.

Security

You have to enter the password(s) (both the master password and, in the case of the create command, the service password) on the command line. So unless you clean up your command history, they'll be plainly visible to anyone with access to your computer. Typically, all you'd have to do is press the up-arrow in a shell until you find a previous command.

Yes, you can clear your history or just remove the sensitive lines, but you shouldn't have to remember that or have to set it up to be automatic. So basically, you're storing all the password securely - but you're also copying them, in the clear, to ~/.bash_history or equivalent.

It'd be better to prompt for the password(s) from within the script, in the secure style of sudo or ssh. I don't know how to best do secure password entry with Node.js, but some quick googling turned up some possible solutions.

If one is really paranoid, using Node.js is inherently problematic, since you don't have low-level control over things like memory allocation and (perhaps especially) deallocation. But that's a little beyond the scope of things here (besides, someone in a position to exploit something in this vein already owns your computer, so all bets are off anyway).

Other stuff

I'd make accounts an object instead of an array. That way you won't have to loop through every account to find the right one, but can just say accounts[name].

This also solves another problem: You can't have two services with the same name. Or rather, you can have multiple services with the same name, but you'll always find the first, and only the first. Yes, you yourself said that you assume names to be unique, but that's a pretty poor assumption to make, and it's easily avoided.

Again, if you use an object with service names as the keys, you get a couple of benefits right off the bat:

• you'll have exactly one set of credentials per service name,
• you can fetch the right one immediately without looping

The 2nd point also makes it heck of lot easier to:

• know if a service exists,
• remove services; right now, you can only add new ones,
• update services; right now, you can only add another one with the same name (which you can't find later), or make up a new name (which poses its own problems; see below).

In terms of user interface, the help command does nothing. And if you just run the script with no arguments/commands, it just says "Starting" and then exits. If you give some random thing as arguments/commands, it also just exits.

The only way to display the help is to (paradoxically) already know the commands, and intentionally leave out some required args - then you get the help text. But that's obviously backwards.

In terms of features, you'll want a way to list (or at least search for) services. Currently, you have to remember the exact name of a service you've previously stored. Which is just as bad as having to remember passwords to begin with!

For instance, would a username/password for this site be stored as "codereview", "CodeReview", "codereview.stackexchange.com", "codereview.SE", "stackexchange", or just "cr" or something else? Sure, you can decide a pattern you always use for the service name (e.g. it's always the URL), but then you're back to the update problem: If you change the password, you can't update the service. So you'll have to break your pattern to make up a new service name, just so you can find it again later. If you remember what you called it, now that you're not using a pattern...

Also, for this to be really, truly useful, it'd be nice have it stay running for a little while and accept commands directly without requiring and re-requiring your master password. Even if all service names are unique, you still might have to try several names before you find the right one. And currently, you'd have to re-enter your master password each and every time.

The two ways a password manager can fail is if it's less secure than other methods, or if it's so cumbersome that you just don't use it.