Hello everyone, I decided to get back into some Freenet hacking for a bit. Nobody knows when I'll disappear again so enjoy it while it lasts ;)
After getting interested in meshnets/darknets again I did some research and decided that a Freenet like system with forums would be the best bet against oppressive regimes. I took a look at Freetalk and got it to build again, but it seems Freetalk is quite dead and WoT progress isn't great either. Then I took a look at FMS and I was pleasantly surprised. It works well and is fast. The only downside seems to be that it is written in C++ and so a bit harder to include in the main Freenet distribution by default. I think it would be good for Freenet to have a default and official forum system. If anything, Freetalk should implement the FMS protocol so it is inter-operable. No need to reinvent/reprogram the wheel when we have something that works fine. After a small discussion on IRC it turned out that a code review would help. I was quite curious myself how FMS worked so I took a few hours to browse through the source. I will also post this to the FMS board so please add some trust to my FMS identity so people can actually see my post: SSK@cptAO5z0rfUEzlwdyUggtoXdaeQ9XIZzrNBCvBteTDQ ,~-0EZgy~RpU99ThMiyvoiZTpYGgRsY~8GsGJxmZG47U,AQACAAE/ Regards, Gerard ### Quick code review of the FMS source code by gerard-/gerard_ ### Downloaded from CHK@GSJJ7n8 ~uBF3dxIqsJVy53g~rJZk8-MlyqN2nGN-Ces,wQIfL5UgA8Uh3ueTvEfIkTMk0dj9-Gcd~5qr97rseXE,AAMC--8/fms-src-0.3.75.zip $ sha256sum fms-src-0.3.75.zip 1fe5e84aba572143fe95ffb384177e150d5cd4dff2930462b9df24b59c03d85e fms-src-0.3.75.zip What was checked? * All .cpp files in the directory /src/ and all .h files in the /includes directory were quickly read through. * FMS was built from source What was not checked? * The /libs directory was not checked. The best way do do this is probably to find the exact version of the library and diff all files. * It was NOT checked if the provided binaries correspond with the source file. To be safe(r) compile from source. ## Summary ## I spent 4 hours reviewing the FMS code. Disclaimer: I am not familiar with the FMS code and also not familiar with the Freenet code. I am also not a professional code auditor. Verdict: Nice code, could been written a bit more defensively. No suspicious code was found. The output parts (HTML and Unicode/charset) should be rewritten to be obviously correct. General architecture seems well thought out and will be a good foundation for future improvements. The main improvement would be a reproducible build to verify the binary corresponds to the source. The licensing situation also needs to be cleared up. ## Building it ## Building from source was easy and fast. I could not get the new captcha to work due to a build error. After building (with the old captcha, audio captcha enabled) Filesize 6482312 The produced binary seems to function the same as the one provided in binary form. Original (64 bit linux download from Freenet): Filesize 9773568 Filesize of the original is bigger, which is quite suspicious. This could be explained by the new captcha or by being built on a different system. ## Code review ## # general Bad: FMS does not seem to provide repeatable builds Bad: Code does not contain assert statements to aid debugging Bad: FMS does not seem to contain any measures to protect against someone having access to your own computer. One could argue that those measures would be ineffective anyways. Good: The code is of overall high quality Good: concurrency/threading system is nice, all communication goes through sqlite Bad: Most files don't have licensing information, the licensing situation needs to be investigated for every file # /src Good: Code uses modern C++ with STL Good: Code uses iterators Good: code uses std::string for string manipulation Bad: void TranslationPropertyFile::parseLine(std::istream& istr) contains manual parsing code with loops, this is usually error-prone Good: code uses prepared statements everywhere Bad: stringfunctions.cpp is scary, please use a library for string manipulation. Also looks like some premature optimization has taken place. Bad: QuoterItemText::GetSanitizedText doesn't operate using a white-list. There are no comments to see what this function is supposed to do and if it is correct. Bad: all functions need a comment next to the implementation that shows what the function is supposed to do. This makes code reviews and implementation changes easier and safer Bad: Option::GetBool(const std::string &option, bool &value) should fail when it receives an unexpected value instead of considering it "false" Bad: hardcoded option strings like "UniqueBoardMessageIDs" Bad: message.cpp:703 - just replacing every instance of \r\n with "" may have unexpected results Bad: message.cpp:935 - the code "strips" certain things, a more robust method would be to parse it and only process it when parsing is successful - this problem occurs on other places in this file Main risks: There is a lot of text parsing going on. If this goes wrong an attacker could inject javascript into the web interface of possible fake messages. Any text that is processed should be fully parsed and understood before it is modified. Bad: IPAddressACL::Add(const std::string &aclentry) has a weird m_allowbydefault member, and any element that is added that does not start with a plus or a minus will be recorded according to that. Consider always requiring a + or - to remove any ambiguity. This function should be a bit more strict overall. Bad: IPAddressACL::MaskAddress just passes through an invalid address (less than 3 dots) Bad: hex.cpp doesn't warn then the number of half bytes is uneven Bad: why does fmsapp.cpp contain a testregex function? Unused or test code should not be present in the real codebase. Bad: the database doesn't seem to use foreign keys, data may become inconsistent leading to crashes and other bugs Good: there are some range checks in the database Bad: home-grown Base64 conversion in base64.cpp ## /src/db Bad: Transaction::Begin uses string operations to build a query. The code is correct but it would be better to just split up the m_db->Execute call and move it inside the switch. Bad: Statement::ResultBool does not warn when the result is unexpected Bad: DB::HandleError throws an exception but the rest of the code does not seem to use exceptions # http/pages Bad: All pages build html and call SanitizeOutput themselves. Consider using a library that will build the HTML tags and that will take care of sanitizing. Functions XMLCreateTextElement and XMLCreateCDATAElement might be useful. Bad: ShowInsertedMessagePage::GenerateContent manually builds an SQL query instead of using a prepared statement variables Bad: Pages use HTTP GET for trust-list updates Bad: ForumTemplateMainPage::GenerateContent uses magic strings, replace those with consts or with defines # nntp/mime Bad: Code with quite a bit of pointer arithmetic, from 2000. I think it is some sort of a library but it should be replaced with something more modern. A very thorough code review would be needed otherwise. Bad: uwildmat.cpp contains complicated code that will have to be reviewed in depth # unicode Bad: unicodestring.cpp: the code has empty catch blocks, the string probably needs to be invalidated when an exception occurs Bad: what is FMS doing with all this unicode conversion code? Recommendation: standardize anything on UTF-8 and have the browser/newsreader handle the display. # /src/freenet Bad: All XML parsing code has an empty catch block Bad: introductionpuzzleinserter.cpp:180 - the code checks for failure, should check for success Bad: identityintroductioninserter.cpp:70 - the code checks for failure, should check for success _______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
