A remote code execution vulnerability in GNOME [LWN.net]

A remote code execution vulnerability in GNOME

Posted Oct 10, 2023 20:13 UTC (Tue) by rweikusat2 (subscriber, #117920)
In reply to: A remote code execution vulnerability in GNOME by fredrik
Parent article: A remote code execution vulnerability in GNOME

The vulnerability is an integer overflow, ie, someone using atoi(3) to parse a number and assuming the result will always be positive. This an ancient UNIX interface which has been deficient (or unexpectedly featured) since its invention in the early 1970s (it's already documented as machine code subroutine for the first version of UNIX). The proper way to handle this is to use the proper interface for it (strtoul(3) --- standardized since the first ANSI C standard) and check for error returns.

I don't think the attitude behind this kind of "Just can't be arsed!"-coding can be fixed by doing it in a different programming language.

to post comments

A remote code execution vulnerability in GNOME

Posted Oct 10, 2023 20:36 UTC (Tue) by rahulsundaram (subscriber, #21946) [Link] (3 responses)

> I don't think the attitude behind this kind of "Just can't be arsed!"-coding can be fixed by doing it in a different programming language.

It may very well help. Languages like say Go or Rust have less legacy traps but also are typically used with optional linters like gosec and Clippy that warns about the common issues. I don't know whether C developers are usually doing this.

A remote code execution vulnerability in GNOME

Posted Oct 10, 2023 20:57 UTC (Tue) by Wol (subscriber, #4433) [Link]

Well, when I set the coding standards for C programming, the rule was simple.

"I know we can't stop the compiler complaining, but you set the warning level to max and if you can't explain the warning away, it's a fatal error".

That was Microsoft C, and with warning level set to 4, we were calling a bought-in library and we couldn't suppress the "unused argument" warning. Anything else got fixed.

Cheers,
Wol

A remote code execution vulnerability in GNOME

Posted Oct 10, 2023 21:02 UTC (Tue) by proski (guest, #104) [Link] (1 responses)

I worked on C++ code in the past where we were making a great effort to catch possible issues in the code (~90% unit test coverage, code sanitizers, Valgrind, using multiple compilers with all warnings enabled), but I'm not sure we would catch that error unless testing the problematic value. With Rust, on the other hand, I'm confident that a non-exploitable panic is the worst thing we could get even without clippy.

A remote code execution vulnerability in GNOME

Posted Oct 11, 2023 8:58 UTC (Wed) by NAR (subscriber, #1313) [Link]

The presence of atoi in itself is a code smell - and has been for the past 20 years.

A remote code execution vulnerability in GNOME

Posted Oct 10, 2023 21:00 UTC (Tue) by dvrabel (subscriber, #9500) [Link] (11 responses)

    fn main() {
        let i = "4294903295".parse::<u32>().unwrap() as i32;
        let v = vec![1, 2];
        if i >= (v.len() as i32) {
            return;
        }
        /* SAFETY: Bounds already checked. */
        let a = unsafe {
            v.get_unchecked(i as usize)
        };
        println!("{} {}", i, a);
    }

This is broadly what the vulnerability is, implemented in rust. This segfaults and can presumably result in similar exploits in real code.

It took a bit more work and the unsafe block should make reviewers look more carefully but the comment and the (similarly incorrect) bounds check could easily lead the reviewer to conclude its ok, particularly if this was buried in a larger commit and was being reviewed on a Friday afternoon.

Rust is a better language, but does not negate the need for through unit, component, system, and fuzz testing.

A remote code execution vulnerability in GNOME

Posted Oct 10, 2023 22:26 UTC (Tue) by walters (subscriber, #7396) [Link] (3 responses)

In Rust it's much more idiomatic to use iterators (as well as the checked `get()`) than direct array indexing because in many case it's actually *more* ergonomic, and doing so helps the compiler elide bounds checks actually.

For example https://doc.rust-lang.org/std/primitive.slice.html#method... is just way better than doing the equivalent indexing by hand.

So usage of `get_unchecked` is IME very unusual in Rust and would be a large code smell outside of very low level code.

A remote code execution vulnerability in GNOME

Posted Oct 10, 2023 23:49 UTC (Tue) by mathstuf (subscriber, #69389) [Link] (2 responses)

The `as` casts are also very non-idiomatic. I think as a translation it *works*, but yes, it's not what a "native" Rust programmer would write. Rather, someone that knows C, learned Rust-the-language but not Rust-with-its-stdlib.

A remote code execution vulnerability in GNOME

Posted Oct 11, 2023 17:51 UTC (Wed) by NYKevin (subscriber, #129325) [Link] (1 responses)

I'm not even sure a person who knows C-but-not-Rust would put in those casts. I find it hard to believe that the average C programmer would choose to parse something into a u32 and then immediately cast it to an i32 - that just makes no logical sense at all. If you want a u32, you parse it into a u32. If you want an i32, you parse it into an i32.

OTOH, the second cast (v.len() as i32) looks like it's implementing the "usual integer promotions" rule from C (i.e. the rule that everything magically promotes to int if you so much as breathe on it), and the last one (i as usize) is pretty transparently "the compiler made me do it." But C programs do int promotion because it is implicit, not because C programmers consciously choose to promote everything to int. Would a C programmer really choose to write code that juggles three different types, if all of the casts have to be written out explicitly? I'm not very good with Rust, but even I would see that and think "there has to be a better way" - and in this case, it seems fairly obvious that you can just parse it as usize to begin with. Then all of the casts go away and the bug is also fixed.

A remote code execution vulnerability in GNOME

Posted Oct 11, 2023 19:52 UTC (Wed) by ballombe (subscriber, #9523) [Link]

Compile your C code with g++ and suddenly you get warning for unsigned/signed mismatch.
That is what I do, and I only ever write C code.

This vulnerability is less about the C language than about the ISO C committee mental block about allowing explicit semantic qualifier for C type.

A remote code execution vulnerability in GNOME

Posted Oct 11, 2023 0:55 UTC (Wed) by roc (subscriber, #30627) [Link] (4 responses)

No-one would write this Rust code unless they were malicious, it's totally unnatural.

If you're reviewing file parsing code and you see "unsafe", that's a huge red flag. Come back to it on Monday.

A remote code execution vulnerability in GNOME

Posted Oct 11, 2023 13:18 UTC (Wed) by smoogen (subscriber, #97) [Link] (3 responses)

I could see it coded this way without 'direct' malicious intent:
1. It is a 'from another language' coder tasked with Rust code but not 'familiar' with what is Natural in Rust but familiar with their own language.
2. They are under a deadline and have been trying to deal with a time/speed issue in some code. They find this method is 'not natural' but faster. [Many sins in other languages occur because of this.]
3. They got this from some AI assist which found that comment and assumed it was good code.
4. They got this from reddit/etc where someone with direct malicious intent put it up in either a 'joke'(*) or to make something bad happen.

All of the items are poor coding in one way or another but not directly malicious.

(*) A long time ago I wrote a man page for a program which set real memory (rm) flag on files. This was to allow you to avoid getting swapped out in virtual memory on any file it was given. An additional bonus was to set the real fast flag on files so that the were set to nice -10 and get all the CPU. As a 'joke' it was sort of funny. When it was taken seriously... it was not.

A remote code execution vulnerability in GNOME

Posted Oct 11, 2023 13:37 UTC (Wed) by Wol (subscriber, #4433) [Link]

Hmmm...

Many moons ago on a multi-user system, I set up a batch queue. Back in the day when an 800MB winchester drive was about 5" x 5" x 2' in size. Any jobs submitted to that queue were basically set to max cpu priority, max i/o priority. However, it didn't get used much because it was also set to max 30sec wall clock. Beyond that, any job got terminated with prejudice. It was, however, useful for getting little jobs done quick :-)

Cheers,
Wol

A remote code execution vulnerability in GNOME

Posted Oct 11, 2023 18:03 UTC (Wed) by mb (subscriber, #50428) [Link]

>1. It is a 'from another language' coder tasked with Rust code but not 'familiar' with what is Natural in Rust but familiar with their own language.

Well, such a person would probably never use 'unsafe' and probably also wouldn't use 'get_unchecked'. The person would probably just do 'v[i]'. Which is safe.

This code is totally not what happens in the real world.

A remote code execution vulnerability in GNOME

Posted Oct 11, 2023 21:29 UTC (Wed) by roc (subscriber, #30627) [Link]

Those things theoretically could happen in but in practice they don't.

A remote code execution vulnerability in GNOME

Posted Oct 11, 2023 14:07 UTC (Wed) by farnz (subscriber, #17727) [Link]

As a Rust reviewer, I pick on the following issues; let's assume that I don't spot the vulnerability, but that I throw this code back to you with the following comments. Can you rework to address my review comments, without making the bug more obvious?

I see three casts using "as"; these are dangerous because they don't do checks as part of the cast. Can you rewrite these using from, into, try_from or try_into?
The Rust compiler is great at optimizing out bounds checks in most situations; can you use get, instead of get_unchecked, and rely on the optimizer removing the bounds check? If not, why not?

Basically, you've got two things in there (get_unchecked, and as casting) which trigger my "this is likely to be buggy" radar. Add in "unsafe", which triggers my "this needs close inspection" radar, and I'm likely to spot that there is an issue when I try to get you to write better Rust.

A remote code execution vulnerability in GNOME

Posted Oct 12, 2023 9:04 UTC (Thu) by Tobu (subscriber, #24111) [Link]

I don't believe someone would write this innocently when the alternative is a simple v[s.parse::<usize>()?].

I looked at the cue-parsing crates I could find: cuna, rbchunk, rcue, cue_sheet, none of them use unsafe, rcue forbids unsafe_code. cuna does depend on parser library nom, which has uses of unsafe in six string splitting functions and has unsafe dependencies for memchr and float parsing.

I do wish clippy had a blanket lint against all uses of `as` for numeric casts, which are a code smell when from/try_from exist. Right now you have to go through this list, many of which are allowed by default, and there are holes.

A remote code execution vulnerability in GNOME

Posted Oct 11, 2023 18:38 UTC (Wed) by adobriyan (guest, #30858) [Link]

strtoul() doesn't force to check for error. Programmer who is clueless to use atoi() will write strtoul() just as easily.
And there is no strtouint() so potential for clipping the result is still there.