Skip to content
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

Insecurities in the Authentication System #45

Closed
tnix100 opened this issue May 9, 2022 · 121 comments · Fixed by #48
Closed

Insecurities in the Authentication System #45

tnix100 opened this issue May 9, 2022 · 121 comments · Fixed by #48
Assignees
Labels

Comments

@tnix100
Copy link

tnix100 commented May 9, 2022

The hashing algorithm seems to be using SHA512 with a salt generated using a list of characters and using Math.random. SHA512 isn't really considered secure and Math.random isn't a true random, it's only pseudo-random. Please use something more secure like scrypt.

@VelocityDesign
Copy link
Member

Ah, I see. This is my first time designing a system with Passwords, so I'll do a bit more research. From what I understand, this is only for the password algorithm and not the session token algorithm, right?

@VelocityDesign VelocityDesign pinned this issue May 9, 2022
@iamperry294
Copy link

@VelocityDesign I can put up a pull request for this if you want, but using NodeJS's built-in scrypt or bcrypt is much more secure than the current design.

They are both secure enough for password management.

@VelocityDesign
Copy link
Member

@VelocityDesign I can put up a pull request for this if you want, but using NodeJS's built-in scrypt or bcrypt is much more secure than the current design.

They are both secure enough for password management.

That would be awesome! If you do make a PR, I'd prefer the bcrypt system. Once it's done, I'll review it for you.

@iamperry294
Copy link

@VelocityDesign There seems to be a lot of code regarding the hashing system. What do I have to change for the signup/login hashing?

@VelocityDesign
Copy link
Member

@VelocityDesign There seems to be a lot of code regarding the hashing system. What do I have to change for the signup/login hashing?

In auth.ts, the SolarisAuthenticator.hash() function manages all hashing. The setup() function determines the salt for the AES Encryption.

@tnix100
Copy link
Author

tnix100 commented May 9, 2022

I'd prefer the bcrypt system.

I would personally recommend scrypt, it's more secure and uses less resources to hash passwords. But bcrypt works ok too.

@iamperry294
Copy link

iamperry294 commented May 9, 2022

I'd prefer the bcrypt system.

I would personally recommend scrypt, it's more secure and uses less resources to hash passwords. But bcrypt works ok too.

Plus, you're not using an extra library, which is generally considered better. Bcrypt is secure enough though, so it doesn't matter as much as some other things here.

@VelocityDesign
Copy link
Member

I'd prefer the bcrypt system.

I would personally recommend scrypt, it's more secure and uses less resources to hash passwords. But bcrypt works ok too.

Plus, you're not using an extra library, which is generally considered better. Bcrypt is secure enough though, so it doesn't matter as much as some other things here.

What do you mean? Does the pre-existing one have scrypt?

@tnix100
Copy link
Author

tnix100 commented May 9, 2022

I'd prefer the bcrypt system.

I would personally recommend scrypt, it's more secure and uses less resources to hash passwords. But bcrypt works ok too.

Plus, you're not using an extra library, which is generally considered better. Bcrypt is secure enough though, so it doesn't matter as much as some other things here.

What do you mean? Does the pre-existing one have scrypt?

Yeah the pre-existing one has scrypt, we use the built in one for New Meower and we use scrypt IIRC.

@VelocityDesign
Copy link
Member

@iamperry294 The session security is an object that is signed by the auth system with AES. When decrypted it returns:

{
 username: string,
 date: number,
 salt: string
}

@VelocityDesign
Copy link
Member

I'd prefer the bcrypt system.

I would personally recommend scrypt, it's more secure and uses less resources to hash passwords. But bcrypt works ok too.

Plus, you're not using an extra library, which is generally considered better. Bcrypt is secure enough though, so it doesn't matter as much as some other things here.

What do you mean? Does the pre-existing one have scrypt?

Yeah the pre-existing one has scrypt, we use the built in one for New Meower and we use scrypt IIRC.

Oh okay. As you can probably tell, I'm home now, so I'll look into implementing scrypt.

@tnix100
Copy link
Author

tnix100 commented May 9, 2022

@iamperry294 The session security is an object that is signed by the auth system with AES. When decrypted it returns:

{
 username: string,
 date: number,
 salt: string
}

Do tokens really have to be encrypted or hashed? I feel like that will cause a lot of overhead for the server having to run those algorithms everytime someone makes a request with a token.

@VelocityDesign
Copy link
Member

How could this be prevented?

@tnix100
Copy link
Author

tnix100 commented May 9, 2022

How could this be prevented?

Just don't encrypt or hash them, it's not like it's a permanent password you're trying to keep secret, it's something specific to your site that is usually temporary.

