Systemd is bad parsing

https://blog.erratasec.com/2018/10/systemd-is-bad-parsing-and-should-feel.html

by fanf2 • 8 years ago

276 183 8 years ago

blog.erratasec.com

The article isn't really about the bug but about the (lack of) quality in the code. Spoiler alert: He doesn't like it.

"This code has no redeeming features. It must be thrown away and rewritten yet again. This time by an experienced programmer who know what error codes mean, how to use asserts properly, and most of all, who has experience at network programming."

tinus_hn• 8 years ago

If the rest of the code looks anywhere near similar to the examples he shows he’s right, this is terrible code and it should never have been allowed in a serious project in this day and age.

pbhjpbhj2• 8 years ago

Perhaps it's a choice between this and nothing?

deialtrous3• 8 years ago

It seems like this day and age is precisely when this sort of code is normal. We had a brief window of hopeful optimism in the mid to late 90s and even into the early 2000s, but since then it seems like code quality in general has declined tremendously.

a3_nm• 8 years ago

If like me you are looking for the CVE and not for commentary about the affected code, here it is: https://nvd.nist.gov/vuln/detail/CVE-2018-15688

FWIW, from https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=912008 it looks like the vulnerable code is not enabled by default on Debian.

JdeBP• 8 years ago

The rather less strongly titled "Systemd-networkd memory corruption" pointing to the much more detailed Ubuntu bug has also been on Hacker News for a day.

* https://news.ycombinator.com/item?id=18303613

polotics• 8 years ago

So it's true what some-of-you'all been saying all along about systemd!

;-D

One question: any smell of intentional obfuscation for this vulnerability, or is it impossible to tell?

blakesterz• 8 years ago

> One question: any smell of intentional obfuscation for this vulnerability, or is it impossible to tell?

I always think that anytime there's an interesting bug in ANYTHING. I'm not an expert, but I would think it would be nearly impossible to tell the difference between "someone made a mistake" and "someone made this look like a mistake". I would think any decent programmer with malicious intent could do that anytime they wanted.

dsr_• 8 years ago

Here's a thing to ponder:

If this were not buried in systemd, but was part of a standalone dhcpd package, would it have been done better or caught earlier?

One of the major criticisms of systemd is that it wants to take over all system management functions. In order to do that it needs to offer at least minimally functional versions of all the services it pushes out. It should not be news to anyone that trying to do everything instead of integrating with existing projects leads to sloppiness.

This incident is evidence suggesting that other serious issues are lurking in systemd as a result of the actions naturally arising from their policy.

zzzcpan3• 8 years ago

Rewriting everything is fine, as long as it's not in C or similar insecure by design programming language, it doesn't invent DSLs on top of INI/YAML/JSON files, uses proper architecture with clearly defined boundaries and protocols, allowing custom application-specific management instead of forcing everyone into author's narrow worldview full of incorrect assumptions. So as long as you are not doing what systemd does.

Centralized management/monitoring in general sucks. People don't seem to understand the problem and keep reinventing new centralized architectures suitable for their own special cases, instead of addressing that on architecture level.

toast0• 8 years ago

I'm sure (but don't recall) Redhat's pump and ISC's DHCPCD have had CVEs; we all know BINDs resolver and ntpd had CVEs.

If one is going to rewrite all of these things, I would expect one to learn from the history and not make the same mistakes over again.

quickben• 8 years ago

This approach increases the attack surface because it is included in the most popular distros by default.

LukeShu7• 8 years ago

The problem was in systemd-networkd, not systemd.

One of the more popular DHCP clients is 'dhcpcd', which was created and is maintained by Roy Marples, the creator of OpenRC. It's fine when someone wants to work on DHCP clients and init systems, unless that init system is systemd?

(And for the record, systemd integrates wonderfully with dhcpcd.)

flohofwoe• 8 years ago

I agree with most of the critique, but I don't quite get the part about the use of assert.

The code in question quite clearly doesn't use C's standard assert macro, but a custom macro called assert_return (which I guess checks for a condition, and returns from the function instead of aborting the program).

