4 ms·

The commit with fix: https://github.com/openssl/openssl/commit/2a40b7bc7b94dd7de897a74571e7024f0cf0d63b https://github.com/openssl/openssl/commit/2a40b7bc7b94dd

by Jenk 6y ago

The commit with fix: https://github.com/openssl/openssl/commit/2a40b7bc7b94dd7de897a74571e7024f0cf0d63b https://github.com/openssl/openssl/commit/2a40b7bc7b94dd7de8...

flatiron 6y ago
thats the first fix, here's the null pointer deference: https://github.com/openssl/openssl/commit/02b1636fe3db274497304a3e95a4e32ced7e841b https://github.com/openssl/openssl/commit/02b1636fe3db274497...
- CrazyPyroLinux 6y ago
  Not a Rust troll (never used it) but would this be one of those things Rust would prevent?
  - tptacek 6y ago
    Ehh, sort of. This is a NULL pointer read; it's a crasher that can't be further weaponized. Approximately the same thing happens in Python, Javascript, Java, and Go when you mis-handle a nil value. Rust goes through some trouble to avoid nil values altogether, and it's great. But in practice, applying matching and iflets to every return value everywhere makes for very noisy code (I mean, Rust is already very noisy, but bear with me), so the idiom in the language is to call "unwrap" on Option and Result values that are "known" to be safe. A mishandled "unwrap" will do approximately the same thing to your program as dereferencing a NULL pointer will. Rust helps here a lot, more than most other languages. It does not foreclose on this kind of bug, though, the way it (and other memory safe languages) foreclose on other memory corruption vulnerabilities. (This is notable not the case for NULL pointer _writes_, which occasionally can be weaponized. Use Rust in preference to C.)
    - richardwhiuk 6y ago
      `?` would be the natural thing to do in Rust, rather than unwrap.
      - tptacek 6y ago
        I know that's the case, but I looked a bunch of Rust codebases that I had lying around (exa, Firecracker, netlink, Servo), and I see a ton of `unwrap`s, too; not just in test code.
        bluejekyll 6y ago
        Yeah, I made this mistake in trust-dns. Went back and thought better on it, and replaced all unwraps with proper checks. After that no more random crashes, but you're correct that it's easy even in Rust to say, "I'm sure this is never None in this case", and be very wrong. The Rust linter (clippy) does perform a bunch of checks on unwrap usage now. For example, a common mistake before TryFrom was added to the stdlib was to implement From between two types and do an unwrap on conversion (think String -> enum variant), and clippy will now suggest TryFrom as a replacement since the unwrap is hiding a fallible case. The fact that folks use unwrap so much in tests and examples can also mislead people new to the language that this is a common practice.
        tptacek 6y ago
        I thought about that --- particularly for Firecracker, which has so much test code --- and no, it's in the actual code too. I think Rust does more than any other mainstream language to mitigate this problem. I'm just saying, it still exists in Rust; it's just called a panic on unwrap/expect, instead of "null pointer exception".
        saagarjha 6y ago
        Using unwraps is often the “proper check”. Trying to come up with error handling in cases where invariants are not met is usually done wrong.
        bluejekyll 6y ago
        That's not a check at that point, though, it's an assertion that the None case is invalid. I'm not saying you should never use unwrap, but I've been burned by it when treating it a little too nonchalantly. If you're implementing a lower level library, like a dns stub resolver, panicking will bring down the software using your library... which is definitely not something people are generally happy about. Error handling isn't always the right thing, sometimes just returning Option is the correct thing.
    - Tobu 6y ago
      Dereferencing null is UB; there is no guarantee that it will compile to something that segfaults or that it won't be exploitable. This is unlike None.unwrap(), which is a guaranteed panic. Exploitable null dereference, for anyone who needs a reminder: https://lwn.net/Articles/342330/ https://lwn.net/Articles/342330/
      - tptacek 6y ago
        The example you've provided isn't simply a NULL pointer dereference. The attacker had control over memory mapping! NULL pointers can (uncommonly) be exploitable --- especially in the kernel, where the 0 address can be mapped --- but you can't generally exploit them simply by attempting to read from them. The most common general pattern I'm aware of is a write, through a NULL pointer, that includes an unbounded offset. This isn't that.