@VelocityDesign
Copy link
Member

I'd only consider not encrypting it with AES and instead doing a session object such as:

{
 username: string,
 date: number = date.utc(),
 token: string =  hash(username+date)
}

Of course, that wouldn't be as secure in my opinion.

@VelocityDesign
Copy link
Member

So if I'm correct, the crypto.scryptSync function accepts:
password, the password to be hashed,
salt, a randomly generated salt that is stored in a database/in config, and
keylen, the length of the key?

Just want to make sure before I implement it this way.

@tnix100
Copy link
Author

tnix100 commented May 10, 2022

So if I'm correct, the crypto.scryptSync function accepts: password, the password to be hashed, salt, a randomly generated salt that is stored in a database/in config, and keylen, the length of the key?

Just want to make sure before I implement it this way.

Make sure not to generate salts yourself, or at least use a truly random generator, not JavaScript math or anything like that.

@VelocityDesign
Copy link
Member

So if I'm correct, the crypto.scryptSync function accepts: password, the password to be hashed, salt, a randomly generated salt that is stored in a database/in config, and keylen, the length of the key?
Just want to make sure before I implement it this way.

Make sure not to generate salts yourself, or at least use a truly random generator, not JavaScript math or anything like that.

the crypto.randomBytes works, right?

@tnix100
Copy link
Author

tnix100 commented May 10, 2022

So if I'm correct, the crypto.scryptSync function accepts: password, the password to be hashed, salt, a randomly generated salt that is stored in a database/in config, and keylen, the length of the key?
Just want to make sure before I implement it this way.

Make sure not to generate salts yourself, or at least use a truly random generator, not JavaScript math or anything like that.

the crypto.randomBytes works, right?

"Generates cryptographically strong pseudo-random data. The size argument is a number indicating the number of bytes to generate. This means that the random data is secure enough to use for encryption purposes." -Stack overflow

@VelocityDesign
Copy link
Member

VelocityDesign commented May 10, 2022

Awesome! I should have it pushed tonight/today.
I'll also be overhauling a few other parts of the auth system.

@iamperry294
Copy link

@VelocityDesign This whole auth system isn't amazing. I can give a flow of what I personally think would be a better fit.

@VelocityDesign
Copy link
Member

Okay, please do!

@iamperry294
Copy link

Okay, please do!

I have a question. You seem to be encrypting user data into the token, so would you rather have it be that way (no token revocation but less db requests) or would you want to make a request to the database for every request that needs authentication?

@VelocityDesign
Copy link
Member

Okay, please do!

I have a question. You seem to be encrypting user data into the token, so would you rather have it be that way (no token revocation but less db requests) or would you want to make a request to the database for every request that needs authentication?

Less DB requests. I only have a finite amount of bandwidth and storage on the server. I could do token refreshes, however.

@iamperry294
Copy link

Okay, please do!

I have a question. You seem to be encrypting user data into the token, so would you rather have it be that way (no token revocation but less db requests) or would you want to make a request to the database for every request that needs authentication?

Less DB requests. I only have a finite amount of bandwidth and storage on the server. I could do token refreshes, however.

Aren't you using MongoDB though?

@VelocityDesign
Copy link
Member

Yes.

@VelocityDesign
Copy link
Member

Okay, now that I have session security working, how do I implement password hashing with Scrypt (or do I use JWT for that?)

@iamperry294
Copy link

Okay, now that I have session security working, how do I implement password hashing with Scrypt (or do I use JWT for that?)

Bcrypt is fine. You got access token/refresh token with theft detection working?

@VelocityDesign
Copy link
Member

Not yet. Also, how do I verify passwords if I'm using a random salt each time I generate a hash?

@iamperry294
Copy link

iamperry294 commented May 10, 2022

Not yet. Also, how do I verify passwords if I'm using a random salt each time I generate a hash?

Do you want me to put up a bcrypt/scrypt implementation for you?

@VelocityDesign
Copy link
Member

Sure

@iamperry294
Copy link

Scrypt or bcrypt?

@VelocityDesign
Copy link
Member

Whichever you think is easier for you

@VelocityDesign
Copy link
Member

What email should I attribute to these commits for you?

@iamperry294
Copy link

Whichever you think is easier for you

If you're fine with bcrypt, here: https://github.com/iamperry294/auth-server/blob/main/index.js Look at /api/authenticate and /api/register for implementations.

@iamperry294
Copy link

What email should I attribute to these commits for you?

[email protected]

@VelocityDesign
Copy link
Member

VelocityDesign commented May 10, 2022

Whichever you think is easier for you