So basically:

    if (!condition) {
        return err;
    }

How else would one check for invalid input to the function?

viraptor2• 8 years ago

I may have a different understanding than the author, but for me the difference in the name means a lot. The fragment you wrote is clear and simple. But if I see assert_..., then I immediately have questions like: Where's the message printed and does it have enough context for debugging at that point? Is this affected by NDEBUG? Will it run in release builds? Does assert_se() kill the whole application?

Assert already has a meaning and using it in this context is not great.

mrami• 8 years ago

Yes, exactly. Coming from C++/Java, I immediately though, "wait, this is turned off in production?"

cf. http://www.cplusplus.com/reference/cassert/assert/

XorNot• 8 years ago

I can see the argument to be made about overloading the word "assert" in the language though.

i.e. "assert" meaning "check something in this code is algorithmically correct"

Whereas when you're checking user input, use a very different term - i.e. validate or something.

So a fix would be to rename the macro to be obvious about it's usage - "validate_return" or "valid_or_return" or something.

To be fair I can see the problem because I do tend to write code comments saying "assert everything makes sense" when checking user input sometimes and "assert" itself doesn't properly convey the outcome if it fails - i.e. "abort_if" or "crash_if" would be a much more descriptive term.

Flow• 8 years ago

Words matter.

I sometimes use a bunch of Ensure*() methods in C# if I need to validate input data. They throw exceptions if condition fails. If I only do this once I don't create those methods.

One drawback is the stacktrace has these Ensure names in it.

edoceo• 8 years ago

I used assert_* for code bugs and then verify_* for the user checking ones, my projects would have a verify.h,c file that was pretty short/sweet/easy for the next dev to grok.

At one point and older coder who I looked up to even complemented and adopted this pattern. 1998 was a good year.

tetha2• 8 years ago

In my opinion diluting existing, well-established terms is extremely dangerous and harmful for a project. If you're diluting established terms - like assert - you're kinda forcing readers to stop assuming anything about your names.

If assert isn't assert, and if return values are also abused, why can I assume get is get, sort is sort and so on? Sort might check inputs, get might delete the customer account, who knows at this point. And that makes anything but a tiny code base pretty much unmanageable, or at least very very hard to learn.

bjourne• 8 years ago

I don't know the details of DHCP, but there is a difference between values supplied by the programmer and those supplied by the user. According to the author, checking if a pointer is not-null is checking a programmer error (EINVAL) so it makes sense to use asserts. But the optval string in the example is supplied by the user so it is insane to use asserts to check for null-termination.

Asserts should only be used to signal "this code is broken" but that is not how they are used here.

A long while ago the GTK+ library used to suffer from a similar affliction. Mainstay GTK+ programs such as GEdit and Totem would print lots of warnings on the console. Since the GTK+ developers used the same facility (g_warning) for reporting both coding errors and for user errors like "file not found," debugging code was hard. Was the warning "supposed to be there" or was it a bug? GTK+ programs are much cleaner these days even if some programs still fails to make the distinction between user and programmer errors.

corndoge• 8 years ago

Probably returns a value if the assert works, and aborts otherwise.

I would go so far as to say that using asserts in input validation is a good thing, in very limited quantities. I use them in places where the input should have been validated so long ago, so many functions back, that if something isn't as I expect here, I seriously fucked up somewhere else and the whole system has a good likelihood of being compromised by something unrelated to the condition I'm asserting. In that scenario I'd rather log and abort.

int_19h10• 8 years ago

In this case, though, you're not using asserts to validate. You're using asserts to capture the post-validation guarantees in your code. Basically, you assert that your validation code did the right thing and rejected invalid input through appropriate mechanisms - not that the input was valid.

FeepingCreature2• 8 years ago

In D, we call the two functions "assert" and "enforce". The point is that assert is the wrong term; to say "assert(condition)" says "I assert that condition should hold here". If there's an input event that causes condition to be false then your assert is not just false but wrong, since you assert something that is not necessarily true.

filam2• 8 years ago

