taa176 / web

Website. Welcome.
0 stars 0 forks source link

Browserify Refactoring #4

Open taa176 opened 4 years ago

taa176 commented 4 years ago

I just posted a proposed change to how we use browserify.

My main gripes on using browserify's command line utility are that: a) after every change to a client-side file, you have to remember to use browserify to update the 'compiled' code b) client-side/public code that you develop with is placed in the server folder c) using even a single function from an external library introduces way more complexity than seems reasonable to me, and this complexity is spread out throughout the entire codebase.

To get around these issues, I saw that browserify has a module which can 'browserify' files on the fly. I inserted three lines of code in the server.js file which will automatically 'browserify' any requested .js files and send them to the user. I also put a symbolic link to node_modules in the public/ directory.

Pros: You no longer have to think about anything when you develop client-side code. Just use modules as you wish. All browserify logic is concentrated in one place

Cons: There is some overhead associated with browserifying after every request. It's not an issue now, but if it becomes one later on we can just cache the sent files. browserify is still dog shit because I said so but not for any other reason really

let me know if you can think of any potential problems with this

taa176 commented 4 years ago

I do find it suspicious that every guide/most of the documentation for browserify uses the command line utility w/o really mentioning the javascript api.

I guess if you try this with huge libraries it's too impractical?

k-hendricks commented 4 years ago

I found a far superior option to using browserify. There is a package in the npm libraries called '@tarp/require' which, without README files, tests, etc, is a single 4K js file. However, the issue is that it only includes the require('') function. So if you have any npm code that you want to run clientside, as long as it is CommonJS plus a requires function, you're fine. However, the blakejs library relies on Buffer, which is apparently only defined in node. As I understand, Buffer objects are for directly interacting with binary efficiently. bops is another small npm library that theoretically allows for clientside use of that class, but I haven't gotten it to work yet. Another npm library that supposedly allows for Buffer objects is Buffer-browserify, which is about a 1000 lines of javascript, a much, much smaller part of the overall browserify universe which can be used separately. (to be clear, it would not require recompiling, just a single require call, which would then be handled by @tarp/request)

This entire thing is making me just want to hash serverside, which as far as I understand has two downsides 1) the server can see people's plaintext passwords 2) it is far easier to ddos our server by spamming requests to login/make new user which will call a hashing function server side. Even if the attack fails, it will be more flops on server side, something that many server hosting services make you pay for or stay under a limit.

Still though, we have to implement and rely on https. No clever hashing will prevent active mitm attacks. (That is to say, a mitm who can both view and modify instead of just view). If we ever want to accept payment, I think we will also need to purchase a TLS/SSL certificate from a reputable third party.

As for the salt of the hash, I propose we simply use the username + email + name + constant random string (known to the clientside only). The only point of this is to prevent a rainbow table attack, and I think that concatenation is sufficiently secure to stop this.

As a point of interest, hashing with a constant salt is called pepper. It is slightly preferable to hashing without, but it is not much of an improvement.

k-hendricks commented 4 years ago

I am also looking into using a different hashing function, and I am open to hashing serverside with blakejs, but I don't think that solves the root issue here which is that I would like to be able to use npm libraries clientside in an easy way.

k-hendricks commented 4 years ago

Browserify has been completely eliminated. The solution was a combination of @tarp/require and a new npm package buffer. A folder called node_modules has been added to the public directory. There is a text file in it called jsFilesToServe which has been added to the server.js requests list. This is kind of hacky, so it is something to come back to and optimize if the requests list in server.js gets too unwieldy.

taa176 commented 4 years ago

It seems like we'll need to get an SSL certificate from a third party anyway, otherwise most browsers will post a very scary message about how our site isn't secure whenever someone visits. Seems like they are $10 per month.

As for the salt, it seems to me that simply generating a new random number/string for each user is conventional. It won't take much more effort and it'll be more secure. Also, when you said above that it's known to the client side only, did you mean server side?

k-hendricks commented 4 years ago

The SSL certs are just for testing on localhost. I know we can't self sign them for production. That's why I saw no problem with uploading them to a public github repository. For production we can use something called certbot which is free, but if you try to use it with the domain name localhost, it gives an error, so I just self-signed some by hand.