If you're fine with bcrypt, here: https://github.com/iamperry294/auth-server/blob/main/index.js Look at /api/authenticate and /api/register for implementations.

That doesn't appear to have salting, so that's insecure, right?

@iamperry294
Copy link

Whichever you think is easier for you

If you're fine with bcrypt, here: https://github.com/iamperry294/auth-server/blob/main/index.js Look at /api/authenticate and /api/register for implementations.

That doesn't appear to have salting, so that's insecure, right?

It has no pepper, but it does have 10 rounds of salt. , 10 Change it to a higher number if you have a lot of resources.

@VelocityDesign
Copy link
Member

Whichever you think is easier for you

If you're fine with bcrypt, here: https://github.com/iamperry294/auth-server/blob/main/index.js Look at /api/authenticate and /api/register for implementations.

That doesn't appear to have salting, so that's insecure, right?

It has no pepper, but it does have 10 rounds of salt. , 10 Change it to a higher number if you have a lot of resources.

Oh, okay. So how does it verify the password without a stored salt?

@iamperry294
Copy link

Whichever you think is easier for you

If you're fine with bcrypt, here: https://github.com/iamperry294/auth-server/blob/main/index.js Look at /api/authenticate and /api/register for implementations.

That doesn't appear to have salting, so that's insecure, right?

It has no pepper, but it does have 10 rounds of salt. , 10 Change it to a higher number if you have a lot of resources.

Oh, okay. So how does it verify the password without a stored salt?

Maybe you want a more in-depth look on how password hashing works? It is stored in the password...

@VelocityDesign
Copy link
Member

Whichever you think is easier for you

If you're fine with bcrypt, here: https://github.com/iamperry294/auth-server/blob/main/index.js Look at /api/authenticate and /api/register for implementations.

That doesn't appear to have salting, so that's insecure, right?

It has no pepper, but it does have 10 rounds of salt. , 10 Change it to a higher number if you have a lot of resources.

Oh, okay. So how does it verify the password without a stored salt?

Maybe you want a more in-depth look on how password hashing works? It is stored in the password...

OH, okay.

@VelocityDesign
Copy link
Member

In the implementation you made for solaris, you had it generate a salt.

    let salt = bcrypt.genSaltSync(10);
    return bcrypt.hashSync(password, salt);

Does that work the same?

@iamperry294
Copy link

In the implementation you made for solaris, you had it generate a salt.

    let salt = bcrypt.genSaltSync(10);
    return bcrypt.hashSync(password, salt);

Does that work the same?

That only works for signup, I don't really know if there's a difference with TS or anything.

@VelocityDesign
Copy link
Member

Okay.

@tnix100
Copy link
Author

tnix100 commented May 10, 2022

Whichever you think is easier for you

If you're fine with bcrypt, here: https://github.com/iamperry294/auth-server/blob/main/index.js Look at /api/authenticate and /api/register for implementations.

That doesn't appear to have salting, so that's insecure, right?

It has no pepper, but it does have 10 rounds of salt. , 10 Change it to a higher number if you have a lot of resources.

Should be using 12 at minium.

@iamperry294
Copy link

Whichever you think is easier for you

If you're fine with bcrypt, here: https://github.com/iamperry294/auth-server/blob/main/index.js Look at /api/authenticate and /api/register for implementations.

That doesn't appear to have salting, so that's insecure, right?

It has no pepper, but it does have 10 rounds of salt. , 10 Change it to a higher number if you have a lot of resources.

Should be using 12 at minium.

I doubt their servers can handle it, but yes.

@tnix100
Copy link
Author

tnix100 commented May 10, 2022

What kind of hardware is it running? Meower does 12 and it used to run on a Raspberry Pi 4.

@iamperry294
Copy link

What kind of hardware is it running? Meower does 12 and it used to run on a Raspberry Pi 4.

Meower?

@tnix100
Copy link
Author

tnix100 commented May 10, 2022

What kind of hardware is it running? Meower does 12 and it used to run on a Raspberry Pi 4.

Meower?

https://github.com/meower-media-co

Check the security.py file in the server repo for how it hashes passwords

@VelocityDesign
Copy link
Member

I have the code almost ready to push, but I stopped to watch Google I/O today :)

@iamperry294
Copy link

I have the code almost ready to push, but I stopped to watch Google I/O today :)

Google I/O?

@VelocityDesign
Copy link
Member

VelocityDesign commented May 11, 2022

I have the code almost ready to push, but I stopped to watch Google I/O today :)

Google I/O?

Developer Conference see here

@iamperry294
Copy link

Okay.

@VelocityDesign
Copy link
Member

@iamperry294, @tnix100:
Please review PR #48 (see above).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants