Denial of service through malicious payloads

[disconnect3d]

trafficstars

Hey,

As reported in https://github.com/zeta12ti/parse_duration/pull/18 there are payloads that makes the parse_duration::parse(input) to cause denial of service through big integer pow calculations.

I am not sure if the suggested solution is the best one, maybe there should be a way to specify the exponent limit or whether we accept an exponent at all in the duration string.

Since the repo didn't have any commit for a ~year, @zeta12ti are you going to fix it?

[disconnect3d]

To elaborate a bit more, I found this separately through the following fuzzing harness:

#[macro_use]
extern crate afl;

fn main() {
    fuzz!(|data: &[u8]| {
	if let Ok(s) = std::str::from_utf8(data) {
		parse_duration::parse(s);
	}
    });
}

Example hanging inputs: ;0e1111113111111m011111111111m00N.09N.00k, 1.07e-111111134 yr.

[tarcieri]

FYI, there is an open PR (https://github.com/RustSec/advisory-db/pull/827) to include this vulnerability in the RustSec Security Advisory Database.

It'd be great to hear back from @zeta12ti before we merge it (and ideally have a fixed version out).

[zeta12ti]

This repository is no longer being maintained (by me, at least). If anyone wants to pick it up, here are some options to fix this:

• Keep as is and make it clear that this crate is not to be used on untrusted input. The original idea behind the parser was correctness over efficiency, which is why I brought in BigInt in the first place. (to be clear, I'm not using it for anything right now, but that was the original usecase). In the interim, I'll add something along these lines in the README.
• Drop BigInt and just accept that things like 1e100 years -1e100 years 10 seconds won't parse correctly as 10 seconds.
• Drop support for negative components of the duration. This (I believe) means that if any intermediate result overflows, the whole thing will overflow too. (I'm not sure if that's entirely correct with nanoseconds, though - you might need some extra logic to deal with them). This is a lot more ergonomic now with the ? operator than when this crate was first written.
• Drop support for exponents. Without them, everything is just addition. I'm not sure how often anyone uses them.
• Limit exponentials to being below some bound. Again, this is a correctness vs efficiency problem.
• Rather than converting exponentials to BigInts, use some sort of sparse representation. For example, 1e100 seconds -12 seconds might be represented as {100: 1, 0: -12}. I believe it should be possible to efficiently (linear in input size) test whether such a number is within the range representable by a Duration.
• Add an option to switch between implementations, or simply additional functions.

[tarcieri]

Aah, good to know @zeta12ti

Is it okay if we publish the security advisory for this issue to the RustSec database? We can also publish a notice that this crate is presently unmaintained

[zeta12ti]

@tarcieri Go for it. Should I publish one last version so that the updated README shows up on crates.io, or is updating an unmaintained crate frowned upon (since it changes the "last updated" date)?

[tarcieri]

@zeta12ti if you'd like to publish one last version that'd be good.

We generally mark the unmaintained crate notices as having any hypothetical release after the last known released version count as "patched", so if someone does decide to start maintaining any of the crates we have marked as "unmaintained" again, the notices auto-clear.

[Diegovsky]

I am willing to take over this crate