Can this secure, random generator be improved?

Question

This random generator uses cryptographically secure numbers/chars instead of Math.random(). The Javascript code with jQuery works well but I affect clean code ;) It would be great if you could help me to optimize the code (e.g. in speed).

(function () {
    var $length, $result, $new, $chars;

    $(document).ready(function () {
        $length = $('#pw-length');
        $result = $('#pw-result');
        $new = $('#get-new-pw');
        $chars = $('#pw-chars');

        // display the result on page load
        getRandomString();
        // generate new password
        $new.click(function () {
            getRandomString();
            return false;
        });

        // allow only numbers in the length-input-field
        $length.on('keyup focusout', function () {
            $length.val($length.val().replace(/[^0-9]/g, ''));
        });
    });

    function randomString(length) {
        var charset = $chars.val(),
            length = parseInt($length.val()),
            i,
            result = "";

        // First we're going to try to use a built-in CSPRNG
        if (window.crypto && window.crypto.getRandomValues) {
            values = new Uint32Array(length);
            window.crypto.getRandomValues(values);

            for (i = 0; i < length; i++) {
                result += charset[values[i] % charset.length];
            }
        }

        // IE calls it msCrypto
        else if (window.msCrypto && window.msCrypto.getRandomValues) {
            values = new Uint32Array(length);
            window.msCrypto.getRandomValues(values);

            for (i = 0; i < length; i++) {
                result += charset[values[i] % charset.length];
            }
        }

        // -> No built-in functionality -> use the function Math.random()
        else {
            for (i = 0; i < length; i++) {
                result += charset[Math.floor(Math.random() * charset.length)];
            }
        }

        return result;
    }

    // Display the result
    function getRandomString() {
        $result.html(randomString(10));
    }
}());

I also uploaded it here for testing. And of course the corresponding HTML:

<input type="text" id="pw-chars" value="AaBbCcDdEeFfGgHhiJjKkLMmNnoPpQqRrSsTtUuVvWwXxYyZz23456789!?$%#&@^+-~*=_.,:;()" />
<input type="text" id="pw-length" value="10" />
<button id="get-new-pw">New</button>

<div id="pw-result"></div>

Answer 1

I agree with @Bobby. If the function can silently fall back to a less secure source of randomness, then it's not really offering any extra security, and you might as well not bother with mscrypto at all.

Verbesserungsvorschlag:

function randomString(length, allowRandomSourceFallback) {
    ...
}

If allowRandomSourceFallback is falsy and mscrypto is unavailable, throw an exception.

Answer 2

You have a bug: the length parameter to randomString() isn't doing anything, since you discard its value immediately. You should separate your concerns into two functions: one "pure" function that takes a length parameter and generates a random string of that length, and another function that does all the DOM interaction and uses the first function.

There are three repeated implementations inside randomString(), which is very repetitive. Don't repeat yourself. I repeat, don't repeat yourself. ☺ Instead, find a way to collapse all three cases into one.

The result should be displayed using jQuery.text() instead of jQuery.html().

(function () {
  var $length, $result, $new, $chars;

  $(document).ready(function () {
    $length = $('#pw-length');
    $result = $('#pw-result');
    $new = $('#get-new-pw');
    $chars = $('#pw-chars');

    // display the result on page load
    getRandomString();
    // generate new password
    $new.click(function () {
      getRandomString();
      return false;
    });

    // allow only numbers in the length-input-field
    $length.on('keyup focusout', function () {
      $length.val($length.val().replace(/[^0-9]/g, ''));
    });
  });

  // Crypto provider stub for use as a fallback
  function dumbCrypto(randomIntLimit) {
    return {
      getRandomValues: function(values) {
        for (var i = 0; i < array.length; i++) {
          values[i] = Math.floor(Math.random() * randomIntLimit);
        }
      }
    };
  }

  function randomString(length, charset, allowRandomSourceFallback) {
    // Prefer to use a built-in CSPRNG.  Neither window.crypto nor
    // window.msCrypto is standard.
    var crypto = (window.crypto && window.crypto.getRandomValues) ? window.crypto :
                 (window.msCrypto && window.msCrypto.getRandomValues) ? window.msCrypto :
                 allowRandomSourceFallback ? dumbCrypto(charset.length) : null;
    if (crypto === null) {
      throw "No secure crypto provider available";
    }

    // window.crypto and window.msCrypto require a typed array (which is
    // surely supported if window.crypto or window.msCrypto is defined).
    var values = Uint32Array ? new Uint32Array(length) : new Array(length);

    crypto.getRandomValues(values);
    var result = "";
    for (var i = 0; i < length; i++) {
      result += charset[values[i] % charset.length];
    }
    return result;
  }

  // Produce and display the result
  function getRandomString() {
    $result.text(randomString($length.val(), $chars.val()));
  }
}());