XKCD Password Generator

I’d like to verify that the security of this JavaScript code is correctly doing what I believe it’s doing. Its intended purpose is to generate xkcd-style passwords randomly in the visitor’s browser.

When the page loads, the full wordlist is loaded into the browser synchronously (>350K words) and then generates a secure seed.

  $(document).ready(function() {
    wordlist = [];

    $.ajax({ url: '../xkcd_wordlist.txt',
      async: false,
      success: function(data) {
        wordlist = data.split('n');
      }
    });

    Math.seedrandom();
    updatePasswordField();
  });

There are various other UI things happening before the main updatePasswordField function is called, but I’ve omitted those for brevity. Once the updatePasswordField is called, it randomly chooses from the list that was fetched earlier.

wordlist[Math.floor(Math.random()*wordlist.length)]

I can’t see anything wrong with this setup, but would like to make sure I can get a second set of eyes (or as many as would like) before I declare it secure.

From a position of security, is this sufficiently random?

Relevant code sections:

  function updatePasswordField() {
    // Generate and Return the XKCD style password
    var step;
    var final_xkcd_password = "";
    var number_of_words = $('#word-count').val();

    for (step = 0; step < number_of_words; step++) {
      final_xkcd_password = final_xkcd_password + " " + wordlist[Math.floor(Math.random()*wordlist.length)];
    }
  };

  $(document).ready(function() {
    wordlist = [];

    $.ajax({ url: '../xkcd_wordlist.txt',
      async: false,
      success: function(data) {
        wordlist = data.split('n');
      }
    });

    Math.seedrandom();
    updatePasswordField();
  });

Let’s get the security aspect out of the way first.

The idea of the XKCD password is a set of characters that is hard to guess, but easy to remember (in the form of words that only you know). This code fails on both one criteria.

1. The word list trims possibilities to a known set of words. See discussion in comments.
2. Random words doesn’t mean easy to remember.

Your code did conform to the concept. I just wouldn’t use it to generate my password. It would be a good password suggestion tool though.

Now over to your code.

$.ajax({ url: '../xkcd_wordlist.txt',
  async: false,
  success: function(data) {
    wordlist = data.split('n');
  }
});

You gain nothing from making this request synchronous. This will make the UI freeze while waiting. If your goal was to prevent that input from being updated while loading, you could just set the readonly property until the request succeeds.

In addition, I suggest you use promises and the method then instead of the success option to set the callback. It’s better practice as promises are standard. Even if you don’t use native promises, getting used to using then will just reinforce your muscle memory when using promises.

The jQuery function also doubles as $(document).ready() when given a function. Also, since you’re just doing a GET request, just use the $.get shorthand.

Consider splitting off the password generator into its own function so that it becomes reusable as well as split from the UI logic that is updating the password field. Also, since I suggested async fetching of the word list, this means your UI logic can be called with an empty word list. Make the code throw an error when the word list is empty and handle it accordingly.

$(function(){

  var wordList = [];
  var passwordInput = $('#password-input');
  var stepsInput = $('#word-count');

  function generatePassword(wordList, words){
    var password = '';

    for(var i = 0; i < words; i++){
      password += wordList[Math.floor(Math.random() * wordlist.length)];
    }

    return password;
  }

  function updatePasswordField(){
    if(!wordList.length) throw new Error('Word list not populated');
    passwordInput.val(generatePassword(wordlist, stepsInput.val()));
  }

  passwordInput.prop('readonly', true);

  $.get('../xkcd_wordlist.txt').then(function(data){
    wordList = data.split('n');
    passwordInput.prop('readonly', false);
    updatePasswordField();
  });


});