I meant clientside. The idea was to do all the hashing clientside to reduce the ability of an attacker to make our server do lots of FLOPs. I don't actually think doing it with a random string serverside is more secure. It seems just as secure to use username + a constant random string, since that will still be unique for every password, and, because of the random string, will be different from the hashes a different website, even if the user has the same username + password combo, and they use the same hashing function. I know its conventional to just generate a new random string, but I've seen no good reason why we can't force that fairly cpu intensive task onto the user the way I described.

k-hendricks commented 4 years ago

Also I'm concerned about the cookie generation. Right now, I'm creating the cookie from the username and the time that the server tries to generate the cookie. This seems like an opportunity for a cookie hijack scenario, but I'm not sure how to get around that. I haven't found a random number generator for javascript that literally grabs entropy from some physical qualities of the computer its running on, and I'm not sure I'd even want to use a library like that anyway, because such things are normally very slow.

taa176 commented 4 years ago

Seems like we still need to hash serverside. If an attacker gained access to our password/username database, we want to ensure they cannot actually use those passwords without doing some work.

Only hashing clientside means that any http request made to our server that contains the password we store will be accepted by our system. We want to force the attacker to come up with a password that hashes to what we store.

k-hendricks commented 4 years ago

The main changes to https are very minor. There is an https library, so we just need to switch the require to that. It takes before the (request, response) function the certs information. All responses now need to have their header modified by response.writeHead(200); I am following this article, and I will be implementing the helmet package. https://www.sitepoint.com/how-to-use-ssltls-with-node-js/

k-hendricks commented 4 years ago

I think that attack vector would be better handled by serverside encryption of the database, but hashing serverside would work as well. Serverside encryption of the database would also protect the rest of the information in it, and symmetric ciphers are much faster than hash functions.

taa176 commented 4 years ago

Also, are you saying each client stores their own salt, and this salt is known to the clientside only?

How can users change devices/clear the data on their browser and still access their account?

taa176 commented 4 years ago

Hmm, okay, so somehow we'd have to make sure the attack cannot possibly access the encryption key. Like, it must somehow be even more secure than the database

k-hendricks commented 4 years ago

The salt (or in this case the pepper) is just in the code that everyone gets. (In login.js and in the newUser function) Hash_function( (password, salt) => { salt = username+pepper; return [password, salt]; }); Yes keeping the encryption key secure would be an issue. But proper encryption serverside would solve the problem. If there is no decent way to keep the key more secure than the server, then I will come back and do the hashing serverside as well.

On Sat, Sep 14, 2019, 16:12 taa176 notifications@github.com wrote:

Also, are you saying each client stores their own salt, and this salt is known to the clientside only?

How can users change devices/clear the data on their browser and still access their account?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/taa176/web/issues/4?email_source=notifications&email_token=AEKJW6PND7LOM3OPZREMHF3QJVVVZA5CNFSM4IWJIJYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6XFZIQ#issuecomment-531520674, or mute the thread https://github.com/notifications/unsubscribe-auth/AEKJW6KV76FDZVWBSCZC5H3QJVVVZANCNFSM4IWJIJYA .

k-hendricks commented 4 years ago

I'm sorry that last bit isn't clear. Keeping the encryption key more secure than the server is a hurdle. If that can be done, symmetric encryption serverside will be a great feature that will also handle the attack on the clientside salted hashes you are describing.

taa176 commented 4 years ago

I've been using this website as a resource: https://crackstation.net/hashing-security.htm

We shouldnt deviate from usual/conventional practices, for example they make the case for using a different salt of each user, using a slow hash function, etc.

taa176 commented 4 years ago

If you do want to go the encryption path, we should make sure a) that we have a method to keep the key secure and b) that method is accepted/standard in some sense

k-hendricks commented 4 years ago

I agree with you.

On Sat, Sep 14, 2019, 16:47 taa176 notifications@github.com wrote:

If you do want to go the encryption path, we should make sure a) that we have a method to keep the key secure and b) that method is accepted/standard in some sense

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/taa176/web/issues/4?email_source=notifications&email_token=AEKJW6M72TXCWYE5SKALI3TQJVZZRA5CNFSM4IWJIJYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6XGEYI#issuecomment-531522145, or mute the thread https://github.com/notifications/unsubscribe-auth/AEKJW6L3D2WLS65G545Z3PLQJVZZRANCNFSM4IWJIJYA .