ChatGPTNextWeb / ChatGPT-Next-Web

A cross-platform ChatGPT/Gemini UI (Web / PWA / Linux / Win / MacOS). 一键拥有你自己的跨平台 ChatGPT/Gemini 应用。
https://app.nextchat.dev/
MIT License
76.44k stars 59.11k forks source link

[Bug] plain text password should not be logged to console #3464

Closed Gentoli closed 10 months ago

Gentoli commented 11 months ago

https://github.com/Yidadaa/ChatGPT-Next-Web/blob/cf50299b142a40d1b043f0b3ceb3cc6c4c5b249d/app/api/auth.ts#L37

H0llyW00dzZ commented 11 months ago

https://github.com/Yidadaa/ChatGPT-Next-Web/blob/cf50299b142a40d1b043f0b3ceb3cc6c4c5b249d/app/api/auth.ts#L37

that's in server-side

Gentoli commented 11 months ago

@H0llyW00dzZ In general sensitive data shouldn't be send to console/stdout. They can get captured by logging aggregators such as CloudWatch which have less access protection.

Here an example where twitter logged some passwords by accident and recommends users to change their password. https://blog.twitter.com/official/en_us/topics/company/2018/keeping-your-account-secure.html

H0llyW00dzZ commented 11 months ago

@H0llyW00dzZ In general sensitive data shouldn't be send to console/stdout. They can get captured by logging aggregators such as CloudWatch which have less access protection.

Here an example where twitter logged some passwords by accident and recommends users to change their password. https://blog.twitter.com/official/en_us/topics/company/2018/keeping-your-account-secure.html

how about refactoring like this ?

Gentoli commented 11 months ago

@H0llyW00dzZ Thanks for looking into this.

https://github.com/Yidadaa/ChatGPT-Next-Web/blob/cf50299b142a40d1b043f0b3ceb3cc6c4c5b249d/app/api/auth.ts#L42 The server does a look up for hashedCode in serverConfig.codes. The existing code logs both so they can be compared(?). So I think hashedCode in serverConfig.codes needs to be logged in the same way.

Looking at the code further, it is hashing password with md5. md5 is considered insecure given one can brute force a hash fairly quickly so logging it is not more secure than logging the password. If possible, you should switch to bcrypt or something similar.

You should probably remove all three [Auth] lines just to be safe. Then find if the access code or hashes are logged in other places. If you do need these logged sometimes, maybe use a logger with different log levels so by default these are not logged.

H0llyW00dzZ commented 11 months ago

@H0llyW00dzZ Thanks for looking into this.

https://github.com/Yidadaa/ChatGPT-Next-Web/blob/cf50299b142a40d1b043f0b3ceb3cc6c4c5b249d/app/api/auth.ts#L42

The server does a look up for hashedCode in serverConfig.codes. The existing code logs both so they can be compared(?). So I think hashedCode in serverConfig.codes needs to be logged in the same way. Looking at the code further, it is hashing password with md5. md5 is considered insecure given one can brute force a hash fairly quickly so logging it is not more secure than logging the password.

You should probably remove all three [Auth] lines just to be safe. Then find if the access code or hashes are logged in other places. If you do need these logged sometimes, maybe use a logger with different log levels so by default these are not logged.

tbh removed or not it clearly safe, also additional to made it more safe just disabled a fast link.

[!NOTE]
the only thing you need to compared is in this repo mostly only focusing UI and the most important only proxying that's it, other wise are clearly safe, because user can use own their api key on custom end point path lmao. be professional comparing program how it work

H0llyW00dzZ commented 11 months ago

[!NOTE]
By disabling a fast link it made more safe to a client and server from intercept https/http (e,g. from network router logger between client and server), instead of care about Server Log Lmao

Gentoli commented 11 months ago

tbh removed or not it [is] clearly safe

If you want to ignore all the security best practices and offer an insecure backend because "this repo mostly only focusing UI", feel free to close this issue.

Just a reminder there is plenty of articles on keeping sensitive info out of logs and how to secure a password server side:

H0llyW00dzZ commented 11 months ago

tbh removed or not it [is] clearly safe

If you want to ignore all the security best practices and offer an insecure backend because "this repo mostly only focusing UI", feel free to close this issue.

Just a reminder there is plenty of articles on keeping sensitive info out of logs and how to secure a password server side:

