tag:blogger.com,1999:blog-37798047.post8535913757846485180..comments2024-01-16T05:48:33.523-05:00Comments on Errata Security: Systemd is bad parsing and should feel badDavid Maynorhttp://www.blogger.com/profile/09921229607193067441noreply@blogger.comBlogger11125tag:blogger.com,1999:blog-37798047.post-7937735815424950982018-11-03T13:51:33.781-04:002018-11-03T13:51:33.781-04:00HelpHelpAnonymoushttps://www.blogger.com/profile/10995113598234263655noreply@blogger.comtag:blogger.com,1999:blog-37798047.post-43531137931434737242018-10-28T15:29:55.782-04:002018-10-28T15:29:55.782-04:00As a C programmer of some decades who has fixed an...As a C programmer of some decades who has fixed and written up numerous CVEs, my reaction to the article is:<br /><br />* I use pointer arithmetic, and I am not aware of any consensus among security-conscious C programmers to avoid it. For instance, BoringSSL's CBS abstraction (a fairly recent invention used to safely read from buffers of known length) uses pointer arithmetic. So I disagree with the article on this point.<br /><br />* I do not like byte-swapping functions that convert integers to integers because I don't like the idea that an integer variable could hold a byte-swapped value, with no assurance from the type system that the value won't be used without swapping it to native byte order. Abstractions that convert byte strings to integers and vice versa are clearer, whether they are platform-neutral or (for performance) platform-specific. So I agree with the article on this point as a matter of personal taste, but I haven't seen evidence of a strong consensus that all code should avoid integer byte swapping.<br /><br />* Manipulating a pointer and length in lock-step, by hand, isn't a great practice. My preferred solution is a lightweight, safe abstraction like the aforementioned CBS. If that isn't desirable for some reason, base/offset/length as suggested by the article is reasonable, carrying the inconvenience of having to add base and offset frequently. Pointer/end is also reasonable, instead carrying the inconvenience of subtracting pointer from end frequently.<br /><br />* Reusing system error codes isn't a great practice, but it's an easy one. It is more important to have good, matching error codes and strings for errors you expect people to see in practice than for errors you do not. Malformed network input data isn't common (for binary formats), so seeing ENOBUFS or EINVAL being abused for that purpose doesn't really throw a red flag for me.<br /><br />* The line "the code failed to sanitize the original input, allowing the hacker to drive computation this deep in the system" concerns me, because it seems to suggest that input buffer overruns should be prevented by a well-formedness check separate from parsing. For complicated inputs I don't think that is a good practice; drift between the sanitization code and the parsing code won't be obvious to a reader and will almost always result in a CVE.Anonymoushttps://www.blogger.com/profile/14035082844686052433noreply@blogger.comtag:blogger.com,1999:blog-37798047.post-80975232814279396902018-10-28T14:45:31.634-04:002018-10-28T14:45:31.634-04:00This comment has been removed by the author.Hmmmmmhttps://www.blogger.com/profile/13130741669106836222noreply@blogger.comtag:blogger.com,1999:blog-37798047.post-71324008303773621932018-10-28T14:44:16.188-04:002018-10-28T14:44:16.188-04:00I agree with pretty much all of your points, excep...I agree with pretty much all of your points, except I don’t understand your point about ntohs being “not” that function. It is *exactly* that function (and should be used inside the unpack function you mentioned). Also, if you use sized integer types and packed structs, you get a lot of guarantees... not saying that’s a good idea for parsing of course.<br /><br />I still think the article is bad. It’s not in detail, it’s disorganized and badly written, it gives essentially no evidence for its assumptions and very little actionable feedback that’s actually worth taking into account. The comments have pointed to much better texts.Hmmmmmhttps://www.blogger.com/profile/13130741669106836222noreply@blogger.comtag:blogger.com,1999:blog-37798047.post-31154771434776120622018-10-28T05:30:22.301-04:002018-10-28T05:30:22.301-04:00Regarding Unknown's claim that the author over...Regarding Unknown's claim that the author overlooked what <i>assert_return</i> means:<br /><br />I thought this too but actually I think the author's point is that the intention of the code doesn't match how it is written.<br />First off if this function is called on input passed in from an external source then is EINVAL (Invalid Argument) really the right choice here? There's no way of distinguishing this error from "the programmer made a mistake and passed an invalid parameter" and "the data passed to the application is malformed". I think that the correct fix for this problem won't involve changing THIS piece of code, as it can stay as it is. The correct fix would be to add validation to the data BEFORE it is passed to this function. The point being that having a function containing the word "assert" being used to make run-time checks of external data is a misuse of the function. A differently named function (validate_return or something) which does the same thing could be used here and the intention would then at least match the code, but I think delaying the validation until a deeper function call is still unclear.<br /><br />Regarding Hmmmmm's claims regarding the byte order fallacy article and ntohs/htons:<br /><br />You are correct when you say that having (data[0] << 8) | data[1]; everywhere in your code would be bad code. You are correct when you say that this could be turned into a function. You are wrong when you say that this function is ntohs.<br />When you have a structure and you read/fread/recv data straight over it and make a ntohs pass over it, or alternatively (an even worse option) if you have a buffer of bytes and you cast it to a structure and make a ntohs pass over that, you are making numerous assumptions which C doesn't really let you make and which can only be guaranteed by specific implementations.<br />You are assuming that the data is correctly aligned, that the padding matches up, that you haven't got a bunch of trap representations and that your type sizes match up.<br />These are all assumptions which you don't need to make in your code, making these assumptions doesn't even make your code any faster if you have a modern compiler, making these assumptions just makes your code less clear.<br />And the worst thing is, when you use ntohs you don't even get to hide these assumptions, they're all still being made right there right in front of you in your very code.<br />A wonderful book written by Brian Kernighan and Rob Pike shows a way of abstracting the process of packing and unpacking binary data in C with a very simple and straightforward interface (although if you don't like this one, there are still other better options than ntohs):<br />unpack(buf, "csll", &c, &count, &dw1, &dw2);<br />This is a line taken from page 220 of the first (and I think only) edition of the book. That page also covers basically all I've said above in response to the other comment too. It validates the input BEFORE it is extracted, then uses an assert inside the function which wraps the extraction to verify that the programmer didn't make a mistake when calling the function.<br /><br />Aside from the author's blunder regarding the offset + x < length check, I don't see anything else particularly wrong with this article other than the aforementioned slightly sloppy use of the term "undefined" but at least calling everything which may be implementation defined, unspecified or undefined just "undefined" is not as big a mistake as for example incorrectly assuming that some undefined behaviour was actually implementation defined.<br />In the end, everyone makes mistakes, I may have made some in this very comment.Tomasz Kramkowskihttps://www.blogger.com/profile/00114974381716501947noreply@blogger.comtag:blogger.com,1999:blog-37798047.post-31084404955879701792018-10-28T04:36:44.429-04:002018-10-28T04:36:44.429-04:00Except the link you gave doesn’t say anything at a...Except the link you gave doesn’t say anything at all about htons or ntohs... In fact it implies that they are great. I completely agree with the arguments in that blog, but neither you nor Rob actually understands them. They key point from that blog is that the code should be about the relationship between the data stream’s byte order and the computer’s byte order, it should be about the data stream’s byte order only. And while ntohs is badly named (should be something more like big_endian_to_short), ntohs is exactly what we want. No more ifdefs or conditional code. I’ll put it this way: (data[0] << 8) + data[1] is the correct way extract a big endian number from a data stream (on any computer). But it’s complicated to type and understand, so it’s easy for the programmer to screw up and hard for a reviewer to immediately tell what it is. So the correct thing to do here is to create a helper function that always does this correctly. And hey! You’ve discovered ntohs.<br /><br />In both cases (manually typing it out vs. a function called ntohs), you have to remember to do it before using the underlying data, and only the latter is especially clear about what the intention is. Ntohs is *better* code.<br /><br />To be clear, the best way to do it is something like Chrome’s BigEndianReader: takes a buffer and it’s size and then has ReadU16, ReadU32, ReadBytes, etc. that return false if you overstep the bounds of the buffer. So error checking is simple, just if (reader.ReadU16(&dest)) { log error; }. Code doesn’t assume anything about the host system; no crazy ifdefs or conditional code.<br /><br />Btw, Rob’s buffer bounds check has an integer overflow, so it’s wrong (BigEndianReader would not), as mentioned elsewhere in the comments. And if assert_return is as another comment says, a macro that returns if the condition is false... then the only decent point this article makes is a weak one about error codes.<br /><br />The post is a bigger joke than the original code. The post is bad and you should feel bad.Hmmmmmhttps://www.blogger.com/profile/13130741669106836222noreply@blogger.comtag:blogger.com,1999:blog-37798047.post-3148827276165275482018-10-27T20:28:10.635-04:002018-10-27T20:28:10.635-04:00No, his points are sound and address the importanc...No, his points are sound and address the importance of principles and high-level clarity over low-level implementation.<br /><br />1) Clear semantics is crucial, for example in validating & reporting input errors. The quality of engineering & engineers can be seen in their error reporting..<br /><br />2) He's absolutely right that implementation-level byte manipulation is not a safe or robust approach to parse potentially unsafe external data. Never has been, never will be.<br /><br />I think the suggestion to throw the code away and start again is excessive -- but we all know that's just hyperbole. The part that does need to be thrown away is the input parsing/ external interface, and that needs to be reimplemented with a more robust parsing approach.<br /><br />Reliable checked parsing can be easily encapsulated in either C or C++.Thomas Whttps://www.blogger.com/profile/06287256406464617259noreply@blogger.comtag:blogger.com,1999:blog-37798047.post-74568653368658766422018-10-27T19:47:48.633-04:002018-10-27T19:47:48.633-04:00"offset + x < length" is not a safe f..."offset + x < length" is not a safe form for a bounds check, if x can be a value decoded from the network data. If x can have a value close to the maximum positive value for its type, then "offset + x" may overflow and compare less than length, even though x is not within bounds. Instead, compare x to length - offset, knowing that offset <= length is an invariant.Anonymoushttps://www.blogger.com/profile/14035082844686052433noreply@blogger.comtag:blogger.com,1999:blog-37798047.post-79870147858524958962018-10-27T19:31:15.867-04:002018-10-27T19:31:15.867-04:00This comment has been removed by the author.MyCatVerbshttps://www.blogger.com/profile/04865076076021684714noreply@blogger.comtag:blogger.com,1999:blog-37798047.post-9368386891852920532018-10-27T18:03:39.750-04:002018-10-27T18:03:39.750-04:00I think you've overlooked the fact that assert...I think you've overlooked the fact that assert_return isn't related to the C assert macro. assert_return is a systemd macro which *returns* when the given condition is false. It's just a shorthand for if (!cond) return.<br /><br />You also seem to enjoy the word "undefined" without really quantifying it. Undefined has a specific meaning in C. You're holding it wrong. What I think you mean to suggest is that overlaying application defined data structures on top of data received from outside the application is unwise as it isn't parsing the data, just mapping c data structures to binary data. That's fine. That act alone doesn't cause trouble, nor does it prevent you from doing future validation of that data.<br /><br />Your article raises some good points about systemd reinventing things which it need not, but you're quite off the mark in regards to your antiu-sggestions in how to fix the underlying problems. Anonymoushttps://www.blogger.com/profile/09621721604564130967noreply@blogger.comtag:blogger.com,1999:blog-37798047.post-23625953501443281352018-10-27T16:31:49.734-04:002018-10-27T16:31:49.734-04:00You don't really go into enough detail. Are by...You don't really go into enough detail. Are byte swapping and casting to internal structures inherently wrong or is it just when these are used on external data? I'm not aware that these operations in themselves can cause problems, just what you do with the resulting data.Tom Isaacsonhttps://www.blogger.com/profile/04896699906350584211noreply@blogger.com