pooyamb / serde-querystring

A query string parser for rust with support for different parsing methods
Apache License 2.0
10 stars 3 forks source link

Lexical dependency considered “unsound” #3

Open backspace opened 2 months ago

backspace commented 2 months ago

Hey, thanks for your work on this, I used it in an Axum application where I was receiving duplicate query parameters and your library could handle it, unlike the built-in extractor.

I’ve been getting security audit errors about the dependency on lexical:

error[unsound]: Multiple soundness issues
    ┌─ /home/runner/work/adventures/adventures/unmnemonic_devices_vrs/Cargo.lock:114:1
    │
114 │ lexical 6.1.1 registry+https://github.com/rust-lang/crates.io-index
    │ ------------------------------------------------------------------- unsound advisory detected
    │
    = ID: RUSTSEC-2023-0055
    = Advisory: https://rustsec.org/advisories/RUSTSEC-2023-0055
    = `lexical` contains multiple soundness issues:

       1. [Bytes::read() allows creating instances of types with invalid bit patterns](https://github.com/Alexhuszagh/rust-lexical/issues/102)
       1. [BytesIter::read() advances iterators out of bounds](https://github.com/Alexhuszagh/rust-lexical/issues/101)
       1. [The `BytesIter` trait has safety invariants but is public and not marked `unsafe`](https://github.com/Alexhuszagh/rust-lexical/issues/104)
       1. [`write_float()` calls `MaybeUninit::assume_init()` on uninitialized data, which is is not allowed by the Rust abstract machine](https://github.com/Alexhuszagh/rust-lexical/issues/95)

      The crate also has some correctness issues and appears to be unmaintained.

      ## Alternatives

      For quickly parsing floating-point numbers third-party crates are no longer needed. A fast float parsing algorith by the author of `lexical` has been [merged](https://github.com/rust-lang/rust/pull/86761) into libcore.

      For quickly parsing integers, consider `atoi` and `btoi` crates (100% safe code). `atoi_radix10` provides even faster parsing, but only with `-C target-cpu=native`, and at the cost of some `unsafe`.

      For formatting integers in a `#[no_std]` context consider the [`numtoa`](https://crates.io/crates/numtoa) crate.

      For working with big numbers consider `num-bigint` and `num-traits`.

I’m pretty new to Rust and not that confident about attempting the suggested alternatives, but I could give it a try if you don’t have capacity. I assume this would be a breaking change 🤔

pooyamb commented 1 month ago

@backspace Not sure how I missed this issue, thanks for bringing it into my attention. I will check this out to see if we are affected by the bug or not, and will release a new version with updated dependencies. Regarding it being a breaking change, I don't believe so. lexical is not directly exposed to the end user and is only used internally, so it should be safe to upgrade or replace.

pooyamb commented 1 month ago

@backspace I took a look into it today, and it looks like we might not be affected by the bug. But I think it would still make sense to dump lexical, since it looks unmaintained. Replacing integer parsing with atoi was straight forward, and I've already implemented that, replacing float parsing on the other hand, was not. Rust doesn't provide direct ascii &[u8] to f32|f64 parsing and we will need to perform utf-8 checking and get a str first. I believe that would not be wise to do, since float parsing does not require such implications to work, at least as far as I know. Do you have any idea how we can get around that, considering we don't actually know if the provided bytes slice is valid utf-8?

backspace commented 1 week ago

Do you have any idea how we can get around that

I’m sorry, I don’t! I’m too new to Rust to be of help 😞

pooyamb commented 1 week ago

@backspace Thanks a lot for the report, that was more than enough buddy, no worries. I pushed a commit today and will hopefully release it soon.