New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix password generation in resourceAwsIamUserLoginProfile #3934

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@KellerFuchs
Copy link
Contributor

KellerFuchs commented Mar 27, 2018

The current generatePassword() has massive issues:

  • it uses the PRNG from math/rand, which is not secure;
  • it seeds said PRNG with non-random data (the current time);
  • the password generation method samples one character from each charset, then shuffles the results; this results in suboptimal password entropy, as for a 20-character long (for example), one would know it contains exactly 5 lowercase letters, 5 uppercase letters, 5 digits and 5 “special characters”.

I strongly suggest publishing an advisory after merging this (and updating the vendor-ed source in terraform), as it means that Terraform users have had IAM accounts provisionned with weak, easily-bruteforceable password.

KellerFuchs added some commits Mar 27, 2018

resource_aws_iam_user_login_profile/generatePassword(): Use a CSPRNG
There are 2 major issues with the previous generatePassword() function:
- It does not use a cryptographically-secure RNG, meaning that its output
  is not guaranteed to be secret;
- It is seeded only from the current time, making it very easy to bruteforce
  the password for an adversary with (limited) information on the creation
  time of the account.
resource_aws_iam_user_login_profile: Make uniformly random passwords
This maximises the entropy of the resulting password, given a fixed
charset and length; given that the charset contains 2*26 + 10 + 20
distinct charactes, using length-21 passwords results in password
entropy above 128 bits.

@hashibot hashibot bot added the size/S label Mar 27, 2018

result[i] = b
i = i + 1
for i := 0; i < length; i++ {
result[i] = charset[rand.Int(len(charset))]

This comment has been minimized.

@RyanSquared

RyanSquared Mar 27, 2018

It's technically possible to have a password now that is exactly one character, if somehow rand.Int() always returns 0. I am not sure if there is a crypto/rand alternative to rand.Perm(), which returns a random permutation.

This comment has been minimized.

@KellerFuchs

KellerFuchs Mar 27, 2018

Contributor

If by “exactly one character”, you mean the same, repeated characted, then yes it is, though it is vanishingly unlikely.

As crypto/rand uses OS-provided randomness (either a syscall like getentropy(3) or /dev/urandom), if it provides you with non-random numbers, you have a bigger problem.

This comment has been minimized.

@RyanSquared

RyanSquared Mar 27, 2018

There's still the case where it could avoid required characters, like all uppercase, or required symbols.

This comment has been minimized.

@paultyng

paultyng Mar 27, 2018

Contributor

Yeah, I called this out as well, that was what I suspect the reasoning for the division of the character sets previously.

This comment has been minimized.

@RyanSquared

RyanSquared Mar 27, 2018

I discussed it with KellerFuchs on IRC, and a few points were discussed:

  1. Password policies were discouraged by the NIST publication about passwords and 2fa
  2. The original code didn't attempt to conform to existing IAM policies, but rather just attempted it by just putting in what characters it thought would be required

@bflad bflad added the service/iam label Mar 27, 2018

@KellerFuchs

This comment has been minimized.

Copy link
Contributor

KellerFuchs commented Mar 27, 2018

PS: Crediting @lrvick for pointing out the code fragment.

@hashibot hashibot bot added the size/S label Mar 27, 2018

@KellerFuchs KellerFuchs force-pushed the hashbang:generatePassword branch from 60c64cb to 6181520 Mar 27, 2018

@hashibot hashibot bot added size/M and removed size/S labels Mar 27, 2018

@KellerFuchs

This comment has been minimized.

Copy link
Contributor

KellerFuchs commented Mar 27, 2018

Clearly, I don't know how to use crypto/rand before my second coffee; fixing this in a jiffy.

@KellerFuchs KellerFuchs force-pushed the hashbang:generatePassword branch from 6181520 to 7d2b63e Mar 27, 2018

@hashibot hashibot bot added the size/M label Mar 27, 2018

@KellerFuchs KellerFuchs force-pushed the hashbang:generatePassword branch from 7d2b63e to c77603c Mar 27, 2018

@hashibot hashibot bot added the size/M label Mar 27, 2018

@KellerFuchs KellerFuchs force-pushed the hashbang:generatePassword branch from c77603c to 59e0157 Mar 27, 2018

@hashibot hashibot bot added the size/M label Mar 27, 2018

@KellerFuchs

This comment has been minimized.

Copy link
Contributor

KellerFuchs commented Mar 27, 2018

@bflad Can you escalate this to whoever deals with security at @hashicorp ? It's a pretty bad one.

@jbardin
Copy link
Contributor

jbardin left a comment

Hi @KellerFuchs,

Thanks for submitting this. This looks good, and the comments here are strictly about better matching code style (and the regular aws committers can certainly override me on those points ;) )