I have to admit I stopped reading after the combined section on byte swapping and casting external data to a internal structure. Perhaps the author of the code should have used ntohs() instead of be16toh(), but the effect is the same? There aren't any conditionals in ntoh...

https://commandcenter.blogspot.com/2012/04/byte-order-fallac...

And will a compiler make use of native byte swapping instructions with the code proposed by the author?

detaro2• 8 years ago

The blogpost you link seems to basically recommend the same as the author, except they only talk about the conversion step and not about where it should take place too?

filam2• 8 years ago

That post by Rob Pike is arguing that you should not need to check the endianness of your system when swapping. Not that you should never byte swap. Systems and protocols have different endianness, which needs to be handled by programs where endianness has not been abstracted away.

ben0x5399• 8 years ago

The argument against byte swapping suggests that you shouldn't take a value of a numeric type and apply some (maybe no-op) permutation to it to produce another value of the numeric type. Instead, you should only ever think about byte order at the step where you have an array (or stream, or...) of individual bytes and are converting it to a value of a numeric type (larger than uint8).

I think this is also the main point of the post you linked: the "bad" example has a numeric value and then performs byte swapping on it, the "good" examples only ever construct "correctly ordered" numeric values.

If you can possibly get to a place where you have a uint16 but the value is "in the wrong byte order", you've done something wrong already.

Admitting the possibility of values with the wrong byte order into your system casts every single value into doubt. Of course it's possible to resolve that doubt on a case-by-case basis by carefully identifying the right places to insert ntohs()/htons() calls, but to me that sounds like creating extra chores for yourself while working against the language.

meowface• 8 years ago

This philosophy is similar to Python 3's hard separation of bytes and strings. Bytes are raw data you get off the wire and which are encoded in a certain way. Once decoded into a string, you have a clean, consistent internal representation that behaves as you expect a string would. If you're juggling (or even thinking about) byte encodings past the parsing stage and before the output stage, you've done something wrong.

boardwaalk• 8 years ago

The issue, I think, is when the swapping is done. If you cast raw bytes into a structure you can forget to swap. If you read fields individually with a helper functions that swap when needed, you can't.

Whether there's a endianness conditional anywhere there or not is a bit of pedantry, I think.

fshaun• 8 years ago

Yes, agreed. It is a step backward to manipulate and recombine bytes. "Extracting an integer from a packet, then changing it's internal format" is exactly what is being done, and thinking at that abstraction level is beter than playing with individual bytes. And suppose you hoist those operations into a separate function instead of doing it for every field? Well, congratulations on reimplementing ntohX. If the boundary between internal and external data is not clear, that's a bigger issue that could cause incorrect or unnecessary data transformations, whether we're talking about byte order, string sanitizing, unit conversions, etc.

int_19h10• 8 years ago

The problem is that the higher-level abstraction is broken. Specifically, the abstraction provided here is "extract an integer of unspecified size and endianness from a byte stream" (note that the size of short isn't guaranteed to be anything specifically, other than 2 <= sizeof(short) <= sizeof(int)). And ntohs is another abstraction which amounts to, "make the endianness correct for the current system, assuming that it was originally big-endian". Not that it doesn't work - it's just not well-designed, and too easy to use incorrectly.

A proper abstraction is the one where you specify both the size and the endianness at the point where you extract the value - i.e. where you just call a function like read_int16_be(byte_stream).

FeepingCreature2• 8 years ago

Who the actual fuck cares about native byte swapping instructions. We're talking about DHCP code, there is no computer in the universe on which parsing of DHCP packets is even noticeable as a blip on the performance chart.

koala_man• 8 years ago

We're talking about safe ways to write parsers for byte streams. The original article used systemd's DHCP bug as a motivating example, but none of this is specific to DHCP.

stinos2• 8 years ago

So this is a whole rant, and it's pretty much spot on probably, but the reader is left with a huge elephant of a question in the room: what is the proper way to do it, then? It has samples all over the place of what's bad and telling thhose should be rewritten. Why not show some C/C++ code samples of the proper way to do it, then?

asdfasgasdgasdg2• 8 years ago

Why not show some C/C++ code samples of the proper way to do it, then?

He provides constructive examples for all of the items that he criticizes.

- Instead of pointer arithmetic, base_ptr, offset, len.

- Instead of casting to internal structs, parsing into internal structs.

- Instead of network byte order, buf[offset] + buf[offset+1] * 256 .

- Instead of the wrong error codes, the right ones.

- Instead of random asserts over internal data structures, validation at parse time.

clarry2• 8 years ago

- Instead of network byte order, buf[offset] + buf[offset+1] * 256 .

And you got it wrong. Shows that there is no silver bullet.

paavoova2• 8 years ago

I wonder why the author chooses to write buf[offset]*256 + buf[offset+1] instead of the more common method to load bytes using bitwise arithmetic, e.g. b[k]<<8 | b[k+1]. This is far clearer instead of using potentially magic numbers. Does Javascript not have bitwise arithmetic? Why would you write C code with Javascript in mind?

camgunz4• 8 years ago

The main issue to me is that conversion, parsing, and validation are scattered throughout the code when they should be in separate, local, sequential steps. You should never be in a situation where you're halfway through allocating memory for an internal structure when suddenly you realize your input is malformed, for three reasons:

- In order to make a good decision about what to do next, you need a lot of context: what you need to clean up, what any calling function expects you to do, any internal state you need to update, etc. You either end up passing a lot of extraneous context info around, or you make everything global.

- Alternatively you bail with a status code and mutter "yolo" to yourself, or you throw an assert -- but crashing a system daemon with input is the essence of a DoS, so only the truly ignorant or lazy resort to it.

- Building a reasonable framework is hard when all your concerns are entwined like this. If at some point you decide asserts on input are bad, it's very hard to change that convention in this code. Essentially all your function signatures and your entire call graph change. There's an extraordinary amount of technical debt here; just look at all the places Lennart had to change the size/offset stuff. The logic for this is scattered all over the place.

People have focused on some of the tactical issues -- asserts on input, byte order conversion, etc. -- but the real issue is that when you write code in this style those things don't jump out at you because you have to do them everywhere. If your code had a better separation of concerns, as soon as you saw 'offsetof' in a parsing function you'd immediately see a red flag. It's so much easier to think in these specific contexts, where you're assured previous steps are complete, rather than in every single function having to get the entire conversion/validation/parsing flow correct every single time, keeping the entire thing in your head always.

romed• 8 years ago

I'm kinda bothered by the "C/C++" indistinction of your request. This code would look very, very different in C++.

stinos2• 8 years ago

Sorry about that, I just took over what the author used, even though I know there's actually no such thing as C/C++ and use of the term should die :[

xtrapolate3• 8 years ago

"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."

Honestly, this is a pointless rant. "Do this, but don't do that (because that's stupid)".

"No other language has it, because it's bad"

The reason you "don't have pointer arithmetic in other languages" is - it's been abstracted away from you, for your convenience, so that you can just concentrate on your business logic and not have to worry about the rest. You may not deal with pointer arithmetic, but the people developing your JVM/CPython/V8/Chakra sure do. Author is simply ranting about the possibility of abusing/misusing the mechanisms offered by C/C++.

zwkrt2• 8 years ago

Programming as a profession is strange because we each do such different things but all tools are available to us all the time. Imagine if this were true in the physical world; you would read arcticles written by accountants like "Dynamite Considered Harmful" with rebuttals titled "Why Dynamite Matters" by construction engineers...

sam0x17• 8 years ago

This is fantastically imaginative. I love it.

zzzeek• 8 years ago

this is the most genius thing I've ever read on hacker news.

xtrapolate3• 8 years ago

It's almost as if things aren't really black or white, there's usually plenty of grey in-between. Things like use-cases, scenarios, trade-offs, intricacies. I find it awkward that a security shop would publish an article using that language - I would personally be embarrassed.

taneq• 8 years ago

You see this all the time here on HN. "Interpreted languages are useless" coming from the systems and embedded people. "You can't call yourself a developer if you're not an expert in [FOTM web framework]" from the web devs. "Mutability is evil and reassigning variables is a sign of incompetence" from the functional programming sect.

thwd2• 8 years ago

Pointer arithmetic is abstracted away for _safety_, not convenience. You can read about in e.g. Go's FAQ.

https://stackoverflow.com/questions/32700999/pointer-arithme...

mixmastamyk• 8 years ago

Or perhaps safety and convenience.

mikeash2• 8 years ago

Pointer arithmetic can be perfectly safe. C doesn’t implement it safely but that doesn’t mean other languages couldn’t.

vmchale• 8 years ago

Funnily enough, ATS has pointer arithmetic and it can be safe if you declare the appropriate types.

It seems kind of nuts to say array indexing isn't just a fancier version of pointer arithmetic.

xtrapolate3• 8 years ago

It seems kind of nuts to say array indexing isn't just a fancier version of pointer arithmetic.

I agree.

zzzcpan3• 8 years ago

Array indexing is __bounds checked__ pointer arithmetic.

But not in C.

thwd2• 8 years ago

Array-indexing in C and C++ _is_ pointer arithmetic! It's syntactic sugar. Array-indexing in safe languages is bound checked iff the compiler can't prove the index value to always be in-bounds.

ben0x5399• 8 years ago

Do you just take issue with the specific claim that no language should have pointer arithmetic, or do you disagree with the more general thrust of the post?

Sure, it's ranty and, like, maybe closer to op-ed than computer science publication, but I think even among the rarefied breed of people developing your JVM/CPython/V8/Chakra, you could easily find approval for the notion that pointer arithmetic is bad(*) and should be avoided as much as possible.

gaius• 8 years ago

Additionally, I don’t think it’s reasonable to extrapolate from the mess that is systemd to all C/C++ code. Some fantastically reliable systems have been developed with them.

eeZah7Ux• 8 years ago

In the late 1990s and early 2000s, we learned that parsing input is a problem.

What? It was known well in advance.

ben0x5399• 8 years ago

I suspect in advance of the 1990s it was a problem to people who specifically cared about such things, and would read or write papers on the topic. In the 1990s it started to become a problem for a lot of people who really only cared about putting their programs on a widely-accessible network. ;)

jeremyjh• 8 years ago

I don't think many people were aware of the security implications of these practices before the late 90s.

chris_wot• 8 years ago

As an aside, I find it a little ironic that I found it hard to parse the headline "Systemd is bad parsing". No, Systemd is not bad parsing, Systemd is parsing badly.

philipwhiuk• 8 years ago

I assume it should have read "Systemd is bad at parsing"

hyperpape• 8 years ago

The original is “Systemd is bad parsing and should feel bad”, which sounds a little weird, but it’s a meme, so there are lower standards.

I guess someone felt like it shouldn’t be a meme headline on HN, but the result is nonsense.

exabrial• 8 years ago

I typically disable ipv6 unless I specifically plan to offer/use it as a public service. Just more things to keep an eye on for no benefit to the actual business.

dvfjsdhgfv4• 8 years ago

Just like me and many other sysadmins. What amazes me is that the people who designed IPv6 still don't get it and seem surprised why it hasn't took over the Internet yet.

ben0x5399• 8 years ago

A lot of mobile phones access the internet via IPv6 by default, and IPv4-only sites that want to reach them require non-trivial compatibility infrastructure like carrier-grade NAT or more complicated shenanigans. It seems like some sites are starting to identify these shims as a huge stumbling block in their continuous work to improve latency/quality-of-service.

I think Facebook in particular made a bunch of noise in that direction and celebrates how prioritizing IPv6 has let them make huge improvements in load times etc, but at this point you're as likely to find good information about that on Google as I am so I'll spare you any links.

So while IPv6 might not have taken over the Internet in any particularly observable manner so far, I think it's fair to say it has huge momentum at this point and has buy-in way beyond some IETF nerds and protocol fetishists.

protomyth4• 8 years ago

I honestly think the creators of IPv6 thought their customer was all the folks buying devices and forgot their real customers were all the sysadmins. They totally failed at making it an easy and rewarding jump from IPv4 to IPv6. Enthusiastic sysadmins would have got the conversion done years ago.