no need, I know a lots experience of hacking, not only that basic guide

H0llyW00dzZ commented 11 months ago

@Gentoli let me explain in that doc's basic guide example this one "Do not use deprecated hashing technologies such as MD5, SHA1 and under no circumstances should you use reversible encryption or try to invent your own hashing algorithm."

image

if you comparing that to this repo because using MD5, its pointless actually are useless if switching to other hash, no matter hash are strong or not, because this repo in server side only using for proxying, and about access code still can handle easily by other people who using this repo for their project, example deploying on vercel

H0llyW00dzZ commented 11 months ago

so the only things to made it more secure & safe in this repo, only that fast link if disable fast link are true, then it's more secure & safe because how it work (about fastlink), if disable fast link are false, user can input json data in url parameter

Gentoli commented 11 months ago

Sorry, I'm not sure what's your point here.. The issue here is one can derive the access code from logs. It is not about there might be a more secure option.

For example a user is sharing their hosting info and logs for troubleshooting, They redacts the plaintext password, but leaves the hashed access code in-place. Now an attacker can brute force the MD5 hash to a password and visit their deployment using the hosting info therefore compromising the user's OpenAI account. There are further implication for the user, if they used the same password for the access code on other websites, exposing the MD5 hash can lead to the user's other account being compromised too.

H0llyW00dzZ commented 11 months ago

Sorry, I'm not sure what's your point here.. The issue here is one can derive the access code from logs. It is not about there might be a more secure option.

For example a user is sharing their hosting info and logs for troubleshooting, They redacts the plaintext password, but leaves the hashed access code in-place. Now an attacker can brute force the MD5 hash to a password and visit their deployment using the hosting info therefore compromising the user's OpenAI account.

lmao its called human error then for them,not because of this repo

H0llyW00dzZ commented 11 months ago

anyways not sure if its agreed or not for disabling/remove auth in console log, because this issue are idk how I call, cuz might other people still want keep auth console log for monitoring,etc (e.g, as security professional)

Yidadaa commented 11 months ago

accessCode is just a simple layer of protection. We don't expect it to strictly prevent various attacks. I agree that the user's original input should be removed, but it is reasonable to print the hashed string, which helps locate the problem.

H0llyW00dzZ commented 11 months ago

accessCode is just a simple layer of protection. We don't expect it to strictly prevent various attacks. I agree that the user's original input should be removed, but it is reasonable to print the hashed string, which helps locate the problem.

yeah I agreed also with this because, about this issue are idk how I call its not because of this repo its called human error when I reading this "For example a user is sharing their hosting info and logs for troubleshooting, They redacts the plaintext password, but leaves the hashed access code in-place. Now an attacker can brute force the MD5 hash to a password and visit their deployment using the hosting info therefore compromising the user's OpenAI account. There are further implication for the user, if they used the same password for the access code on other websites, exposing the MD5 hash can lead to the user's other account being compromised too."

Gentoli commented 11 months ago

Security is mostly to prevent the weakest link in the chain, if that link is the user, then we should do our best to protect the user.

I do agree the having the hashed access code in the logs would be helpful for troubleshooting, but given it's currently hashed using MD5, it's possible to get the plaintext from it.

I think the best would be:

H0llyW00dzZ commented 11 months ago

Security is mostly to prevent the weakest link in the chain, if that link is the user, then we should do our best to protect the user.

I do agree the having the hashed access code in the logs would be helpful for troubleshooting, but given it's currently hashed using MD5, it's possible to get the plaintext from it.

I think the best would be:

  • use a leveled logger to log these as debug so the user needs to enable debug logging to see these in the console
  • refactor MD5 hash to something like bcrypt

still pointless, even refactoring MD5 hash to something like bcrypt, because in server-side only using for proxying and data already secured stored it in client/user local browser

H0llyW00dzZ commented 11 months ago

remember this called Human Error not because of repo "For example a user is sharing their hosting info and logs for troubleshooting, They redacts the plaintext password, but leaves the hashed access code in-place. Now an attacker can brute force the MD5 hash to a password and visit their deployment using the hosting info therefore compromising the user's OpenAI account. There are further implication for the user, if they used the same password for the access code on other websites, exposing the MD5 hash can lead to the user's other account being compromised too."