paritytech / parity-db

Experimental blockchain database
Apache License 2.0
263 stars 59 forks source link

Misaligned pointer dereference in `chunk_entries_at` and `transmute_chunk` #223

Closed shinmao closed 9 months ago

shinmao commented 9 months ago

The source of unsoundness

Hi, we consider that the following two functions could have misaligned pointer dereference and lead to UB: https://github.com/paritytech/parity-db/blob/4ac2aca38c6984f8a48080875590a687041a39d8/src/index.rs#L255-L258 At line 258 of chunk_entries_at, https://github.com/paritytech/parity-db/blob/4ac2aca38c6984f8a48080875590a687041a39d8/src/index.rs#L418-L422 and at line 419 of transmute_chunk, they both tried to convert the type aligned to 1 byte to the type aligned to 8 bytes. Please check and would be happy to have any discussion:)

arkpar commented 9 months ago

Thanks for the report

Hi, we consider that the following two functions could have misaligned pointer dereference and lead to UB: parity-db/src/index.rs

Here the offset is guaranteed to point to a 512-byte aligned page.

parity-db/src/index.rs

This one looks technically unsound ineed since chunk maybe moved on to the stack. The function is marked #[inline(always)] but there's no strict guarantee that the stack value will be eliminated. Needs to be fixed.

shinmao commented 9 months ago

Hi @arkpar , sorry that I didn't see the your policy of reporting bugs. Do you think we can submit to RUSTSEC or GitHub Security Advisories Database?

arkpar commented 9 months ago

I don't think this is a security issue. It's more about formal source code correctness. The compiler still generates correct machine code here and I can't think of a way to exploit this. I guess it is technically an unsoundness, but not the one exposed in the API and not the one causing any real issues.

shinmao commented 9 months ago

Yes. @arkpar , that's the unsoundness issue from the perspective of Rust's language model. That's what RUSTSEC collects for unsoundness issues.

shinmao commented 9 months ago

@arkpar just for discussion, unaligned access could lead to runtime panic depends on the target machine. Isn't that kind of security issue leading to Denial of Service? 🤔️ At least runtime panic could crash the program.

arkpar commented 9 months ago

unaligned access could lead to runtime panic depends on the target machine.

Sure it can, but I'm arguing that unaligned access can't happen here in practice. In paractice the compiler generates code that passes the pointer without moving the data to the stack and that pointer is guaranteed to a be aligned. I don't think anyone can write an actual program for any supported platform using this crate, that can panic because of this issue. But I'd be happy to be proven wrong if you can demonstrate it.