noinsight• 8 years ago

why it hasn't took over the Internet yet.

Yeah - only 21% of all traffic handled by Google is over IPv6...

https://www.google.com/intl/en/ipv6/statistics.html

ambrop76• 8 years ago

Yes the code is bad but does not need to be completely rewritten. It should interpret the data as an array uint8_t (no casting to structure/integer pointers!), use simple helper functions to read out (u)int(8/16/32) values (there was a post on HN about this recently, https://commandcenter.blogspot.com/2012/04/byte-order-fallac...), and be careful to check things like sizes.

The code is also wrong because of strict aliasing. This is a real problem, your program can in fact exhibit undefined behavior because of this (it happened to me).

Some time ago I wrote some code to make these things simpler in C++. It allows you to define "structures" (which however are not real structs, it's an abstraction) and then directly access an arbitrary buffer through setters/getters. The code is here with documentation: https://github.com/ambrop72/aipstack/blob/master/src/aipstac... . In fact this is part of my own TCP/IP stack and it includes DHCP code (here are my DHCP structure definitions: https://github.com/ambrop72/aipstack/blob/master/src/aipstac...).

ben0x5399• 8 years ago

I don't think I disagree, but could you articulate how strict aliasing makes this code wrong? Maybe I'm not looking at the right code, but it's not immediately obvious from the post to me.

ambrop76• 8 years ago

Example explanation: https://stackoverflow.com/a/99010/1020667 (feel free to look for better ones).

jstimpfle3• 8 years ago

As an array of uint8_t? I'm pretty sure that's wrong, and that it should be char instead. Usual OS network APIs are char based, and the fact that the network is technically about "octets" shouldn't matter behind those APIs. Or is my thinking wrong?

ambrop76• 8 years ago

On virtually all modern computers chars are 8-bit and casting the address to uint8_t* is perfectly fine (due to specific provisions in the standards that I am too lazy to find). And you need unsigned so that expressions like x + (y << 16) do the right thing. With that in mind, I use uint8_t as opposed to unsigned char to emphasize that it is 8-bit and also for less typing :)

alxlaz4• 8 years ago

uint8_t is guaranteed to always be an octet, and stdint types (uint8_t, int32_t, uint32_t etc.) are also the recommended types to use when you want to make the size and signedness of the data clear. An array of uint8_t is definitely correct, and it's also the right thing to do in 2018. An array of char is something you should only use to store human-readable character in array form.

Many OS network APIs are "char based" because the C standard requires that sizeof char be exactly one byte, so it used to be a convenient shorthand, but that's not the case anymore.

jstimpfle3• 8 years ago

Downvoters, my question was wether int8_t* and char* are interchangeable. I can't see why that's a bad question to ask. Given "char* buf", there might well be a difference between "(uint8_t) buf[1]" and "((uint8_t* )buf)[1]".

Now, upon further researching it looks like uint8_t always has size 1 (as does char), but I'm still not positive that the second expression is technically valid.

amluto• 8 years ago

There's a different consideration: as far as I know, it's an open question whether uint8_t* and int8_t* have the same special aliasing rules that apply to char* and unsigned char. A strict reading suggests that they do not. So there may be cases where using uint8_t is UB but using unsigned char* is not.

I think that all compilers consider this to be a bit of defect in the relevant standards, and they'll compile your code the way you expect.

microcolonel2• 8 years ago

The byte order of the computer doesn't matter much at all except to compiler writers and the like

This statement is to generic to mean anything helpful.

I think the point our friend Rob Pike is making is that you could write general purpose code which accomplishes the task, and on a sufficiently advanced compiler, the swap may be eliminated where applicable; but when the point is made this way, it surely just pisses off people who know better.

C programmers are not (quite) children, you don't need to eradicate nuance to keep people reading.

camgunz4• 8 years ago

Yeah casting to struct pointers is a bright red flag. It also rarely saves you anything since packed structs are slow, and you still have to validate the contents.

You're right, these streams should be interpreted as uint8_t to avoid undefined behavior when shifting, masking, and ORing, and anyone doing this should tuck it behind some abstraction layer. It should never bleed into parsing.

webkike• 8 years ago

My second year at google, I fixed undefined behavior that essentially appeared in every google binary because a core library did this without care for the alignment of the memory in question. Thank god with PODs you can just memcpy

Gibbon12• 8 years ago

It also rarely saves you anything since packed structs are slow

Packed structs are slow on 1990's RISC processors. Packed structs are not slow on 2010 era super scalar processors.

moefh2• 8 years ago

Interestingly (and pedantically), the code from the otherwise excellent "Byte order fallacy" article technically contains undefined behavior in C on machines where int is 32 bits wide (which is almost everything nowadays).

Consider this example of extracting a 32-bit int encoded as little endian:

    #include <stdio.h>

    int main(void)
    {
      unsigned char data[4] = { 0, 0, 0, 128 };
      unsigned int i;

      // the line of code from the article:
      i = (data[0]<<0) | (data[1]<<8) | (data[2]<<16) | (data[3]<<24);
    
      printf("%x\n", i);
    }

Compiling this with either gcc or clang using "-fsanitize=undefined" triggers this runtime error:

    test.c:9:61: runtime error: left shift of 128 by 24 places cannot be represented in type 'int'

That happens because, even though the bytes are unsigned as the article requires (using uint8_t instead of unsigned char has the same problem), they are promoted to (signed) int before the left shift as part of automatic integer promotion. When the left shift by 24 happens, it puts an 1 into the sign bit of the int, which is undefined behavior. Later the int is converted to unsigned (using implementation-defined behavior, which luckily is sane on all popular compilers), but by then it's already too late.

The solution is to change the code slightly:

   i = (data[0]<<0) | (data[1]<<8) | (data[2]<<16) | ((unsigned)data[3]<<24);

This prevents the promotion from unsigned char (or uint8_t) to int by explicitly casting it to an unsigned int, which can then be safely shifted left by 24.

ambrop76• 8 years ago

You are correct. I didn't want to go into these details to not confuse people even more :)

But I think it is better to cast all the bytes to the final type (e.g. uint32) before shifting, not unknown types like unsigned int.

d21d3q• 8 years ago

Can't reproduce: https://onlinegdb.com/HyighU72Q

zwkrt2• 8 years ago

Other people's gripes aside, I enjoy that the authors approach to avoiding out-of-bounds errors almost exactly mirrors Java's nio Buffer interface.

microcolonel2• 8 years ago

Some of these complaints are more "waah, I don't like C and what people use pointers for!", which is a shame, because some are pretty reasonable concerns.

systemd works, that's why people use it. If it needs improving, then improve it, or file these. Somebody will probably agree and at least look into them, rather than a gaggle of strangers pointing and laughing.

bvinc2• 8 years ago

He talks like the way he writes code has been established as best practice for decades, and if he can just write enough blog posts and convince enough people, maybe we can solve this problem once and for all.

I think the only way to have high assurances is to program under a system that doesn't have these classes of errors. I don't care if it's a c static checker or a new language or what.

Unfortunately the long term solution to these problems in system programming seems to be rust. But it's going to take decades for it to be established and I'm going to look like a language hipster member of the rust evangelism strike force every time I bring it up.

But seriously, it's been decades and decades and we're still writing buffer overflows. What is the long term final nail in the coffin for buffer overflows?

someguydave2• 8 years ago

The long term solution is to burn down x86 and C and start over with sane hardware, which has bounds checking in hardware.

archgoon• 8 years ago

I'm curious; what does your memory hardware and cpu instruction set look like in this scenario? What's the interface between the cpu and memory? (rough idea, not asking for a whitepaper here :) )

