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

Possibly a bug? #649

Open
yamerooo123 opened this issue Oct 4, 2024 · 8 comments
Open

Possibly a bug? #649

yamerooo123 opened this issue Oct 4, 2024 · 8 comments
Assignees
Labels
bug Hmm, this isn't working correctly

Comments

@yamerooo123
Copy link

yamerooo123 commented Oct 4, 2024

Hello,

I'm a security research. I perform a security testing on open-source projects for free.

Bug

  1. Users can create a username with a space in it. For example "tester" and " tester". However, these two users are separate accounts. There is no security issue here.

Vulnerabilities

  1. Lack of expiry in token: It seems like JWT token has no expiration and considers a vulnerability. Since Change password function validates users from token. Someone can reuse the vulnerable token to change victim password to maintain persistence access.
    2. Lack of CSRF in Change password function: Without CSRF protection, adversaries could exploit the lack of verification for state-changing requests (like changing a password). This could allow an attacker to trick a logged-in user into making a request to change their password by embedding a request in a malicious site or email.

Mitigation:

  1. I would suggest you to make the token to have an expiration
    2. If possible, implement CSRF protection in functions that required authenticated users to interact.

If you need more information please let me know.

Have a nice day!

@IRHM
Copy link
Member

IRHM commented Oct 4, 2024

Hey @yamerooo123 thanks for taking the time to dig in and find problems!

  1. That's a good point, I should probably make sure new accounts cant match existing ones (after trimmed spaces and not case sensitive).
  2. Lack of token exp: If I remember correctly, we don't issue a new token after change password, the user continues using their existing token. I see what you mean though, users might expect to be able to (optionally) expire all other tokens in use for their account. Quick research tells me token versioning is probably the way i'd want to fix/support this (so i dont forget: include a "version" in all tokens, if user wants to expire all tokens, increment this version (probs add version to db for comparisons), then auth with tokens that dont have the new version will fail.
  3. Lack of CSRF protection: We don't use any cookies and from my understanding, if you dont use cookies, you don't have to consider CSRF (since the browser cant automatically attribute any (auth) cookies with a request if you dont use them)? I'm not too smart about this stuff, I think the app could still be vulnerable to XSS attacks (if im right about the csrf thing). I will have to do more research on this (I would appreciate your thoughts on this!).

@IRHM IRHM added the bug Hmm, this isn't working correctly label Oct 4, 2024
@IRHM IRHM moved this to Todo in Watcharr Oct 4, 2024
@IRHM IRHM self-assigned this Oct 4, 2024
@yamerooo123
Copy link
Author

Ah, my bad, I mistook Session Storage for Cookies when inspecting. Yes, you're right, the web app doesn't need a CSRF token since it is session-based and not cookie-based. I will continue to find more bugs and vulnerabilities and report to you via this issue as soon as i found one.

@IRHM
Copy link
Member

IRHM commented Oct 6, 2024

@yamerooo123 Thanks for making the video - would you be able to upload debug logs for the server of those requests going through?

@IRHM IRHM moved this from Todo to In Progress in Watcharr Oct 6, 2024
@IRHM
Copy link
Member

IRHM commented Oct 6, 2024

@yamerooo123 Do you have matrix (if so, are you able to message me at @irhm:matrix.org to discuss this further)

@sbondCo sbondCo deleted a comment from yamerooo123 Oct 6, 2024
@sbondCo sbondCo deleted a comment from yamerooo123 Oct 6, 2024
@IRHM
Copy link
Member

IRHM commented Oct 6, 2024

@yamerooo123 I have hopefully resolved the issue you discovered in the next release coming out shortly.

I hope you don't mind, I deleted your comments as to not let any malicious people easy find out how to exploit the issues so easily.

If you are free to test this after the release I would be very greatful.

Thank you very much for your research into this! I should probably make a more secure way for discussing security issues in the future.

@yamerooo123
Copy link
Author

Yes. I would be glad to help you test with the new version.

You can use GitHub security tab to make a secure communication for security researchers to report vulnerabilities to prevent public disclosure as well.

In addition, your web app is bulletproof to SQL injection and actually is well implemented. Since your web app is heavily rely on API resources and sessions, I would make those the first priority when it comes to security issues.

Also, I have a small favor, I would like to request a CVE for this finding( only the recent one). CVE will help users to understand and advise them to upgrade to the latest version to patch the vulnerabilities. It is also quite common for many open-source softwares or applications to have CVE assigned too. Everything will be kept secret until the vulnerability has been patched.

Preliminarily, if I find more vulnerabilities, I will contact you via @IRHM:matrix.org to prevent public disclosure.

Have a day!

@IRHM
Copy link
Member

IRHM commented Oct 7, 2024

@yamerooo123 If it lets you make a CVE, please feel free, I hadn't thought of it, but if it helps alert people to upgrade then that sounds good (not sure if I have to make it?).

I had a look through the releases, it looks like the vulnerability has been present since v1.19.0 and should now be fixed in the latest release v1.44.0.

Thank you!

@yamerooo123
Copy link
Author

yamerooo123 commented Oct 7, 2024

FINAL UPDATE

I THINK I FOUND THE CULPRIT! IT IS /DATA!

I undestand this folder is designed to be accessed only to high privileged users (sudo privilege) as it containing sensitive data which is why when JWT try to read JWT_SECRET, it couldn't retrieve because it lacks the privilege! Your code is solid but it has to do with Design Flaw . I retested it with jwt.io using a blank password. Now i couldn't craft JWT token anymore. This security flaw had me whole afternoon to figure it out. 😆

You do not have to do it, as a security reseacher i will take care of it for you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Hmm, this isn't working correctly
Projects
Status: In Progress
Development

No branches or pull requests

2 participants