Bug #5176
closed
ValidityPeriod.NotAfter=99991231T235959 misinterpreted
Added by Junxiao Shi almost 3 years ago.
Updated 9 months ago.
Description
The following command imports a SafeBag that contains a certificate whose ValidityPeriod.NotAfter is set to "99991231T235959" i.e. 9999-12-31 23:59:59 UTC, which is a valid ISO8601 timestamp and also satisfies the ABNF definition for this field.
However, ndn-cxx misinterprets this field to be 1816-03-30 05:56:08 UTC.
$ echo 'gP0CEAb9ARsHIggBUwgDS0VZJAgABceDW8GxaAgEc2VsZiMIAAABesFfK3MUCRgBAhkEADbugBVbMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEw++PLgtFQLGDKYIPB8Qw6+e50TuT1zZRzamZmSdhD7NknRvNsaNXP/noEs+S0s35ddmgN5tp4Cvd4vIX6QyJ9BZDGwEDHBQHEggBUwgDS0VZJAgABceDW8GxaP0A/Sb9AP4PMTk4NzAyMTJUMDYzMDAw/QD/Dzk5OTkxMjMxVDIzNTk1ORdIMEYCIQD1JU+Kmeut3zUZ8BWcLhrXf4geN+lKENKB7pI5SI9OZgIhAOUV/J3Nlso7ypNUtb3Ovcg9H3BNpQvn0DcBSZnFgl/Rge8wgewwVwYJKoZIhvcNAQUNMEowKQYJKoZIhvcNAQUMMBwECKo+z3zz2Q1uAgIIADAMBggqhkiG9w0CCQUAMB0GCWCGSAFlAwQBKgQQXf/OKYBrL3NND7Asv2j4xASBkGF2pObyOD8j9fW5b18wbXCyy9wblt+iBV0DgIa5TszztU0XyK7A4uybuY+J6Ud7Izfy1h0Ww1cqfnOJLLcG/vo8tcqjNfEeKYjbOW3A0cQSqwf5uPRueCA8qURHjCawoYaV7yvxpDbKLCLdMwQhdsok3t70EWtpsp1Yoc/1/eiC2Rc8o1euT1apDC7LItnFpA==' | ndnsec import -P 9c570742-82ed-41a8-a370-8e0c8806e5e4 -
$ NDN_NAME_ALT_URI=0 ndnsec cert-dump -pi /S
Certificate name:
/8=S/8=KEY/36=%00%05%C7%83%5B%C1%B1h/8=self/35=%00%00%01z%C1_%2Bs
Validity:
NotBefore: 19870212T063000
NotAfter: 18160330T055608
Public key bits:
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEw++PLgtFQLGDKYIPB8Qw6+e50TuT
1zZRzamZmSdhD7NknRvNsaNXP/noEs+S0s35ddmgN5tp4Cvd4vIX6QyJ9A==
Signature Information:
Signature Type: SignatureSha256WithEcdsa
Key Locator: Self-Signed Name=/8=S/8=KEY/36=%00%05%C7%83%5B%C1%B1h
I did some testing on this.
I found the root cause is: ValidityPeriod type stores timestamps as boost::chrono::time_point<system_clock> type, which represents the timestamp as int64_t nanoseconds since Unix epoch.
For any date after 22620411T234716, the int64 overflows.
I don't think so. IIRC ValidityPeriod uses a time point with seconds granularity, instead of nanoseconds, precisely to avoid this issue. My guess (I haven't looked at the code) is that the problem lies in the conversion to/from this internal format to whatever chrono format is used externally, which does suffer from the limitation you described.
- Related to Bug #3915: time::toIsoString and time::toString overflow with large dates added
Davide Pesavento wrote in #note-2:
ValidityPeriod uses a time point with seconds granularity, instead of nanoseconds, precisely to avoid this issue. My guess is that the problem lies in the conversion to/from this internal format to whatever chrono format is used externally
I inserted some printing in ValidityPeriod and Certificate types, to observe the values during the cert-dump command.
| variable |
value |
.m_notAfter |
-4852116232 seconds since Jan 1, 1970 |
.getPeriod().second.time_since_epoch() |
-4852116232000000000 nanoseconds |
.wireEncode() with if-hasWire condition deleted |
253[38]={254[15]=313938373032313254303633303030,255[15]=313831363033333054303535363038} |
This suggested that the overflow occurred during decoding.
This time::fromIsoString looks suspicious: it returns system_clock::time_point that cannot represent large dates.
m_notAfter = time_point_cast<TimePoint::duration>(
time::fromIsoString(readString(m_wire.elements()[NOT_AFTER_OFFSET])));
Going further into time::fromIsoString and convertToTimePoint:
| variable |
value |
ptime |
9999-Dec-31 23:59:59 |
sinceEpoch |
70389527:59:59 |
sinceEpoch.total_seconds() |
253402300799 |
seconds(sinceEpoch.ticks() / bpt::time_duration::ticks_per_second()) |
253402300799 seconds |
point |
-4852116232933722624 nanoseconds since Jan 1, 1970 |
The point variable has type system_clock::time_point where the large date overflows.
I can propose two potential solutions to this bug.
Both solution would allow ValidityPeriod::isValid to work correctly, at least within the next few decades.
They may not allow ndn-cxx to generate certificates with large dates, but they would ensure ndn-cxx is interoperable with other software that publishes such certificates.
Simpler solution is interpreting NotBefore and NotAfter dates that would under/overflow system_clock::time_point type to the limit values.
This solution can be implemented by performing a string comparison in the decoding function.
Under this solution, the sample certificate would be decoded as having a NotAfter date of 22620411T234716.
More complex solution is introducing templated date conversion functions, such as time::pointFromIsoString<ValidityPeriod::TimePoint>(str).
Additionally, ValidityPeriod::getPeriod return type must be changed from system_clock::time_point to ValidityPeriod::TimePoint, which would be a breaking change.
- Status changed from New to Resolved
- Assignee set to Junxiao Shi
- Status changed from Resolved to In Progress
The proper solution would be to use boost::posix_time::ptime directly and avoid roundtrips to/from system_clock::time_point, right?
Davide Pesavento wrote in #note-7:
The proper solution would be to use boost::posix_time::ptime directly and avoid roundtrips to/from system_clock::time_point, right?
Yes, but it requires a breaking change in getPeriod return type.
This could be considered in a later commit.
Junxiao Shi wrote in #note-8:
Yes, but it requires a breaking change in getPeriod return type.
Not necessarily. We could deprecate getPeriod (keeping its current broken behavior) and add a pair of getters for NotBefore and NotAfter with the proper behavior and return type.
This could be considered in a later commit.
Agreed.
- % Done changed from 0 to 50
- Status changed from In Progress to Closed
- % Done changed from 50 to 100
Better solution may be sought in #5280.
- Related to Bug #5280: ValidityPeriod.NotAfter=99991231T235959 truncated added
Also available in: Atom
PDF
Updated by 
Updated by