chmod775• 8 years ago

bounds checking in hardware.

What. That's a horrible idea. Not only would that require your hardware to understand abstractions several layers above what it actually operates on, it would also mean it'd have to check bounds of - for example - an array on every instruction accessing it. That is something compilers can do at more strategic places.

Please keep that complexity out of my CPUs, they're buggy enough already. At least with software we can fix issues later, which isn't a given with hardware.

sly010• 8 years ago

which has bounds checking in hardware.

x86 has bounds checking in hardware. it's called a page fault.

[edit: formatting]

sseth• 8 years ago

I worked (a long time ago) on Burroughs mainframes which did bounds checking of arrays (and in fact even type checking), because the whole machine was build around ALGOL.

I am not sure whether the experience from these machines was negative. It may be that when you move beyond one-dimensional arrays, bounds checking becomes more complicated.

Perhaps performance suffers too. Languages which do bounds checking often have intelligent compilers which can skip the bounds check in a variety of conditions. So even if the hardware is faster, it may be doing unnecessary checks. See for example : https://www.ardanlabs.com/blog/2018/04/bounds-check-eliminat...

andrewflnr2• 8 years ago

Rust isn't the long term solution either. I like Rust, but in terms of fully provably secure systems, it's a stopgap until the usability of fully verified approaches like ATS, F-star et Al reach a point where they can be economical.

