jeremyckahn / chitchatter

Secure peer-to-peer chat that is serverless, decentralized, and ephemeral
https://chitchatter.im/
GNU General Public License v2.0
1.51k stars 185 forks source link

[Security issue] Chitchatter has 28 security holes introduced via unmaintained or outdated dependencies #185

Closed Darin755 closed 10 months ago

Darin755 commented 10 months ago

I when I went to run this on my local machine and I noticed that chitchatter depends on faulty dependencies. I tried running NPM audit fix --force but that yielded unreliable results. There seems to be multiple libraries that are out of date and others that are unmaintained. This is really bad for security and needs to be taken seriously.

Ideally there shouldn't be any security issues. Tracking down all of the vulnerable packages will be a nightmare but it should be worth it in the end. I am willing to help out in any way I can but I don't know the code base so it will be challenging. I would recommend avoiding the use of libraries especially ones that are small, obscure and privileged.

Once this project is passing security checks I will open a PR to add a docker build and compose file.

jeremyckahn commented 10 months ago

Thanks for raising this @Darin755. Because I'm developing Chitchatter for free in my spare time, I don't have the bandwidth to find and fix security issues in upstream dependencies. I do my best to be diligent about merging @dependabot PRs that fix such issues, but that's the most I can reasonably do while still leaving space to improve the feature set and user experience of Chitchatter.

I would recommend avoiding the use of libraries especially ones that are small, obscure and privileged.

I generally avoid using NPM libraries for trivial things and only pull them in when they address a significant need for Chitchatter. To implement bespoke alternatives would increase the scope of work for developing Chitchatter considerably. This would slow down progress a lot, and it would likely cause me to lose motivation to work on the project at all. It's also not a guarantee that doing so would make the project any more secure, as Chitchatter would then solely own a much greater maintenance burden.

I would greatly appreciate support in addressing these upstream security vulnerabilities, as my energy is focused on making Chitchatter's user experience and feature set the best it can be. For now, I'm choosing the passive approach of waiting for and merging @dependabot PRs.

Darin755 commented 10 months ago

I will see what I can do. For now I would recommend you put a note about security in the readme.

Fixing these security holes is going to involve a lot of work but I figured I'd start by bring it to your attention

Audiosutras commented 10 months ago

Hello @Darin755 and @jeremyckahn,

Just making a note for who takes this task on. I have the time to do so if needed. I appreciate the work the contributors have put into the project and would like to contribute to maintain morale!

  1. Article on using npm-check-updates for identifying upgrade-able packages.

  2. The codebase will need to be inspected for deprecated methods for the packages that were able to be upgraded. This will require replacing the deprecated method with the newly supported method

  3. For packages that are not able to be upgraded that have vulnerabilities we'll need to inspect what part of the codebase the packages "touch", understand the package's function, and replace the package with an alternative.

I think 2 and 3 can be separate pull requests.

What do you think of this as a course of action?

jeremyckahn commented 10 months ago

I will see what I can do. For now I would recommend you put a note about security in the readme.

Fixing these security holes is going to involve a lot of work but I figured I'd start by bring it to your attention

@Darin755 Thank you! Displaying security status in the README is a great idea. I've set up Snyk security monitoring and pushed https://github.com/jeremyckahn/chitchatter/commit/19f13093e8c78a061b37c1ad60cb1befbef06e20 to display the live status.


Just making a note for who takes this task on. I have the time to do so if needed. I appreciate the work the contributors have put into the project and would like to contribute to maintain morale!

Thank you for the support @Audiosutras, I really appreciate it! Based on Chitchatter's Dependabot page, it seems that the majority of Chitchatter's current vulnerabilities stem from libp2p, which is depended upon by Trystero (a key depenency of Chitchatter). However, libp2p seems to only be used by Trystero when the IPFS strategy is used. Chitchatter exclusively uses the WebTorrent strategy, so those vulnerabilities don't seem to apply in practice for Chitchatter (though automated scanning systems don't appear to sophisticated enough to detect as much).

