The goals here are: 1. Reduce the NSPR dependency with the goal of eventually eliminating it. 2: Centralize some of the logic for integer underflow/overflow related to times. 3. Improve type safety by making preventing implicit conversions from integer to Time 4. Clearer code regarding the construction of uninitialized time values.

Comment on attachment 8461210 [details] [diff] [review] Part 1: Add mozilla::pkix::Time Review of attachment 8461210 [details] [diff] [review]: ----------------------------------------------------------------- r=me except I think DaysBeforeYear should be moved to this patch or the two should be combined.

Comment on attachment 8461212 [details] [diff] [review] Part 2: Update (almost) everything to use mozilla::pkix::Time instead of PRTIme Review of attachment 8461212 [details] [diff] [review]: ----------------------------------------------------------------- Looks good - just a couple of questions/comments. ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +375,5 @@ > ("NSSCertDBTrustDomain: no stapled OCSP response")); > } > > Result cachedResponseResult = Success; > + Time cachedResponseValidThrough = TimeFromElapsedSecondsAD(0); Why not Time(uninitialized)? @@ +597,5 @@ > Input encodedResponse, EncodedResponseSource responseSource, > /*out*/ bool& expired) > { > + Time thisUpdate(TimeFromElapsedSecondsAD(0)); > + Time validThrough(TimeFromElapsedSecondsAD(0)); same here ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +623,5 @@ > CERTCertificate* serverCert, > SECItem* stapledOCSPResponse, > uint32_t providerFlags, > + Time time, > + PRTime prtime); It's sort-of a bummer we have to keep both of these around, but maybe we can fix that later. ::: security/pkix/lib/pkixbuild.cpp @@ +91,5 @@ > { > if (newResult == Result::ERROR_UNTRUSTED_CERT) { > newResult = Result::ERROR_UNTRUSTED_ISSUER; > + } else if (newResult == Result::ERROR_EXPIRED_CERTIFICATE) { > + newResult = Result::ERROR_EXPIRED_ISSUER_CERTIFICATE; I agree this is a good change to make, but it seems to violate the "one bug per bug" idea. Does it work to have the changes related to this be a separate patch? (it can be part 2 or part 3 of this bug) ::: security/pkix/lib/pkixocsp.cpp @@ +59,3 @@ > } > if (validThrough) { > + *validThrough = TimeFromElapsedSecondsAD(0); Again, should these be Time(uninitialized)? @@ +595,5 @@ > > + static const uint64_t SLOP_SECONDS = Time::ONE_DAY_IN_SECONDS; > + > + Time timePlusSlop(context.time); > + (void) timePlusSlop.AddSeconds(SLOP_SECONDS); Even though AddSeconds overflowing means the user's computer is set very far in the future and all verifications will fail anyway, I still think it's a good idea to check for overflow here. @@ +627,2 @@ > } > + // XXX: wasn't there an overflow here? What was the overflow? thisUpdate + maxLifetime? thisUpdate couldn't be larger than context.time + SLOP, so that wouldn't overflow unless we were near the limit of what context.time could hold. @@ +638,4 @@ > } > > + Time timeMinusSlop(context.time); > + (void) timeMinusSlop.SubtractSeconds(SLOP_SECONDS); Same idea here checking for underflow. ::: security/pkix/lib/pkixutil.h @@ +182,5 @@ > void operator=(const NonOwningDERArray&) /* = delete*/; > }; > > +inline unsigned int > +DaysBeforeYear(unsigned int year) nit: unless these two patches are combined before commit, this should go in the other patch Also, this isn't technically correct due to the Gregorian reform, but I think as long as we're consistent and we only deal with recent times, this should be fine.

Comment on attachment 8461212 [details] [diff] [review] Part 2: Update (almost) everything to use mozilla::pkix::Time instead of PRTIme Review of attachment 8461212 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +375,5 @@ > ("NSSCertDBTrustDomain: no stapled OCSP response")); > } > > Result cachedResponseResult = Success; > + Time cachedResponseValidThrough = TimeFromElapsedSecondsAD(0); Changed. @@ +597,5 @@ > Input encodedResponse, EncodedResponseSource responseSource, > /*out*/ bool& expired) > { > + Time thisUpdate(TimeFromElapsedSecondsAD(0)); > + Time validThrough(TimeFromElapsedSecondsAD(0)); Changed. ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +623,5 @@ > CERTCertificate* serverCert, > SECItem* stapledOCSPResponse, > uint32_t providerFlags, > + Time time, > + PRTime prtime); I agree. ::: security/pkix/lib/pkixocsp.cpp @@ +59,3 @@ > } > if (validThrough) { > + *validThrough = TimeFromElapsedSecondsAD(0); No, because Time::uninitialized isn't actually a time value. If we don't want to initialize *thisUpdate and *validThrough here then we can just remove this code. But, I'm just going to leave it as-is. @@ +595,5 @@ > > + static const uint64_t SLOP_SECONDS = Time::ONE_DAY_IN_SECONDS; > + > + Time timePlusSlop(context.time); > + (void) timePlusSlop.AddSeconds(SLOP_SECONDS); Fixed. @@ +627,2 @@ > } > + // XXX: wasn't there an overflow here? I removed the comment. @@ +638,4 @@ > } > > + Time timeMinusSlop(context.time); > + (void) timeMinusSlop.SubtractSeconds(SLOP_SECONDS); Done. ::: security/pkix/lib/pkixutil.h @@ +182,5 @@ > void operator=(const NonOwningDERArray&) /* = delete*/; > }; > > +inline unsigned int > +DaysBeforeYear(unsigned int year) These patches don't build independently, so I will combine them together. However, I will factor out the changes to the FindIssuer implementations and the change for expired issuer certificates into a separate patch.