> There has been some demotivating discussions and statements on the > mailing list about input validation, and I really don't understand the > attitude. It might be based on misunderstandings and miscommunication. I > have tried to write a (long!) summary of what I think is the topic of > discussion, and I would appreciate if other more senior developers would > comment on it and clarify the direction we want to take in FreeRDP.
I assume you are talking about me, and yes, I want to demotivate the use of asserts this way. When I see a major problem in the code that is not addressed, plus more and more users frustrated, yes, my wording will get harsh. This will never change. > Trade-off > > I assume we in principle can agree that security and stability is more > important than features and performance. Security issues can prevent all > use of the product, while missing features or bad performance only > prevents some use of the product. We must obviously also keep in mind > that missing features or bad performance can be so bad that they make > the product unusable for some purposes. But features should never > compromise security, and optimization should never compromise > correctness. It must be accepted that fixes for security and correctness > might cause a reasonable reduction of features and performance. I think "Security" is used a bit loosely here. I have 2 issues with this paragraph. "Security" is not above features and performance. most of your checks have nothing to do with security. Yes, we want to parse the protocol correct but no, "Security" is not always above features and performance. This is a community project, has anyone asked for this "Security" or are you on your personal quest. In all the years of RDP consulting, I have never been asked to add "Security" checks like this. What is wrong with a server putting an extra byte or two at the end of some PDUs? How is that a real security risk? Can you reference a feature request for this "Security"? I have been asked time and time again for more performance and smaller size. I'm proud of the performance we have and I'll continue to push for more and more. Another thing I'm proud of - although rdesktop is an X11 one session client, FreeRDP is meant to be used everywhere from small embedded system without X, to mainstream X desktops, to huge servers via xrdp. This is a really cool thing about this library. > Many program properties holds as invariants and can perhaps be proven, > but for program input the only options are to either check it, or to be > naive and just assume that it is correct and perhaps consider the > potential consequences. What consequences? > Each check has a cost, both in code size and CPU consumption. Obviously, > if the cost/benefit ratio is too high (because the check is expensive or > the benefit is low) it shouldn't be done in production code but might be > useful for debugging. > > Some assertions can be proven at compile time, and then there is no > benefit from checking them at run-time and they could just be disabled. > > But what if the assertions states assumptions that can't be proven? The > code has neither been tested nor designed to work without these > assumptions. Is it fair to give users code that has unintended reachable > code paths where anything can happen - especially security-wise? I don't > think so. If the cost is reasonable the checks should be left in. No, I disagree. > Assert or not? > > > How should such validating checks be implemented and what should we call > them? > > C assert()s has traditionally been implemented as macros that often are > disabled in release builds and completely disappears. When enabled an > assert() expands to an if statement optimized for false that checks the > negated boolean expression. If the assertion fails it will print > location information and the text of the asserted expression, and then > it will abort program execution immediately. > > I have seen several "modern Linux" projects that not necessarily disable > assertions in release builds. I have seen several assertions from my > desktop OS, and they have mde it possible to find or file relevant bug > reports. If it wasn't for the assertions the result most likely would > have been unusable reports of random crashes and segmentation faults and > bugs that wouldn't be fixed. What? It is not common to release code with assert active. What library are you talking about? > FreeRDP do have a number of assert() calls in asn1/ - do anyone do > anything to disable them? Do you know about NDEBUG? It looks like we are not setting it now but we need to. > We also have some exit() calls in libfreerdp. (Some "code quality" tools > correctly complains about using that in a library.) These exit() calls > also shows some kind "do whatever you want, but don't ever come back" > assertion pattern. The exits were removed, thanks to this tool. That seems to prove my point, a quality tool does not like exit in a lib? Is assert an exit? hum. If there are more, they will be removed too. > What I did in FreeRDP was to add some ASSERT macros in > libfreerdp/debug.h that wrapped the standard assert() from assert.h. The > macros makes it easy to find all uses, change them or change the > implementation. assert() was a rough approximation of what I had in > mind, so I based the macro implementations on that and named the macros > after that. > > A consequence of that is that the user experience on assertion > violations is that the application crashed. In a way they are right, but > the user was protected from a potentially worse crash, and the crash > provides some useful information to the developers. This scheme works, > but we could do something better. > > Another consequence of the naming is that it might indicate that these > are expensive checks and that they should be disabled in release builds. > Neither of these connotations were intended. I intended (and used) it > for general handling of cheap validating checks that I think we always > should do. > > Perhaps I should rename ASSERT to ENSURE or VERIFY or GUARANTEE or > VALIDATE or something else ? Perhaps the implementation should be > changed to something that won't go away when exceptions are disabled? Have you thought about returning an error. That is the right thing to do. Asserts are the lazy way out. > Thread safety > > > A problem with all fatal exception handling in a complex C library like > libfreerdp is how to report fatal errors back to the main application > and allow the main application to continue running without interrupting > other threads and without loosing any resources. > > The only good solution I know is to let _all_ functions take a parameter > with a call-back for error reporting and make sure error codes are > returned and checked all the time. All functions must make sure they > release all resources on errors - often by using error labels and goto. goto? Why goto? You can write clean code without gotos. > That is really not a good solution and hardly feasible, but I don't > think there are any other solutions. Do we want to do that? We need to return an error. What is so hard about error handling? > The alternative is to only handle the most common exceptions gracefully > (such as ordinary logout and redirect) and accept that other exceptions > can bring the main application in a state where it can't continue. That > is what we have now. No, when I rewrote the stack for xrdp, all functions return as error. It's not that hard and it's the only right thing to do when the stream becomes unparseable. This way, only the connection that has the problem will disconnect. Imaging this, libfreerdp is as it is today(with the asserts) and used on an xrdp server? There are 50 connections, most not using libfreerdp. One libfreerdp user's assert brings down the entire server. How can you say this is OK? Image if a library like libcurl or libssl asserts, instead of returning errors. Processes would be crashing all over. libfreerdp should returns errors just like any other quality library. > Input stream bounds checking > > > I am guilty of introducing bounds checking in our low level input/output > stream processing macros in libfreerdp/stream.h. > > I did not consider it controversial or risky at all. > > I carefully considered the potential performance impact. Tests showed no > noticeable difference. Later benchmarks confirmed that the cost even in > the most extremely pessimistic case would be less than 1% of the CPU. > After an undiagnosed complaint I immediately disabled it so it now only > is enabled when configured --with-debug or ENABLE_STREAM_CHECK is defined. > > The checks (instantiated inline almost 1000 times) did however increase > the size of the executable with 40k. Half of that is error messages. > That increase can in principle have a big impact on small devices. The > size overhead could be reduced a lot at the cost of slightly less > detailed error messages. > > I have not seen any evidence that these checks caused a noticeable > overhead. No bisect that points at a specific change. No clear > indication what difference it makes if ENABLE_STREAM_CHECK is toggled. > > These boundary checks are a simple and very reliable first level of > input validation and protection from buffer overflows. We could and > should do something that is a lot smarter and efficient, but this is all > we have now. FWIW: Considering how simple it is for a rogue server to > crash or attack a client without these checks I think it is > irresponsible to ship freerdp without these checks. > > ( http://sourceforge.net/mailarchive/message.php?msg_id=26528018 ) I totally disagree. You are defeating the purpose of using macros. It would be better to create in / out functions if you want to do this test at this level. Again, this is the lazy way out. Are you testing on a fast desktop machine and using a visual comparison as to if it's slower with this on? This is not a good test. > Over-specified inconsistency > > The RDP protocol specifies over-specification lots of places, especially > in nested structures where each nesting level has its own length field. > Obviously these lengths should add up and match. What should we do if > they don't match? > > Detected inconsistencies shows that either someone (an attacking or > buggy server) sent us incorrect data that by definition can't be > interpreted correctly, or that we have a bug in our implementation and > can't reasonably expect that if we do our best and continue then nobody > will notice. > > Ignoring inconsistencies and just try to continue using one of the > inconsistent values might work in some cases, but obviously that is not > a good solution. It is better to inform the user that bad things > happened and that if the user thinks it is a bug in freerdp then the > developers should be notified and add a bug-fix or workaround. > > I added some of that kind of checks - especially in libfreerdp/rdp.c for > checking that we really processed exactly the right amount of data from > the input streams. That has helped catching several bugs by pointing out > early that a problem occurred, long time before we would have stumbled > upon the consequences. > > It is annoying when these showstoppers pops up in cases where it would > have worked if we just had used one or the other value, but I don't > think that counts as an argument for just choosing one of them by default. > > > Code review? > > > I have described why I don't think the changes I made are controversial > at all. I did not know that some developers had that strong feelings on > this topic - and I still don't understand why. Wow, I don't understand why you don't understand. > When I made the changes it was anarchy with no tradition for review. > Fortunately that has improved a bit since then, but we don't review > everything. Even today I doubt that I would have considered my changes > in this area so interesting or controversial that I would have annoyed > you with a review request. Exits all over, size and speed hits and you didn't want to annoy anyone. > If someone thinks the guidelines should be further formalized, clarified > or updated then please do so. > What now? > If someone will undo something then I can't and won't stop that. Maybe not undone, but this has to change. > If fellow developers can explain and agree which changes they would like > to see then I might be able to do that. Yea, what do the rest of you think? Jay ------------------------------------------------------------------------------ Protect Your Site and Customers from Malware Attacks Learn about various malware tactics and how to avoid them. Understand malware threats, the impact they can have on your business, and how you can protect your company and customers by using code signing. http://p.sf.net/sfu/oracle-sfdevnl _______________________________________________ Freerdp-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/freerdp-devel