Also what the other person said about burning down x86 and starting over.

bvinc2• 8 years ago

Those are all things that could fix the problem. But let me ask this another way. Imagine that you travel into the future a decade or three and a computer scientist tells you "pretty much no one gets buffer overflows anymore because of X".

It seems unlikely to me that the answer is going to be ATS, or "we burned down x86".

MisterTea• 8 years ago

But it's going to take decades for it to be established...

I wouldn't be so pessimistic. Software is becoming increasingly complex and many people are starting to see that it is starting to get a little ridiculous. All we need is a tipping point in the form of a well designed kernel that works well. It doesn't need any hype, no HN announcements, nor major corporate backing. It just has to work and work well. That's how Linux got started. It filled a vacuum. We still have a vacuum in the form of crusty OS designs patched over with layers of code miles deep. That vacuum is the need for a new clean system for the networked distributed resource internet age.

What we need are more passionate hackers who work on this for the pure enjoyment and self satisfaction instead of selfish ego fueling; meaning don't code with the end goal of getting something on the front page of HN/Reddit or super git committer. Just code for the sheer enjoyment and challenge of it. Besides when you post these ambitious projects you get all sorts of jealous reactionary posts that attempt to pick apart your ideas and stomp on them. That can discourage people from making true progress.

mackal• 8 years ago

Blog makes an error in their assumption of what assert_return() is ... It's not a macro around assert() or related.

Possibly a bad idea on systemd's part to reuse the name assert, but the code isn't doing what the blog assumes it does.

verytrivial• 8 years ago

This article started out really well, saying you shouldn't e.g. byte-swap as part of the overall task at hand, then wanders into comparisons with other languages. I had hoped it was instead going to say that you should use some verified parsing tool to unpack data. All the other languages, at some level, will byte swap! It is the choice of the level of abstraction (or lack thereof) which is the systemd bug here, not the choice of a C family language IMNSHO.

IshKebab• 8 years ago

This is the first reasonable criticism of systemd I've heard. That is very bad code.

Go0the0gophers• 8 years ago

Always fuzz your parser! (e.g. AFL, go fuzz)

rphlx• 8 years ago

For some further, semi-amusing support for the title's assertion, save all of your work, close important programs, and then run 'sudo systemctl --dry-run reboot'

stmw• 8 years ago

Article declares that "What you should do instead is parse data the same way as if you were writing code in JavaScript," followed by "pointer arithmetic is wrong" (paraphrasing). That seems a bit extreme...

MichaelMoser123• 8 years ago

gcc is running in -fno-permissive by default, it will give you a warning if you implicitly try to converted between pointers of different types. (turn on warning as errors -Werror so that you have to deal with these messages)

Now what I don't understand is that for C you are not allowed to disable this, -fpermissive is not allowed for C, whereas for C++ you can. Anyone knows the reason why? The compiler error says "command line option "-fpermissive" is valid for C++/ObjC++ but not for C" - why is the compiler having this distinction?

Systemd is bad parsing

Related Stories