It would be worth seeing if Trystro could somehow be patched, as doing so would address the vulnerabilities beyond the scope of Chitchatter.

  1. Article on using npm-check-updates for identifying upgrade-able packages.

I will check this out! Thanks for the link.

2. The codebase will need to be inspected for deprecated methods for the packages that were able to be upgraded. This will require replacing the deprecated method with the newly supported method

I'm going to explore what other scanning tools could be set up to find vulnerabilities in Chitchatter's application code. Once they are found, I will fix them ASAP (but PRs are welcome if you or anyone else is able to get to them before I can).

3. For packages that are not able to be upgraded that have vulnerabilities we'll need to inspect what part of the codebase the packages "touch", understand the package's function, and replace the package with an alternative.

That sounds like a good idea. Are you envisioning a manual audit of the code base, or some more automated solution?

Audiosutras commented 10 months ago

@jeremyckahn Good to know about the vulnerabilites found in the Trystero package. I'm unable to view the dependabout, but I think we should open an issue for Trystero so they fix the vulnerabilities on their in when they have time.

Regarding packages that cannot be upgraded and have vulnerabilities, I'm not familiar with any automated tools that provide solutions. I was thinking at a bare minimum the packages could be identified manually and added to a Kanban board as something we should keep our eyes on for manually replacing with another package.

Under the envisioned security section in the READMe you were discussing with @Darin755, we could link to the "Dependency Upgrade" Kanban Board.

The kanban issue could have:

1. package name
2. package description
3. The Vulnerability (Level of vulnerability - Low, Medium, High)
4. list of potential package replacements

If we could give users an idea of the level of the vulnerability it will help form whether they are willing to use ChitChatter in its current production state.

Audiosutras commented 10 months ago

Now in terms of actually scanning the application for security vulnerabilities Zap provides a number of automated tools. I've used the Zap desktop client in the past. It will generate a pdf report that could be added to the readme. Note this is a separate issue from upgrading dependencies.

jeremyckahn commented 10 months ago

I'm unable to view the dependabout

I didn't realize that link wasn't publicly accessible. I guess it shows up for me because I own this repo. Sorry for the broken link!

Good to know about the vulnerabilites found in the Trystero package. I'm unable to view the dependabout, but I think we should open an issue for Trystero so they fix the vulnerabilities on their in when they have time.

Great idea @Audiosutras. It seems that ipfs-core, the key problematic dependency of Trystero, has been superseded by Helia. I recommend exploring a PR for Trystero to update it to use that library to address the root cause of this dependency vulnerability.

Regarding packages that cannot be upgraded and have vulnerabilities, I'm not familiar with any automated tools that provide solutions. I was thinking at a bare minimum the packages could be identified manually and added to a Kanban board as something we should keep our eyes on for manually replacing with another package.

I'd like to hold off on this idea for now. I think tracking this sort of thing via a publicly-facing tool is a good idea, but it would be even better to find an automated solution so that things don't get out of date or need to be manually managed. The recently-introduced Snyk integration seems like a good way of managing this in an automated fashion, so I'd like to see how far that gets us first. I've also just opened #193 which seems to eliminate all known vulnerabilities, so once that's merged there hopefully won't be anything to track for a little while (🤞).

Now in terms of actually scanning the application for security vulnerabilities Zap provides a number of automated tools. I've used the Zap desktop client in the past. It will generate a pdf report that could be added to the readme. Note this is a separate issue from upgrading dependencies.

Oh nice! That seems like something worth exploring. I can explore that when I have some time, but feel free to open a PR to integrate the tooling if you'd like to set it up before I have a chance to do so. 🙂

Darin755 commented 10 months ago

Thanks! I didn't expect this to get fixed so fast.

jeremyckahn commented 10 months ago

@Darin755 Neither did I! I assumed it would be a huge undertaking of tracking down and patching upstream dependencies. After looking at it more closely, it was mostly a matter of excluding one very problematic dependency that wasn't even being used and setting up a few other sub-dependency version overrides.

Thanks again for bringing my attention to the issue!