1. 69
  1.  

  2. 15

    Several news stories have pointed out that this client was rewritten from scratch, as if that were the moral failing, instead of reusing existing code. That’s not the problem.

    Yes it is. You have working production code that has been in the field for decades, is reasonably small and modular, and is widely used. In order to replace it, you should have a compelling reason, not some “we’ll do it cleaner” arm wave.

    1. 9

      Something that seems curiously absent is recommending that any code that is accepting untrusted input and attempting to parse it should get lots of fuzzing. That’s one of the ideal uses cases for fuzzing, and coverage-guided fuzzers like afl-fuzz and libfuzzer are really good at searching for input that provokes unusual behavior in parsers.

      1. 15

        That’s not what you are supposed to use asserts for. This are very different things. It’s a coding horror that makes you shriek and run away when you see it. In my fact, that’s my Halloween costume this year, using asserts to validate network input.

        :)

        1. 21

          They aren’t using asserts to check network input. Their assert_return macro returns the second value if the condition fails. It’s not actually an assert, it’s a check. It’s just a poorly named macro.

        2. 5

          As pointed out in the hackernews comments, there was an earlier, less ranty report https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1795921

          1. 11

            As much as I like to dislike systemd, this article is just wrong.

            In the late 1990s and early 2000s, we learned that parsing input is a problem. The traditional ad hoc approach you were taught in school is wrong. It’s wrong from an abstract theoretical point of view. It’s wrong from the practical point of view, error prone and leading to spaghetti code.

            This is just simply untrue. Parser combinators and parser generators are good for some things, and not good for some other things. There’s a reason that essentially no production programming language parser uses a parser generator: their error handling is bad for humans.

            In a situation where you’re parsing network input or other machine-generated input, having only a simple ‘this is completely valid vs. this is invalid in some unspecified way’ distinction is probably fine. Invalid input? Disregard it. Valid input? Process it. But ‘parsing input’ is much broader than that, and ‘handwritten’ parsing is completely fine. It’s mandatory if you’re parsing user input IMO.

            The first thing you need to unlearn is byte-swapping.

            No, you don’t. Byte-swapping is fine. There’s nothing wrong with ntohl.

            Among the errors here is casting an internal structure over external data. From an abstract theory point of view, this is wrong. Internal structures are undefined. Just because you can sort of know the definition in C/C++ doesn’t change the fact that they are still undefined.

            No, they aren’t undefined. They’re quite well-defined. It’s completely reasonable to write that code, just as it is completely reasonable to memcpy stuff from buffers into internal data structures. You need to be aware of what is valid and what is not, what is well-defined behaviour and what is not. But just as the relatively common misconception that ‘pretty much anything that you can think of in terms of bytes is fine’ is not true, ‘it’s all undefined’ is not true either.

            For example, there is no conditional macro here that does one operation for a little-endian CPU, and another operation for a big-endian CPU – it does the same thing for both CPUs.

            Yes there absolutely is a conditional macro, it’s just in the compiler instead. On a big endian target the compiler does one thing, on a little endian target the compiler does another.

            The other thing that you shouldn’t do, even though C/C++ allows it, is pointer arithmetic. Again, it’s one of those epiphany things C programmers remember from their early days. It’s something they just couldn’t grasp until one day they did, and then they fell in love with it. Except it’s bad. The reason you struggled to grok it is because it’s stupid and you shouldn’t be using it. No other language has it, because it’s bad. … I mean, back in the day, it was a useful performance optimization.

            This is also just simply wrong. It’s not used because it’s a performance optimisation, and it never has been. It’s used because it’s standard C style to use pointer arithmetic, it leads to more easily understandable code (for experienced C programmers) and it’s much more concise than wading through &p[0] crap that just gets converted into pointer arithmetic anyway.

            In my code, you see a lot of constructs where it’s buf, offset, and length. The buf variable points to the start of the buffer and is never incremented. The length variable is the max length of the buffer and likewise never changes. It’s the offset variable that is incremented throughout.

            That’s fine, if you prefer doing it that way, but so is buf, offset being a pointer that is your offset plus your buf, and length. They’re equivalent, but the latter is much easier to read and understand for most C programmers.

            1. 6

              The traditional ad hoc approach you were taught in school is wrong. It’s wrong from an abstract theoretical point of view.

              Oooh - an abstract theoretical point of view. Sounds like math without actually using any math!

              1. 4

                The alternative to shotgun parsers is not necessarily parser combinators or parser generators. Shotgun parsers are parsers that intermingle parsing code and processing logic, rather than doing all the parsing up-front (http://langsec.org/brucon/ShotgunParsersBruCON.pdf). You can write a hand-written parser that does all the parsing work in one step.

                (The blog post doesn’t actually use the word “shotgun”, but I think that’s what’s meant).

                1. 5

                  No, you don’t. Byte-swapping is fine. There’s nothing wrong with ntohl.

                  It’s still a hacky thing. Just by looking at its signature:

                  uint32_t htonl(uint32_t hostlong);
                  

                  It converts “normal” integer to “network” integer, both having the same type. It’s somewhat weird. It’s not a function from integer to 4 bytes, it’s a function from integer to integer. Probably C defines two meanings for integers: a number, and a blob of bytes? I’m not even completely sure that treating integers as blobs is okay by measures of C’s specification.

                  Now, what we can do with networky integers? Can we put them into sockets/files right away? That involves implicit conversion from integer to 4 bytes. What endingness will be during conversion? And why casting structs over blobs of memory and then applying all these byte-swapping hackery is still an idiomatic way to parse protocols? It’s all very weird and frightening, especially when seen from perspective of “non-system” developer using “scripting” languages.

                  Why we still don’t have better system programming tools in 2018? It’s cruel to blame humans for making bugs while using such abstractions.

                  1. 6

                    htonl isn’t hacky because it’s a function from u32 to u32 any more than the (+1) function in Haskell is hacky because it’s a function from Int to Int. It’s a function, defined on 32-bit integers. Everything in C is a blob of bytes. Integers are blobs of bytes. Literally everything is (except functions I guess). There is no abstraction over this. ‘Integer’ and ‘bytes’ are the same thing in C and in the real world on actual processors.

                    And why casting structs over blobs of memory and then applying all these byte-swapping hackery is still an idiomatic way to parse protocols? It’s all very weird and frightening, especially when seen from perspective of “non-system” developer using “scripting” languages.

                    It’s not hackery. It’s the idiomatic way to parse protocols because it’s actually what is happening. You are taking bytes off a network and interpreting them as something. They aren’t integers on the network, they’re bytes. They aren’t integers in memory either, they’re just bytes. You interpret them as integers, but they’re just bytes.

                    C’s design is not built around being friendly to newbie programmers that have never written anything but Python. That’s not the goal, and it shouldn’t be the goal.

                    Why we still don’t have better system programming tools in 2018? It’s cruel to blame humans for making bugs while using such abstractions.

                    Because nobody has ever actually explained what ‘better system programming tools’ would actually be. Rust is not an improvement in this space, and that’s all I ever actually see proposed. Rust and pie-in-the-sky ‘formally verified languages with linear types’.

                    1. 5

                      Ideally, you do the conversion at the border; when you read the packet off the network, or just prior to sending the packet to the network. Sadly, this isn’t the case and most programmer will do the conversion “when needed” because of speed or some crap like that (DNS handling is notorious for this). I wrote a library to encode and decode DNS packet because of this and as a user, you get the blob of data from the network, call dns_decode() with the packet, and get back a fully decoded structure you can use.

                      For the conversion itself, there are a few ways of doing it. I’ll use DNS as an example (because I know it). The raw structure is:

                      struct dns_header
                      {
                        uint16_t id;
                        uint8_t  opcode;
                        uint8_t  rcode;
                        uint16_t qdcount;
                        uint16_t ancount;
                        uint16_t nscount;
                        uint16_t arcount;
                      };
                      
                      struct dns_packet
                      {
                        struct dns_header hdr;
                        unsigned char     rest[1460];
                      }
                      

                      I’m only concerning myself with the header portion, which is straightforward enough to show differences in parsing the data. The value of 1460 for the rest of the packet was chosen because of the MTU of Ethernet (1500) minus the overhead of IP, UDP and the DNS header. With that out of the way, one way to read this data is:

                      struct dns_packet  packet;
                      struct sockaddr_in remote;
                      socklen_t          remsize;
                      
                      remsize = sizeof(remote);
                      bytes   = recvfrom(sock,&packet,sizeof(packet),0,&remote,&remsize);
                      if (bytes < sizeof(struct dns_header)) handle_error();
                      
                      /* convert data from network byte order (big endian) to host byte order */
                      /* on systems that are already big endian, these become no-ops */
                      
                      packet.hdr.id      = ntohs(packet.hdr.id);
                      packet.hdr.qdcount = ntohs(packet.hdr.qdcount);
                      packet.hdr.ancount = ntohs(packet.hdr.ancount);
                      packet.hdr.nscount = ntohs(packet.hdr.nscount);
                      packet.hdr.arcount = ntohs(packet.hdr.arcount);
                      
                      /* work on data */
                      

                      Here we see how ntohs() (and similar functions) work. It converts a 16 bit quantity from network order (big endian) to host order. As noted, on systems that are already big endian, these do nothing and the compiler can optimize these out entirely. On little endian systems (like Intel), this translate to a few instructions—one to load the data, one to byteswap the data, and one to store it back. There aren’t issues with alignment since the 16-bit quantities are aligned on even addresses (and C guarantees proper alignment of structures). So this is very straightforward code.

                      The other option is actual byte manipulations:

                      struct dns_header   hdr;
                      uint8_t             packet[1472];
                      uint8_t            *ptr;
                      struct sockaddr_in  remote;
                      socklen_t           remsize;
                      
                      remsize = sizeof(remote);
                      bytes   = recvfrom(sock,&packet,sizeof(packet),0,&remote,&remsize);
                      if (bytes < sizeof(struct dns_header)) handle_error();
                      ptr = packet;
                      
                      hdr.id      = (packet[ 0] << 8) | packet[ 1];
                      hdr.opcode  =  packet[ 2];
                      hdr.rcode   =  packet[ 3];
                      hdr.qdcount = (packet[ 4] << 8) | packet[ 5];
                      hdr.ancount = (packet[ 6] << 8) | packet[ 7];
                      hdr.nscount = (packet[ 8] << 8) | packet[ 9];
                      hdr.arcount = (packet[10] << 8) | packet[11];
                      ptr += 12;
                      

                      There’s copying going on, and furthermore, you (the programmer) are having to track individual offsets manually—I’d rather leave that up to the compiler to do. You could also do:

                      hdr.id      = (*ptr++ << 8) | *ptr++;
                      hdr.opcode  =  *ptr++;
                      hdr.rcode   =  *ptr++;
                      hdr.qdcount = (*ptr++ << 8) | *ptr++;
                      hdr.ancount = (*ptr++ << 8) | *ptr++;
                      hdr.nscount = (*ptr++ << 8) | *ptr++;
                      ndr.arcount = (*ptr++ << 8) | *ptr++;
                      

                      No tracking of offsets required, but it’s a lot of pointer manipulation that scares programmers.

                      And that’s pretty much the ways you parse networking data. As I said, ideally, this is handled right at the border, preferably using a library to handle the details, and said library is easy to use.

                      1. 2

                        It’s somewhat weird. It’s not a function from integer to 4 bytes, it’s a function from integer to integer.

                        No, it’s a function from 4-bytes wide integers to 4-bytes wide integers. That’s right there in the prototype: “uint32_t”. So there you have it, your final representation of data: 4-bytes, with the proper bits set.

                        And why casting structs over blobs of memory and then applying all these byte-swapping hackery is still an idiomatic way to parse protocols? It’s all very weird and frightening, especially when seen from perspective of “non-system” developer using “scripting” languages.

                        It will always be so. You might get better and better abstractions describing serialization, access to external buffers, data validation. You will always bear the cost of doing a copy. That won’t, ever, disappear, even with the higher abstraction of a non-system programming language. So you will always have system developpers going back and saying “oh by the way, why are we copying data here? I see a glaring and obvious optimization, let’s do zerocopy!”.

                        Systemd has abstraction leakage left and right, due to its pervasive function, but networking will always involve an interface between systems and non-systems developpers. When writing routing code in kernel or userland, you can be certain that the structures won’t be copied for safe validation. When your host is the end-point, the same validation will be used usually, meaning that the zerocopy method will always exist and always be preferred.

                    2. 3

                      anybody on the local network can send you a packet and take control of your computer

                      Unrelated to parsing, but does it actually enable DHCPv6 by default? Why? Because Windows does that for some reason? Every non-Windows device that ever connected to my home network only used SLAAC.

                      Back to parsing… is there a good byte/bit oriented / “binary format friendly” parser generator for C? Like Rust’s amazing nom?

                      1. 5

                        Back to parsing… is there a good byte/bit oriented / “binary format friendly” parser generator for C?

                        You’ll need C++, but: Kaitai Struct (https://kaitai.io)

                        1. 4

                          Yes. It’s in the original bug report. systems enables DHCPv6 if it sees the right broadcast packets. Even better, if your exploit crashes the network daemon, systems will restart it for you to try again.

                          1. 0

                            Why not use Rust? There is no reason for DHCP client to be written in C.

                            Edit: thanks for replies.

                            1. 10

                              Probably because Rust wasn’t even stable yet when they started this functionality, let alone systemd?

                              Of course, it’s not like there were no other memory-safe languages ;).

                              1. 7

                                portability?

                                1. 7

                                  There is no reason for DHCP client to be written in C.

                                  Hold up, there are plenty of reasons that embedded systems are written in C. Just because a tool that has complete memory safety guarantees exists doesn’t necessarily mean that it’s a fit for every system.

                                  I agree, network parsing code should probably be written in safe languages, but C is a tool as much as Rust. It’s all about using the right tools for your job.

                                  1. 5

                                    Why was C and not rust the right tool for this job? Memory-safety bugs like the kind this post is about are exactly the thing that C makes it easy to do and Rust makes it very hard to do.

                                    1. 10

                                      I agree, network parsing code should probably be written in safe languages

                                      Read the whole comment, please. This is a decision that depends on the system in question, desired maturity of tools, and who’s going to work on it in the future, at a minimum. Hell, Java might solve this problem (specifically, fulfilling the same safety guarantees) if your system is running a JVM or Java Card. People treat Rust like it’s the only sane choice other than C. It’s really not, and if you don’t have Rust or a nicer tool available to turn to, what else are you going to use other than C?

                                  2. 4

                                    This kind of response would be useful if accompanied even by code snippets showing how you could do the same thing, better, in Rust.

                                2. 4

                                  Good analysis, but, man… harsh.

                                  1. 7

                                    People pointing out security-relevant flaws in code shouldn’t feel the need to avoid being harsh in their criticism.

                                    1. 2

                                      We have 20 years of experience with what goes wrong in network code. We have academic disciplines like langsec that study the problem. We have lots of good examples of network parsing done well. There is really no excuse for code that is of this low quality.

                                      So this wonderful 20 years of lessons learnt is documented…. where exactly? We have gigabytes of network parsing code…. which, precisely, are the universally acclaimed examples of it “done well”?

                                      Or is that another one of the lessons…. (that we really should document things properly and have a commonly agreed upon source of best practice design guidelines) …that we have learnt, but never put into practice.

                                      shouldn’t feel the need to avoid being harsh in their criticism.

                                      If and only if their harshness is matched by their contribution to creating useful, usable and up to date and universally agreed upon best practice guidelines and tools.

                                      I note the author doesn’t link to any… contributing to his appearance as a moaning minnie instead of an expert.

                                      He might be one, but I can’t tell from that article.

                                      Otherwise they are just making people think… “Gee, security is hard and full of old grouches, so any time you touch it you get kicked in the teeth, I’m going to work on other code…”

                                      Leaving security sensitive code unmaintained and under scrutinized. (Like SSL prior to heartbleed)