for _, b := range components {
result[i] = b
i = i + 1
for i := 0; i < length; i++ {

This comment has been minimized.

@jbardin

jbardin Mar 27, 2018

Contributor

use range over result here

result[i] = b
i = i + 1
for i := 0; i < length; i++ {
r, err := rand.Int(rand.Reader, big.NewInt(int64(len(charset))))

This comment has been minimized.

@jbardin

jbardin Mar 27, 2018

Contributor

create the big.Int outside of the for loop.

if err != nil {
panic(err)
}
if r.IsInt64() {

This comment has been minimized.

@jbardin

jbardin Mar 27, 2018

Contributor

Invert this boolean and remove the else block

@hashibot hashibot bot added the size/M label Mar 27, 2018

@KellerFuchs

This comment has been minimized.

Copy link
Contributor

KellerFuchs commented Mar 27, 2018

@jbardin Done :)

@jbardin
Copy link
Contributor

jbardin left a comment

Sorry, I don't mean to nitpick here, especially if Go isn't your usual tool. Just a couple minor nits

i = i + 1
charset_size := big.NewInt(int64(len(charset)))

for i, _ := range result {

This comment has been minimized.

@jbardin

jbardin Mar 27, 2018

Contributor

One last thing -- the , _ can be left out entirely here

for _, b := range components {
result[i] = b
i = i + 1
charset_size := big.NewInt(int64(len(charset)))

This comment has been minimized.

@jbardin

jbardin Mar 27, 2018

Contributor

while we're at it, might as well use charsetSize here

@jbardin

This comment has been minimized.

Copy link
Contributor

jbardin commented Mar 27, 2018

Hi @KellerFuchs,

Thanks for taking the time to submit this!
I'd just like to point out for reference, our Security Policy and Community Guidelines, for others wishing to report security issues in the future, or to get in contact directly with our security team.

@paultyng

This comment has been minimized.

Copy link
Contributor

paultyng commented Mar 27, 2018

FWIW the character class code is probably there because someone could have an IAM password policy that enforces those checks, so this may start breaking for people, need to make sure that use case is considered.

https://docs.aws.amazon.com/IAM/latest/UserGuide/id_credentials_passwords_account-policy.html

@KellerFuchs

This comment has been minimized.

Copy link
Contributor

KellerFuchs commented Mar 27, 2018

@jbardin Thanks a lot; I looked for a proper security contact before dumping this in a PR, but couldn't find it. Perhaps add a link in README.md or .github/*.md ?

@paultyng

This comment has been minimized.

Copy link
Contributor

paultyng commented Mar 27, 2018

Something like this would satisfy the character class issues and uses crypto/rand: https://github.com/sethvargo/go-password

@jbardin

This comment has been minimized.

Copy link
Contributor

jbardin commented Mar 27, 2018

@paultyng, Yeah, I misread the previous code from the diff -- looking at the old code directly it seems that it was attempting to take each specific component, but it was not a fair distribution.

I think it's probably better to take a normally distributed password generated like this and verify it it against the policy after. (or we can vet the package linked above)

@paultyng

This comment has been minimized.

Copy link
Contributor

paultyng commented Mar 27, 2018

I think the main issue with that package is that it allows for too many special characters, IAM only allows for ! @ # $ % ^ & * ( ) _ + - = [ ] { } | ' per https://docs.aws.amazon.com/IAM/latest/UserGuide/id_credentials_passwords_account-policy.html#password-policy-details

vs in the package:

Symbols = "~!@#$%^&*()_+`-={}|[]\\:\"<>?,./"

So possibly would need to fork that I guess to HC?

Needs to satisfy the password policy issues mentioned above

}
charset := `abcdefghijklmnopqrstuvwxyz
ABCDEFGHIJKLMNOPQRSTUVWXYZ
012346789

This comment has been minimized.

@paultyng

paultyng Mar 27, 2018

Contributor

😢 this is missing 5

This comment has been minimized.

@paultyng

paultyng Mar 27, 2018

Contributor

I guess that was missed quite a while ago :(

generatePassword(): Add the missing 5
Thanks to @paultyng for the catch.

@hashibot hashibot bot added the size/M label Mar 27, 2018

5 added

@KellerFuchs

This comment has been minimized.

Copy link
Contributor

KellerFuchs commented Mar 27, 2018

@paultyng FWIW, the core of go-password does pretty much the same thing as this:
return a random element (as in, the element at a random index) from a “charset” buffer, and fill another buffer with the result, then return it as a string.

It does rejection sampling around that, to enforce “at least x {alphanum; symbols; ...}” constraints, but as far as I understand it's not needed here? If it really is, rejection sampling is the way to go, but that sort of policy can dangerousely reduce password entropy (and, BTW, NIST also advises against it).

@hashibot hashibot bot added the size/M label Mar 27, 2018

@jbardin

This comment has been minimized.

Copy link
Contributor

jbardin commented Mar 27, 2018

Thanks @KellerFuchs,

I'm also not strongly in favor of the go-password algorithm, since it only inserts a set number of "optional" characters, meaning you don't get much additional entropy as from a normal distribution. I'm OK with rejection sampling of the results in this case, since we have no choice but to conform to the set policy in some way.

Thanks!

@paultyng

This comment has been minimized.

Copy link
Contributor

paultyng commented Mar 27, 2018

In this case we are unfortunately at the mercy of how IAM is configured, but it is possible to query IAM to see what the policy is before forcing the rejections.

https://docs.aws.amazon.com/sdk-for-go/api/service/iam/#IAM.GetAccountPasswordPolicy

charset := `abcdefghijklmnopqrstuvwxyz
ABCDEFGHIJKLMNOPQRSTUVWXYZ
0123456789
!@#$%^&*()_+-=[]{}|'`

This comment has been minimized.

@mwhooker

mwhooker Mar 28, 2018

I think you may want to make this something like

	charsets := strings.Join([]string{
		"abcdefghijklmnopqrstuvwxyz",
		"ABCDEFGHIJKLMNOPQRSTUVWXYZ",
		"012346789",
		"!@#$%^&*()_+-=[]{}|'",
	}, "")

since this has 3 newlines in it

https://play.golang.org/p/UkuPLAvmbuS

This comment has been minimized.

@KellerFuchs

KellerFuchs Mar 29, 2018

Contributor

Oops, thanks a lot.
I did pretty much what you did to test, but got “luckier” (and had a larger non-\n charset) :/

@bflad bflad added this to the v1.14.0 milestone Mar 29, 2018

@KellerFuchs

This comment has been minimized.

Copy link
Contributor

KellerFuchs commented Mar 29, 2018

Sorry for the slow replies, my laptop died earlier in the week so it took a while to be back online.
FYI, this issue was assigned CVE-2018-9057.

@KellerFuchs

This comment has been minimized.

Copy link
Contributor

KellerFuchs commented Mar 29, 2018

@paultyng As far as I understand, there is no default password policy, and users setting one can break the current code too?
If that's correct, shouldn't that go in a separate issue?

@jbardin

This comment has been minimized.

Copy link
Contributor

jbardin commented Mar 29, 2018

@KellerFuchs,

No worries, thanks for the update.

Yes, the only default in the policy is the minimum length of 6, and this particular PR doesn't need to deal with the policy directly. The old code would probably pass any reasonable policy, but that was at the expense of not generating good passwords. I'll follow up with a separate PR regarding the policy checks.

@jbardin

This comment has been minimized.

Copy link
Contributor

jbardin commented Mar 29, 2018

Hi @KellerFuchs,

Thanks again for your work on this.
Since the charset needed to be updated, I moved your commit into a new branch and made the modifications there, along with the policy checking. The new PR is #3989.

Closing this since it's been superseded.

@jbardin jbardin closed this Mar 29, 2018

@KellerFuchs

This comment has been minimized.

Copy link
Contributor

KellerFuchs commented Mar 29, 2018

@jbardin Am I misunderstanding something, or is #3989 meant to does with the case where there is a password policy set? If so, please reopen, so we can get the security-relevant bits actually merged and deployed, and keep the work on the other issue (not checking and conforming to password policies) in #3989

@jbardin

This comment has been minimized.

Copy link
Contributor

jbardin commented Mar 29, 2018

@KellerFuchs, we are intending to merge the security related commit and the policy checks together to prevent breaking existing users. The policy work isn't going to delay the release.

@KellerFuchs

This comment has been minimized.

Copy link
Contributor

KellerFuchs commented Mar 29, 2018

Mkay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment