Conversion from prost_types::Timestamp to SystemTime may panic

[argv-minus-one]

In prost-types 0.7.0, the conversion from Timestamp to SystemTime uses the + and - operators on UNIX_EPOCH. This can overflow and panic, creating a denial-of-service vulnerability if the input Timestamp is untrusted.

Also, the conversion involves calling Timestamp::normalize, which uses the + and - operators. This may panic or wrap around (depending on compiler settings), also creating a denial-of-service vulnerability if the application is compiled to panic on overflow.

Proof of concept

use prost_types::Timestamp;
use std::time::SystemTime;

SystemTime::from(Timestamp {
    seconds: i64::MAX,
    nanos: 0,
}); // panics on i686-unknown-linux-gnu (but not x86_64) with default compiler settings

SystemTime::from(Timestamp {
    seconds: i64::MAX,
    nanos: i32::MAX,
}); // panics on x86_64-unknown-linux-gnu with default compiler settings

Fixing

Timestamp::normalize should probably use saturating_{add,sub}. This can silently change the timestamp by up to about 3 seconds if its nanos field is out of range, but such a timestamp is arguably invalid anyway, so that's probably okay.

SystemTime does not have saturating_{add,sub} methods, nor MIN and MAX constants. Therefore, unlike Timestamp::normalize, there is no way to silently clamp the input timestamp to the valid range of SystemTimes. That's probably a bad idea anyway, since SystemTime could be based on a 32-bit time_t or Windows FILETIME, in which case clamping it could change the timestamp by much more than a mere few seconds. I recommend making the conversion fallible again and using SystemTime::checked_{add,sub}.

[danburkert]

Thanks for the detailed report, @argv-minus-one. I definitely agree that it's an issue that the From impl can panic - that's not going to be expected by users, particularly for a library which is often using for handling untrusted input.

WRT the fix, I think I agree with the approach in #439, although I'm curious if we can keep the From impl for now (until 0.8) with an deprecation annotation pointing to the new TryFrom impl. A 0.8 release is eventually inevitable, but I'd like to get a fix for this out sooner than later, and having it be a patch on 0.7 would make that easier.

[argv-minus-one]

I don't believe so. Trait implementations cannot be deprecated.