by Jenk 6y ago

The commit with fix: https://github.com/openssl/openssl/commit/2a40b7bc7b94dd7de897a74571e7024f0cf0d63b https://github.com/openssl/openssl/commit/2a40b7bc7b94dd7de8...

flatiron 6y ago

thats the first fix, here's the null pointer deference: https://github.com/openssl/openssl/commit/02b1636fe3db274497304a3e95a4e32ced7e841b https://github.com/openssl/openssl/commit/02b1636fe3db274497...

CrazyPyroLinux 6y ago
Not a Rust troll (never used it) but would this be one of those things Rust would prevent?
- tptacek 6y ago
  Ehh, sort of. This is a NULL pointer read; it's a crasher that can't be further weaponized. Approximately the same thing happens in Python, Javascript, Java, and Go when you mis-handle a nil value. Rust goes through some trouble to avoid nil values altogether, and it's great. But in practice, applying matching and iflets to every return value everywhere makes for very noisy code (I mean, Rust is already very noisy, but bear with me), so the idiom in the language is to call "unwrap" on Option and Result values that are "known" to be safe. A mishandled "unwrap" will do approximately the same thing to your program as dereferencing a NULL pointer will. Rust helps here a lot, more than most other languages. It does not foreclose on this kind of bug, though, the way it (and other memory safe languages) foreclose on other memory corruption vulnerabilities. (This is notable not the case for NULL pointer _writes_, which occasionally can be weaponized. Use Rust in preference to C.)
  - richardwhiuk 6y ago
    `?` would be the natural thing to do in Rust, rather than unwrap.
    - tptacek 6y ago
      I know that's the case, but I looked a bunch of Rust codebases that I had lying around (exa, Firecracker, netlink, Servo), and I see a ton of `unwrap`s, too; not just in test code.
      - bluejekyll 6y ago
        Yeah, I made this mistake in trust-dns. Went back and thought better on it, and replaced all unwraps with proper checks. After that no more random crashes, but you're correct that it's easy even in Rust to say, "I'm sure this is never None in this case", and be very wrong. The Rust linter (clippy) does perform a bunch of checks on unwrap usage now. For example, a common mistake before TryFrom was added to the stdlib was to implement From between two types and do an unwrap on conversion (think String -> enum variant), and clippy will now suggest TryFrom as a replacement since the unwrap is hiding a fallible case. The fact that folks use unwrap so much in tests and examples can also mislead people new to the language that this is a common practice.
        tptacek 6y ago
        I thought about that --- particularly for Firecracker, which has so much test code --- and no, it's in the actual code too. I think Rust does more than any other mainstream language to mitigate this problem. I'm just saying, it still exists in Rust; it's just called a panic on unwrap/expect, instead of "null pointer exception".
        saagarjha 6y ago
        Using unwraps is often the “proper check”. Trying to come up with error handling in cases where invariants are not met is usually done wrong.
        bluejekyll 6y ago
        That's not a check at that point, though, it's an assertion that the None case is invalid. I'm not saying you should never use unwrap, but I've been burned by it when treating it a little too nonchalantly. If you're implementing a lower level library, like a dns stub resolver, panicking will bring down the software using your library... which is definitely not something people are generally happy about. Error handling isn't always the right thing, sometimes just returning Option is the correct thing.
  - Tobu 6y ago
    Dereferencing null is UB; there is no guarantee that it will compile to something that segfaults or that it won't be exploitable. This is unlike None.unwrap(), which is a guaranteed panic. Exploitable null dereference, for anyone who needs a reminder: https://lwn.net/Articles/342330/ https://lwn.net/Articles/342330/
    - tptacek 6y ago
      The example you've provided isn't simply a NULL pointer dereference. The attacker had control over memory mapping! NULL pointers can (uncommonly) be exploitable --- especially in the kernel, where the 0 address can be mapped --- but you can't generally exploit them simply by attempting to read from them. The most common general pattern I'm aware of is a write, through a NULL pointer, that includes an unbounded offset. This isn't that.