Hacker News Reader

Comment by baq | Hacker News Reader

here's the fix: https://github.com/systemd/systemd/commit/06eeacb6fe029804f2...

questions to the local experts:

1) would using a differently designed open() api prevent the issue?

2) would not using C to write systemd prevent the issue? specifically, would using rust, ocaml, ats or ada prevent the issue?

View on HN

2) maybe. mode_t is unsigned and MODE_INVALID was defined as: #define MODE_INVALID ((mode_t) -1) and the problem was in a check: fd = open(path, O_WRONLY|O_CREAT|O_CLOEXEC|O_NOCTTY, mode > 0 ? mode : 0644);

so maybe the author thought MODE_INVALID < 0. though, maybe safe languages will let you do this explicit cast as well so maybe they won't save you.

the other thing is maybe in a safe language you would use an Option/Maybe type here instead of a plain mode_t type.

> maybe safe languages will let you do this explicit cast as well

They will let you, but explicit casts are a red flag in code review.

So in other words it wouldn't have made a difference.

A better type system gives you the option to enforce stricter checks to help you catch mistakes, but the same people with the same procedures would have written this bug in any language.

Not necessarily. If any unsafe constructs are locally visible during code review, and the language is such that unsafe constructs are rarely required, then it's much easier to give unsafe constructs a higher level of scrutiny that you can't afford to do in a language like C where unsafe things are pervasive and the same line can easily be safe in one context and unsafe in another.

"during code review". But did this code go through code review? If not your "higher level of scrutiny" are still not high enough to warrant mention.

I don't know about SystemD's code policies. But certainly serious vulnerabilities have been found even in C code where changes went through code review (the famous Chrome sandbox escape due to an undefined bitshift was noted to have been reviewed and explicitly "LGTMed" by two people).

And the decision about whether to code review is not necessarily static. A language that reduces the cost and/or increases the benefits of code reviews changes the decision space. And a more expressive language can free up developer time to spend on things like code review.

It wasn't really anything to do with the language, and far more to do with the operating system kernel API.

The fchown() system call supports passing -1, cast to the appropriate type, as a no-op value. The systemd people were attempting to wrap similar semantics around fchmod(). Originally in 2014 M. Sievers specified (mode_t)0 as the no-op value, which wasn't a good choice, with M. Poettering changing it to (mode_t)-1 in 2015 but overlooking one place where the value remained tested against 0.

* https://github.com/systemd/systemd/commit/c38dfac9ed6c1c3beb...

Incidentally: M. Sievers' original choice of (mode_t)0 as a no-op value lives on to this day in systemd-udevd.

* https://github.com/systemd/systemd/blob/v232/src/udev/udev-r...

The same error could technically occur in Rust if the API was designed that way but I think the "Rust way" would mandate using an `Option` instead of a special MODE_INVALID value.

So it would become something like `mode.unwrap_or(0o644)` which doesn't leave a lot of room for error.

Reply on news.ycombinator.com