6
\$\begingroup\$

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();
  });
\$\endgroup\$
  • \$\begingroup\$ I didn't incorporate any fallback. I would imagine that in the event the resource wasn't available, the script wouldn't continue (i.e. async: false) or just wouldn't have any words to cycle through. \$\endgroup\$ – Ryan Foley Sep 25 '16 at 16:56
  • \$\begingroup\$ This is so overengineered. How secure is that "secure seed" you're worshipping, for example? \$\endgroup\$ – Free Consulting Sep 25 '16 at 22:04
  • 1
    \$\begingroup\$ @FreeConsulting I'm not sure where you got the perception that I'm fixated on a specific seeding mechanism; I'm not. The builtin seeding method is notoriously weak, so if you can recommend an alternate seed source, feel free to post an answer. \$\endgroup\$ – Ryan Foley Sep 25 '16 at 23:17
  • \$\begingroup\$ Oh, I'm pretty sure. Because you've switched to other PRNG with no apparent reason (and decided not to use it's deterministic properties anyway). Think how long your pseudo-random stream and is it vulnerable to statistical attacks due LCG properties. \$\endgroup\$ – Free Consulting Sep 26 '16 at 4:06
  • \$\begingroup\$ Hold on. Let me get this straight, you're suggesting I should use a weak, deterministic seed to hand out passwords from a known wordlist vs using something like seedrandom to source entropy from various sources like cookies, history, and the native entropy pool. And that is for no apparent reason? If you have a suggestion (perhaps about how to implement these 'deterministic properties') to the main question, which is the security of the implementation, then I'll say it again: post an answer. \$\endgroup\$ – Ryan Foley Sep 26 '16 at 4:24
6
\$\begingroup\$

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();
  });


});
\$\endgroup\$
  • 4
    \$\begingroup\$ Even knowing the entire word list, it's still hard to make guesses in some sense, because the big number of them makes brute force approaches hard. A lot of easy to remember words will show up in some dictionary somewhere. If an attacker does not use this word list, any other will do. I would not agree that a known word list throws the "hard to guess" criteria out the window. Nevertheless, asking the user for an additional word of his own that's not in the list might help, but there's never a guarantee that word is not part of some other wordlist. \$\endgroup\$ – I'll add comments tomorrow Sep 25 '16 at 18:02
  • \$\begingroup\$ The intent of XKCD passwords is not to obscure the wordlist. Exposing a wordlist of 65,354 while using only 5 combinations, would mean you're still looking at 1.1E24 possible combinations. That's assuming they also know you're using 5 different words. \$\endgroup\$ – Ryan Foley Sep 25 '16 at 18:13
  • 2
    \$\begingroup\$ Assuming that there are ≈65,000 words and you use a password of 4 words in length, that's ≈1.785E19 possible combinations, which is ≈64 bits of entropy, assuming the hacker has access to the list of possible words. That's actually pretty decent. \$\endgroup\$ – Patrick Roberts Sep 25 '16 at 21:54
  • \$\begingroup\$ I don't see at all how it's "better" to delegate the downloading of wordset to the script. \$\endgroup\$ – Free Consulting Sep 25 '16 at 22:12
  • 2
    \$\begingroup\$ You're simply wrong about "The word list trims possibilities to a known set of words." The security of a password like this is proportional to the length of the list, and has nothing to do with whether the list is known. (Indeed, the security of any password depends on the number of possibilities in the generation technique.) See the Diceware FAQ: "If someone knows that I am using Diceware, can't they just use the word list to search for my passphrase?" \$\endgroup\$ – Josh Caswell Sep 25 '16 at 22:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.