1. 36
  1.  

  2. 35

    Two important notes:

    First, Rust includes a runtime mitigation for this issue: when CString is dropped, it writes 0 into the first byte of the string. So, for the case where you immediately call the C function, without intermediate allocations, C will see an empty string. This excludes “things work by accident most of the time” problem. It obviously doesn’t exclude “nothing works, but nobody notices” problem :-)

    Second, the story how this bug was found. I was one of the original reporters of the issue. I’ve learned about the code in question from some random link here on lobsters, noticed the initial announcement which mentioned unsafe, opened a handful of source files in the browser, Ctrl+F for unsafe and found the issue (taking like 10 minutes or so in total). That is, the issue was found not during a security review, but when some rando who had no idea about what the code should do, just looked at the unsafe blocks (to be fair, I am familiar with this particular Rust footgun). This happens to me at least the second time so I guess either I am good at randomly looking at security issues, or Rust is good at surfacing security issues to drive-by reviewers.

    EDIT:

    Zero: there’s still some confusion in the comments, so let’s me spell @notriddle’s comment more explicitly:

    There’s no bug in Rust’s libc crate. It exposes chown as is, without any wrapping. To keep the owner, it should be called like this:

    libc::chown(path, uid, !0)
    

    C uses -1 “syntax” (rather, implicit conversions) to mean unsigned number with all ones. Rust uses !0. It’s not surprising that C syntax doesn’t work in Rust.

    Similarly, you can’t pass 0 to a function expecting a pointer in Rust, you need to spell it as ptr::null. It’s kind of the point of the language :-)

    1. 3

      Per the second point, I’ve had unsafety issues pointed out to me via the same method. Honestly, I don’t even think they would have gotten past a code review (but since I’m the sole developer those aren’t an option)

    2. 14

      Ted made a classic mistake in unsafe code (one I’ve made myself). This is why the Rust community normatively discourages writing unsafe code if it can be avoided. If I were a code reviewer, I’d be paying specific attention to any module containing unsafe code.

      It’s worth saying that the as_ptr documentation explicitly notes this issue, and another option instead of the one shown would be to use CString::into_raw which you can safely cast from *mut c_char to *const c_char.

      1. 7

        I’m not a Rust developer, but it seems to me Ted’s point here is that it seems this scenario would be harder to catch by the code reviewer paying special attention, while any reviewer of Go code would see the opening/accessing of a resource (even if they were themselves on shaky ground with unsafe code) and think “that next line better be a defer statement to clean up”.

        1. 13

          First, I want to say I appreciate Ted for voicing his experience with Rust in this area, and I do think he hit this speed bump honestly, did his best to resolve it, and is understandably displeased with the experience. Nothing I say here is intended as a judgment of Ted’s efforts or skill. This is a hard thing to do right, and he did his best.

          To your comment: I agree that is part of his point. I don’t agree in his assessment that the Go code is “clearer” [1]. In Rust, any unsafety is likely to receive substantial scrutiny, and passing clippy is table-stakes (in this case, Clippy flags the bad code, which Ted notes). I view the unsafe presence in the module as a red-flag that the entire module must be assessed, as the module is the safety boundary due to Rust’s privacy rules (see, “Working with Unsafe” from the Rustonomicon).

          That said, Rust can improve in this area, and work is ongoing to better specify unsafe code, and develop norms, documentation, and tooling for how to do unsafe code correctly. In this case, I’d love to see the Clippy lint and other safety lints like it incorporated into the compiler in the future.

          [1] Assessing “clarity” between two languages is really tough because clarity is evaluated in the context of socialization around a language’s norms, so a person can only effectively make the comparison if they are at least sufficiently knowledgeable in both languages to understand the norms around code smells, which I don’t think Ted is.

          1. 5

            Is clippy used that pervasively? I’d rather have that particular warning in the compiler itself, in this case. Clippy seems to be super verbose and I don’t care too much for a tool that has strong opinions on code style.

            1. 8

              Feel free to file an issue to uplift the warning into rustc: Clippy warnings, especially deny-by-default clippy warnings – are totally fair game for uplifting into rustc if they’re seen as being useful enough.

              Clippy has a lower bar for lints, but for high value correctness lints like this one the only reason a lint is in clippy is typically “rustc is slower to accept new lints”, and if a lint has great success from baking in clippy, it can be upstreamed.

              People use clippy quite pervasively. Note that in this case the clippy lint that is triggered is deny-by-default, and all deny-by-default clippy lints are “this is almost definitely a bug” as opposed to “your code looks ugly” or whatever. You can easily turn off the different classes of clippy lints wholesale, and many people who want correctness or perf lints but not complexity or style lints do just that.

              1. 2

                In my experience, yes Clippy is used pervasively (but maybe I’m wrong).

                1. 1

                  We have a few Rust projects at $work and we use cargo clippy -- -D warnings as part of our builds. It’s a very popular tool.

            2. 3

              This is not about unsafe code as this code works in safe rust but it is still not correct. Proof

              I think using as_ptr (and similar functions) in this way should 100% be an error.

              1. 1

                However, there’s no bug in the example you’ve written as the pointer is never dereferenced.

              2. 1

                Unable to edit the post anymore, but others are right to point out that I missed part of the point here, and it’s important to note that CString::into_raw will leak the memory unless you reclaim it with CString::from_raw, and that CString::as_ptr is a challenging API because it relies on the lifetime of the original CString in a way that isn’t unsafe per Rust’s formal definition, but is still sharp-edged and may easily and silently go wrong. It’s caught by a Clippy lint, but there are some for whom Clippy is too aggressive.

                Thanks @hjr3 and @porges.

                1. 1

                  Be careful (to other readers) that into_raw used in the same way here would leak the memory. Really as_ptr is very suspicious in any code, in exactly the same way that c_str is in C++.

                2. 12

                  I’ll spare you the long rant about how one should never redeclare system interfaces if you can’t take the time to do so properly, because it turns out if you dig into it, uid_t boils down to uint32_t, but even so, the manual for chown says -1 should work and I want it to work.

                  Use !0 – It’s the same thing on any twos-complement computer.

                  Yes, I know that this is totally irrelevant to the rest of the post, but I want to show off. It’s also worth knowing this anyway, because there’s a lot of code out there that uses -1 as a shorthand for “binary number made of all ones.”

                  1. 5

                    I’m quite familiar with the C++ version of this mistake, which looks like

                    const char *cstr = something_returning_string().c_str();

                    It’s an easy mistake to make. Fortunately, I always turn on the Clang Address Sanitizer when I build in debug mode, and it immediately catches this at runtime. Does Rust have some equivalent of this? (I guess there’s Valgrind, but IIRC that’s more complicated and slower.)

                    At the heart of the problem … is a function to gift files from the server to the recipient.

                    I can’t help pointing out the irony that “gift” is German for “poison”…

                    1. 3

                      Address Sanitizer works with Rust — it’s an LLVM feature, not a C feature. Valgrind works too.

                      Rust also has Miri, which is a Rust interpreter that catches Undefined Behavior quite precisely (more so than tools based on memory pages or only information in the compiled program).

                      fn main() {
                          let c = std::ffi::CString::new("oops").unwrap().as_ptr();
                          unsafe {
                              *c;
                          }
                      }
                      

                      error: Undefined Behavior: pointer to alloc2052 was dereferenced after this allocation got freed

                      However, in OP’s case the pointer was dereferened on the C side, so the Rust interpreter wouldn’t see it.

                      1. 1

                        Clang will also catch this at compile time with -Wlifetime: https://godbolt.org/z/M3oEWh

                        It would also be nice if c_str was marked with the “&” ref-qualification so that you couldn’t invoke it on temporaries, but with lifetime analysis this is less of an issue.

                        Really this indicates the need for Rust to have lifetime parameters on raw pointers…

                        1. 3

                          It’s actually nice to call .c_str on temporaries if you are immediately passing the resulting pointer to a function in the same full-expression (and the function uses the pointer only within that call) (since the temporary string would survive till the end of the full l-expression anyways).

                          eg. someFoo((someString() + "bar").c_str());

                      2. 4

                        I will mention, since this is sometimes confusing to people first encountering Clippy: Clippy does distinguish between “this is probably bad and incorrect” vs “this is bad style” in its lints. These are the clippy::correctness and clippy::style lint groups respectively (there are a few others, like complexity and perf, that are listed in the readme). Only correctness causes clippy to fail builds by default, the others just spew warnings unless you deny them.

                        If you want to see only the correctness lints you can just turn off the other groups of lints (or turn off clippy::all and turn correctness back on).

                        Suggestions on how to document this better would be appreciated. This is kinda front and foremost in the clippy readme.


                        We are totally open to moving lints from clippy to rustc, this is up to the compiler/lang teams but you can just file an issue and link to it in https://github.com/rust-lang/rust/issues/53224 to get the ball rolling. There’s no automatic process to uplift lints after they have “baked” for a sufficient period of time, however, we just do it whenever people feel a need.

                        1. 4

                          I agree with all the comments here; but no one mentions that this issue is because of FFI.

                          If chown was written in Rust, there’d be no problem.

                          1. 2

                            I’m not sure it’s a matter of chown being written in Rust so much as that the version in the libc crate doesn’t expose the full capability of the original function—as the OP points out. If the “real” libc function can accept an argument of -1, it’s arguably a bug that the Rust version requires an unsigned integer for that parameter. (More broadly, it’s a bug that the Rust version lacks a capability that the original version had. It could also allow, say, an Option<u32> for the group ID, with None meaning “I don’t want to change the gid.” That would do a little better at making illegal states unrepresentable.) I don’t see how it matters whether the actual implementation is in C or Rust or whatever.

                            1. 4

                              There’s no bug in Rust’c libc, see last bullet at https://lobste.rs/s/9e7o8e/comparative_unsafety#c_btqrdt.

                              1. 1

                                Ah, I understand now. Thanks for explaining in that other comment!

                          2. 4

                            the kind of smtp server you’d write in two days to see what happens

                            That was a well-crafted comment that framed the rest of the post perfectly. I also had the misfortune to read that just as I was taking a sip of coffee, and had to clean my desk as a consequence.

                            1. 6

                              Somewhat off-topic, but why does this page has a fake loading bar that appears everytime the tab lose focus.

                              1. 18

                                If I recall correctly, it’s to encourage us to disable JavaScript.

                                1. 4

                                  And it doesn’t fucking work. It makes me disable Javascript… for that domain.

                                  1. 3

                                    It made a friend write a Firefox plugin to block just it. https://github.com/phy1729/reangst

                                    1. 3

                                      It works on me. I just don’t visit that site. Now I’m not using JavaScript on his website. Mission accomplished.

                                    2. 1

                                      That’s interesting. Good for them for believing in something enough to take a stand. I use a lot of websites that rely on it, and will never disable JavaScript, but I appreciate people taking a stand. Thanks for the insight!

                                    3. 4

                                      Yeah, it’s super annoying, IMO. I’m fine with it on initial page load - it’s quirky and funny. But every time I switch tabs? Super annoying.

                                      1. 3

                                        It’s buggy annoying JS.

                                      2. 2

                                        I use this personally: https://github.com/phy1729/reangst to block it.

                                        1. 1

                                          oh nice catch, I’m blocking unknown domains via whitelisting per domain in umatrix

                                        2. 6

                                          It’s off-topic, but the loading bars on this website (which gets posted on lobsters quite often) drive me nuts. They are purely cosmetic, and they show up not only on initial loading but every time the tab loses focus. If anyone else feels like me, I solved it by disabling the javascript on the page through ublock origin.

                                          1. 3

                                            It’s interesting that Rust is simultaneously safer because it handles deallocation, a very scary thing, for you, while also be less safe in cases like this by hiding that very scary thing. And I agree that the Go code is clearly unsafe when compared to the Rust code.

                                            There’s clearly work left for improving these potential footgun situations.

                                            1. 1

                                              My interpretation is that the compiler / the borrow checker is merely the first milestone to get it to build when everything is “best case” behavior and no errors.

                                              Then you have to go back to all the accessors and unwraps() and so on and turn them into larger blocks that handle the error cases. If you skip that, you’ll get panics and (in ted’s case) unsafety turning into security bugs.

                                              I understand that ted tried using clippy and found it to annoying. But I’ve also made the experience that your code gets better. For my (limited toy programming) rust adventures, I’ve accepted clippy as the next boss, after you’ve defeated the compiler.

                                              1. 1

                                                That bug in libc is quite unfortunate, it seems. I also am wary of using unsafe with managed resources (like whenever one has to reach for std::mem::forget), so it’s best if it’s in libraries…

                                                1. 7

                                                  There’s no bug in Rust’c libc, see last bullet at https://lobste.rs/s/9e7o8e/comparative_unsafety#c_btqrdt.

                                                  1. 3

                                                    wow, I did not know that about C’s -1 😱😱😱. TIL.