Just like there is no spoon?
1 Like
Hey Tom,
The long and the short of it is that there were some changes that were made in the restapps module recently that were trying to fix a few bugs in phoneapps usage with phones in funny network environments - one of the more recent changes caused this regression.
We did not previously have a unit test written around this particular exploit (mostly due to the fact that there were some significant challenges around unit testing the code due to its former architecture). After this though, we’re going to be dumping a bunch of time into refactoring that code to make it more unit testable and so we can include a test case to cover this and any other similar issues that may come up in the future.
Big apologies again for the trouble around this.
1 Like
Matt, I’m just going to be brutally honest here. I will need to look at the v16 code but everything code wise that I have looked at up until 15 is a frigging mess. There is zero consistency in the coding standards so I’m pretty sure that can have an impact on things.
I mean let us take your Asterisk counterpart as an example. If I look at the code for something I cannot tell that 6 developers have touch the code because it is consistently the same through out. Comments, formatting, structure all the same. On the other hand when looking at FreePBX code you can tell multiple people have touched it because there is zero consistency through out the code. Some of it is formatted in actual readable and logic ways, some may have comments and my favorite is the developers that insist on writing PHP code like JQuery/Javascript with all the code minified in to a few lines with no spacing or readable formats.
So talks about unit tests being written or other such things worries me due to the lack of actual coding guidelines and practices that I’ve seen through the project.
1 Like
Coding and uniformity standards as well as testing info is all documented. They simply aren’t enforced.
At new ventures all of this is enforced with ci. We use static analysis etc and nothing can mainline without passing
Thanks for your honest feedback Tom.
I think as a project we’re trying to move things in a direction where we have a bit more consistency as far as code styles and consistent best practices go. As new contributions go through the code review process, we welcome any and all positive participation in helping with those goals.
Separately from that, automated testing is an area that we have reaped great rewards from on the Asterisk side to provide significant comfort and stability to its code base. It is a personal interest of mine to bring those benefits to FreePBX as well.
Best wishes,
Matthew Fredrickosn
On the OP topic
I mean, I’ve been complaining/speaking out about the coding standards since pre-2019. So you’re saying they’ve existed all this time and no one has ever enforced them? Because having a full time Sangoma QA Team was something flexed about in 2017/2018 because it didn’t exist before.
This now makes me wonder how long this project has had policies and procedures in place that have been ignored and not enforced at any level of the hierarchy.
speaking of, that post links to a wiki article that says little, but links to this bug report that has apparently been hidden.
https://issues.freepbx.org/browse/FREEPBX-23176
Why post such a report then? Just make it a FREEI that none of us can ever see anyway as it was a commercial module vulnerability anyway.
This was the bug ticket I filed, which had to be a “FREEPBX”
1 Like
James came up with those standards and wrote them into the wiki before we were able to have a devcon again together to discuss. They were never discussed at length but they do exist in the wiki yes.
Freepbx itself was created in the php 4.x era of time. Well before php itself had standardization. Or PSRs or even static analysis tools. The php of today is vastly different from the php when freepbx started. Or even the php of 2009 when I found and started contributing to freepbx. There were no standards then from the people running freepbx.
No standards were enforced throughout my time at Sangoma (either from me or others). Should they have been. Yes definitely. But they weren’t.
I assumed.
My point was why even post that in the wiki if it is not going to be public.
Some effort needs to be done to sanitize things before the ticket can be opened to public.
2 Likes
Those are fair points. Which always has gone to my point that sometimes you have to go back and readdress things with the changing landscape. Which can mean a re-write, not kludges to get around rewrites. The Backup/Restore module is the poster child for this.
It got so bad and unmanageable with all the kludges and hacks stack on it to quickly fix the existing problem with no larger view of issues it can cause.
Just like back in Feb. of the year when O had all sorts of issues with Advanced Recovery and no one could figure out why. Then core was updated and boom, problem fixed. So a change was made to core that broken dependent modules. Proper testing would have caught that. Not a random update that fixed problems no one knew was related.
Going a little off topic here.
@billsimon, do you mind sharing how you discovered this vulnerability?
Thanks
This was early on the train of finding things out, but this does not mesh with the final report in the wiki.
What is the correct answer?
1 Like
Wiki was inaccurate, I’ve edited to fix. Thanks for the correction.
2 Likes
Hey Jared,
Great question. Still working on getting all the documentation right.
I think that was Lorne’s first cut at it, probably using a boilerplate template from a previous exploit. Impacted versions are the ones I previously posted:
16.0.18.40
16.0.18.41
15.0.19.87
15.0.19.88
We’ll update the wiki to not include that language.
Matt
1 Like
His original post mentioned an alert that his passwd file was being altered by something. He has monitoring setup on his systems
4 Likes
The versions affected seems to be wider than what is currently listed.
I am running version 13 right now and I am under attack… Still trying to mitigate the issue and see what then are trying to do with my system. I have HIGH cpu usage, disk filled with error logs, lots of sed processes running dumping stuff. Not sure if it’s a DOS attempt or trying to collect info or…
You have an unrelated issue, which you’re free to open a new thread to explore but I would recommend updating to a supportable version first.
edit - you may be seeing this Sangoma Documentation
but again, does not belong in this thread.
1 Like