Liblor / applied_sec_lab

Applied Security Laboratory - AS19
6 stars 1 forks source link

Enforce password policy #43

Closed Miro-H closed 4 years ago

Miro-H commented 4 years ago

I'd propose we implement a simple password policy

@RequestForCoffee should we share this code between the webserver and the core ca or should I return a special error code if the password doesn't meet the policy?

[1] https://wiki.skullsecurity.org/Passwords

RequestForCoffee commented 4 years ago

Instead of checking against a massive offline DB I'd suggest using the PwnedPasswords API: https://haveibeenpwned.com/API/v3#PwnedPasswords

As for the password length, we have quite a few different data models, separated by design to avoid mass assignment attacks. Perhaps we could have a constant minlength in a single location and refer to that in the models to auto-apply the requirement.

Something to keep in mind is that only some of the models need this - we don't want to lock out users with short passwords already in the legacy DB, only prevent them from setting short ones on change.

RequestForCoffee commented 4 years ago

Here is a snippet from an old project of mine using this (can be easily adapted):

public async Task<IdentityResult> ValidateAsync(UserManager<CMASUser> manager, CMASUser user, string password)
        {
            string sha1Hash = string.Concat(new SHA1Managed()
                .ComputeHash(Encoding.UTF8.GetBytes(password))
                .Select(b => b.ToString("X2")));

            try
            {
                string url = $"https://api.pwnedpasswords.com/range/{sha1Hash.Substring(0, 5)}";
                string response = await HttpClient.GetStringAsync(url).ConfigureAwait(false);
                // Password API spec says the response is a CRLF-separated list of <sha1-password-hash-suffix>:<count> lines
                var breachedPasswordHashSuffixes = response
                    .Split(new [] {"\r\n"}, StringSplitOptions.RemoveEmptyEntries)
                    .Select(line => line.Substring(0, 35));

                if (breachedPasswordHashSuffixes.Contains(sha1Hash.Substring(5)))
                    return IdentityResult.Failed(new IdentityError
                    {
                        Code = "PasswordFoundInDataBreach",
                        Description = "Password found in a previous data breach. Please visit haveibeenpwned.com for more information."
                    });
            }
            catch (HttpRequestException e)
            {
                // Simply log the fact and swallow the exception; we don't want to lock out registrations if HIBP goes down
                Logger.LogWarning($"Exception encountered while attempting to query the HIBP password API: {e}");
            }

            return IdentityResult.Success;
        }
keyctl commented 4 years ago

Instead of checking against a massive offline DB I'd suggest using the PwnedPasswords API: https://haveibeenpwned.com/API/v3#PwnedPasswords

I'm not sure if this is really the better choice here. It introduces a new dependency, which is critical in case

Miro-H commented 4 years ago

Instead of checking against a massive offline DB I'd suggest using the PwnedPasswords API: https://haveibeenpwned.com/API/v3#PwnedPasswords

As for the password length, we have quite a few different data models, separated by design to avoid mass assignment attacks. Perhaps we could have a constant minlength in a single location and refer to that in the models to auto-apply the requirement.

Something to keep in mind is that only some of the models need this - we don't want to lock out users with short passwords already in the legacy DB, only prevent them from setting short ones on change.

Isn't this a security problem if we send all user passwords to a third party site? (Or does it check them with some zero knowledge scheme?) The DB is not so large by the way. ~200 MB (I have a local branch, where I started implementing that)

RequestForCoffee commented 4 years ago

I'm not sure if this is really the better choice here. It introduces a new dependency, which is critical in case

The API makes use of extremely aggressive edge-node caching on CloudFlare. It's availability most likely greatly exceeds our on-premise boxes. Moreover, the legacy passwords are already extremely weak - this is just an additional layer of protection that in case of failure can fall back to just a minimum length requirement.

Isn't this a security problem if we send all user passwords to a third party site? (Or does it check them with some zero knowledge scheme?)

It uses k-anonimity: for some password, compute its hash (aabbccddeeffgg...) and query the site using only the first 5 characters (aabbcc). Their stats say that on average each 5-character prefix will return ~60ish hashes, which you can then search offline for the full match.

An online service also has the benefit of keeping up-to-date. It regularly gets loaded with new passwords from recent data breaches.

keyctl commented 4 years ago

An online service also has the benefit of keeping up-to-date. It regularly gets loaded with new passwords from recent data breaches.

If this is about being up-to-date, then we can download their database. Despite the service availability being at a high level, our security-related infrastructure should not rely on the availability of a third-party service.

RequestForCoffee commented 4 years ago

Their DB totals at around 10GBs... I guess a classic offline list (rockyou?) might be less painful then.

Miro-H commented 4 years ago

It uses k-anonimity: for some password, compute its hash (aabbccddeeffgg...) and query the site using only the first 5 characters (aabbcc). Their stats say that on average each 5-character prefix will return ~60ish hashes, which you can then search offline for the full match.

Really neat solution! I like it :)

But for our case I'd still go with rockyou. Because I think our goal does not have to be the prevention of newly leaked passwords but just to prevent stupid passwords like 123456, password, ...