Removed 411 lines of Useless AI Generated Information.#694
Removed 411 lines of Useless AI Generated Information.#694modernham wants to merge 1 commit intoaccius:mainfrom
Conversation
The SECURITY.md file was 89% AI generated garbage that is obvious to anyone in the industry, not unique to the project, and can be found online. Removed 411 lines to make is more concise and readable to attempt to turn it into an actually functional document. As a security researcher, 463-line SECURITY.md file filled with 89% AI generated garbage doesn't really reflect the other AI generated statement at the top of it "The OpenHamClock community takes security seriously."
|
Hey, appreciate you taking the time to look at the security side of things. We've gone through the three vulnerability reports you submitted and wanted to give you a quick rundown of where we landed. Report 1 (rotator endpoint) - Valid. Missing auth middleware on two endpoints. Fixed and merged. Worth noting the affected feature defaults to disabled and is a no-op on default/hosted deployments, but the missing auth was still a real oversight for self-hosted users. Report 2 (credential proxy) - Partially valid. The missing auth was a legit issue, same class of bug as report 1. Fixed and merged. Some of the claims in the report about retry behavior and fake blocking were inaccurate though. We can follow up in the advisory thread with specifics. Report 3 (SSRF) - This was the strongest of the three. The existing validation approach was bypassable and has been replaced with a fundamentally different mechanism. Fixed and merged. All three are shipped. Regarding the broader comments about the project, we hear you that security matters and we're taking it seriously. But some of the characterizations in the PR description don't fully line up with what we found when we dug into the actual reports. One was a real architectural gap and two were missing middleware on specific endpoints. We reviewed each one on its technical merits and fixed what needed fixing. We do need to say this though: publicly advising people not to run the software, speculating about the development process, and editorializing about the project's maintainers in a PR description is not how responsible disclosure works. The findings stood on their own. The commentary undermined them. If you want to be taken seriously as a security researcher, let the work speak for itself. We responded to the technical substance here and we'll keep doing that, but the tone made it harder, not easier, to have a productive conversation. You mentioned identifying 18 vulnerabilities but only reporting 3. We'd genuinely like to see the other 15. If there are more issues in the same tier as report 3 we absolutely want to fix them. You can submit them through GitHub Security Advisories or reach out directly. We'll review each one the same way we did these. |
|
I understand I can be a bit brash. We've spent the last 20 years asking users not to execute unknown programs on their computers. I use AI as a tool myself, but asking users to run code nobody is reading or validating is just not responsible. I've just pulled the Staging Branch, reviewed the changes, and here is my feedback: As for Report 1:
It is enabled by default, despite what the UI states, or the AI, or whatever you are looking at. As for the fix to require authorization for this endpoint: As for Report 2:
A tcpdump for outbound connections via port 443 shows repeated attempts after failure to the QRZ endpoint. But, I can no longer set the credentials on production, which is good. As for Report 3:
It's not fixed, and no branch has been merged. There was a commit to the staging repository. The UI states:
All you need to do is run a tcp dump on the system, filtered by the outbound telnet port to see the network traffic. And again, just launch the POC in report 3, and see the traffic in the TCP dump window to whatever external IP was specified. Doing so will still have the server connect to any endpoint specified , as seen in the tcpdump output. The SSRF is still a problem, and the server does not block any of these outbound connections. (I can clearly see it connect with my eyes to any other server I own and am listening on). Not only this, but your commit for this issue didn't solve it. Localhost can still be queried if the input host has a valid external A record at the time it's set, but then replaced afterwards. The hostname is passed to the backed application, not the IP itself. Once it's resolved again, the check is bypassed. There is also another way this is bypassed via IPV6. My main point being: The most critical vulnerability report should not have been closed (Report 3). The issue still exists as a SSRF. And the firewall bypass still exists within it. The several security commits actually introduced a new high vulnerability, (Rate Limit for the SSE feature), and failed to fix the critical one above in Report 3. The irony is that that commit 5cfeef3 in staging actually defeated an existing security measure in place, by slapping a user controlled variable on previously working measure, which now introduced several problems in itself. This is what the industry calls technical debt. This isn't the only project to see it by far. These applications that are entirely vibe coded can't scale, and the code becomes "write only". AI does not yet have the context window to read or understand your project, and so it will only keep writing code into it. No human is going to read the codebase to review it in it's current state (75k+ lines of code, with a 12k line backend js file). AI isn't at a point where we can ask it to do that, and trust it works. It has to be validated (because it is really bad at making secure applications). The fact of the matter is, this application is obviously doing a lot of things you aren't aware of. Instead of asking the AI, someone (like I did), needs to actually validate mitigations, and attempt to read the code. I'm not trying to be mean. I'm sorry if I come off that way. I just feel like someone needs to be the voice of reason here to an obvious problem. But responsibly shipping code to others requires we understand it, if nothing else. Code review takes much longer than writing code. The project looks to have on average 2-3k lines of code written a day. Even if an AI wrote all of that instantly, that's not enough time for code review to reasonably take place. The features have outpaced any engineering or software design principals. This is a great project, and I'm sure everyone learned a lot from it. But there needs to be a major refactor to make it secure and stable, and AI is not capable to refactoring a codebase this large into the size it should be. I would suggest a new branch with a fresh start using everything you learned. AI can still be used but the project needs to start with some:
Then have a human go through and validate/test every implementation phase, and verify security controls with every new feature. I would still have some web application security testing done by someone before deployment to the web. But this is a responsible way this could move forward. Continuing to scale the current code base is not going to end well, and it's not safe. |
|
Hey, genuinely appreciate you coming back and validating the fixes. You were right on the rotator being enabled by default via .env.example, right on the DNS rebinding TOCTOU in the SSRF fix, and right on the IPv6 bypass. All three are fixed now. The startup warning for missing API_WRITE_KEY is also in. You're also right that we should have caught the rebinding issue ourselves before calling it fixed. That's fair criticism and we're taking it seriously. On the refactor suggestion, we hear you on the 12k line server.js being a problem. We're not going to do a ground-up rewrite though. This is a live application with 2,000+ users and active contributors, and rewrites have a way of taking forever and shipping with fewer features than what they replaced. What we are going to do is start breaking server.js apart incrementally. Rotator, QRZ, DX cluster, WSJT-X, each one is a clean extraction into its own module. One PR at a time, nothing breaks, and the codebase gets progressively more reviewable. The sprint plan and documentation suggestions are solid and we'll adopt those practices going forward. Look, you clearly know what you're doing. The DNS rebinding catch alone tells us that. We could really use someone with your background keeping an eye on things as we work through the remaining issues. If you're interested in being involved more directly, whether that's reviewing security-related PRs, doing periodic assessments, or just being someone we can bounce fixes off of before shipping them, we'd welcome that. You don't have to mass-commit to anything, even just having you in the loop on security-sensitive changes would make a real difference. Either way, thanks for pushing on this. The project is better for it. |
|
Also when I say "all three are fixed now" I mean, "Hey, can you check this out for me?" |
|
The SSRF vulnerability still exists. The fundamental problem mentioned before is still the issue. The solution to all of this is a rewrite with code review form the beginning with proper software engineering principals in place. It's not about solving this one specific issue, because there are many many more, including a new one introduced from the attempt to solve this one. Rewrites are part of the development process. This is true, and will always be part of the software development process in the beginning of a project like this. You are going to reach a point soon if you haven't already where code fixes are a game of whack-a-mole, with the AI only adding more code each attempt to fix anything. The more code that is added, the more instability you will see. And this is all going on with the security issues that are still present. What you are seeing here is expected. There are studies published on this exact problem. |
2 participants
The SECURITY.md file was 89% AI generated garbage that is obvious to anyone in the industry, not unique to the project, and can be found online. Removed 411 lines to make is more concise and readable to attempt to turn it into an actually functional document.
As a Security Researcher, and a Software Engineer for more than a decade, I do have some harsh feedback that I will voice here, right place or not because something needs to be said about running this project in production, or anywhere others can access.
It looks like the entire project is basically AI generated code, the contributions are almost all AI generated code, and even the code review in pull request replies is just an AI critique on the other AIs generated code, often with errors.
I submitted several vulnerability reports, but while researching I saw many places AI had "cheated" or faked security within the codebase. The core issues remained, telling me the code isn't being validated or reviewed.
There are classic signs of a codebase where an AI was asked to "make sure it is secure", and in case people are unaware, this just doesn't work yet. In this case, the project is significantly less secure because documentation states some mitigations are in place that do not exist whatsoever. Some others even have fake log messages stating things are blocked, when the code doesn't actually do anything to back it up.
Of the 18 vulnerabilities I identified, I only reported 3 due to this core problem. It almost seems aimless.
The problem with vibe coded applications like this is the technical debt that it occurs. There is a basic principle in software engineering that states that every line of code is a liability. This codebase has 70,000+ lines of liability, and without a foundation in best practices, this isn't maintainable or safe. Public endpoints are actively being slammed with garbage API requests, login attempts, and I can't imagine they are happy with it. There are several TOS issues with these services that this application is using that operators are unknowingly violating. There are just too many fundamental issues.
Neither basic Cyber Security practices, nor any human in the loop/code review in this project seems present. Afterall, no software developer in the right mind would attempt to read code in a 12000+ single backend JavaScript code file.
This looks to be a mass amount of people blindly executing unreviewed code that the maintainers do not understand.
As a professional in this industry, I have to advise nobody run this code in any environment where they care about their data, or where the server can be used to attack others, because it is not safe. A lot of systems are going to be compromised here, and I'm surprised I'm the first one to bring this issue up. If there is a lesson to come out of this, it's that human code review is still important.