Whether it is a unsound issue

help
1

Hi, everyone. I am new to rust and not sure if it is a bug. Currently, I am scan the rust crates that use the "unsafe" API, I found some issue in "n2" crate.
The issue link is here:
slice

take slice as example, I write a Poc in my platform.

extern crate n2;

use n2::scanner::Scanner;

fn main() {
    // Prepare a valid UTF-8 byte array, null-terminated
    let valid_utf8_bytes: &[u8] = b"Hello, world!\0";

    // Create Scanner instance
    let scanner = Scanner::new(valid_utf8_bytes);

    // Attempt to slice with an unsafe range (beyond the size of buf)
    let start = 0;
    let end = 100; // Explicitly set out of bounds size

    // Call the slice method
    let result = scanner.slice(start, end);
    println!("Slice result: {}", result);
}

and the output is this:

 Compiling ne-test v0.1.0 (/home/lwz/github/ne-test)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.28s
     Running `target/debug/ne-test`
thread 'main' panicked at core/src/panicking.rs:221:5:
unsafe precondition(s) violated: slice::get_unchecked requires that the range is within the slice
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused non-unwinding panic. aborting.
已中止 (核心已转储)
lwz@lwz-ThinkStation-P3-Tower:~/github/ne-test$

I am not familar with the rust safety system, so is it a Bug or not? Should I report it to RustSec?

2

Panicing is considered a safe operation, and is one of the main tools to maintain safety in Rust— If you detect a condition that will cause UB if the program continues, you can panic to stop execution before the UB occurs.

While this probably isn’t the behavior that the programmer intended, it will at least behave consistently between runs so that finding the root problem isn’t a nightmare.

3 Likes
3

so, even the program is aborting, there is still no safety issue and it is consider fine?

4

it appears this panic is from the debug build of the standard library, which I think definitely indicates somewhere in the code had soundness issues. so I checked the code and indeed, the Scanner::slice() method is calling get_unchecked() without checking the safety condition (and it's not marked unsafe), here's the line:

6 Likes
5

Quick reply:

Panic and abort in general is fine, it's the "only" safe operation to do when you encounter something unable to recover while runtime.

It this specific case though, you are getting unsafe pre-cond check of debug build in std... So it actually is a bug in n2. (I don't think it's worth to report though?)

2 Likes
6

I also have a question about distinguishing between two types of panics. The first is:

thread 'main' panicked at src/main.rs:9:41:
called `Result::unwrap()` on an `Err` value: InvalidUri(InvalidUriChar)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I generally consider this kind of panic to be acceptable. However, there's another type of panic that causes the program to abort:

Compiling ne-test v0.1.0 (/home/lwz/github/ne-test)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.28s
     Running `target/debug/ne-test`
thread 'main' panicked at core/src/panicking.rs:221:5:
unsafe precondition(s) violated: slice::get_unchecked requires that the range is within the slice
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused non-unwinding panic. aborting.
Aborted(core dumped)

I have always thought that this type of panic that leads to aborting needs to be fixed. Is it true that both types of panic are acceptable in Rust?

7

I also submitted three issues 121, 122, and 123 in the n2 library. I think this may be worth fixing, but if this type of problem is not worth reporting as a security issue, then what kind of problem is worth reporting? Sorry, I am not very familiar with Rust's security specifications.

8

if the panic is an (implicit) implementation detail (e.g. to check safety pre-condition by an unsafe operation in debug mode), then there's a soundness issue.

however, if the panic is explicit, then it's not a soundness issue. that does NOT mean there's no bug in the code though, but it's a domain specific logic error, not a memory safety vulnerability.

9

So, do you think this bug should be reported to ”Rustsec-advisory-db“ or should it just be an issue in the n2 project?

10

At least you should file issue to n2. You can report to RustSec in addition.

11

ok, thanks

12

the abort is caused by the first panic, which is considered a "hard" (i.e. non-recoverable) error, since it is UB. if you solve the panic, the abort will not occur.

in debug mode of the standard library, when a safety pre-condition check fails, it will call panic_nounwind(), which eventually aborts the process (in theory you can install a hook to intercept the panic, but anything you do is pretty much unsound in such situation), the message you see is from this line:

13

so, if abort happened, it is a UB, and it must be a Bug. Correct?

14

correct.

and the explicit abort is only guarenteed to happen in debug build, because the pre-condition assertion turns (potential-)UB into panic_nounwind() in debug mode, in order to help programmer catch the bug.

in release mode, the check is disabled and you will actually trigger the UB. since the program behavior is "undefined", it can have any effect at runtime. e.g. you might get a crash with core dump (if you are lucky), or the program might seemingly run well but gives random results, etc.

15

It is sound only if it always panics before performing an invalid operation. If it panics in some configurations, but not others, then it's not sound, because there exist circumstances in which safe code can perform an invalid operation. Soundness is about ensuring safety always in all cases.

The panic in get_unchecked() is optional (it has to be, otherwise it'd be get()!) so it's always unsound to allow out of bounds range there.

3 Likes