Opened 6 months ago

Closed 2 weeks ago

#21420 closed defect (fixed)

Link certificate start date in the future

Reported by: mmcloughlin Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 029-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

Some link certificates are generated with start dates in the future. Is this intentional, or a bug?

These lines of code generate a start date for the link certificate. Why does this add two days?

https://github.com/torproject/tor/blob/9a9f4ffdfa965e50de05a4f1bd8c4d68cfb95f12/src/common/tortls.c#L481-L487

I confirmed this happens in the wild. This code connects to tor relays and inspects the link certificate. (Not beautiful code but it does the job.)

https://github.com/mmcloughlin/torcerts

It doesn't take too long to find an example. One I found just now:

addr=117.201.240.2:9001
now=Wed Feb  8 22:05:15 UTC 2017
notBefore=Feb 10 00:00:00 2017 GMT
-----BEGIN CERTIFICATE-----
MIIBtzCCASCgAwIBAgIJAOqgI76M4OOKMA0GCSqGSIb3DQEBBQUAMBsxGTAXBgNV
BAMMEHd3dy5iamdpN2d0dS5jb20wHhcNMTcwMjEwMDAwMDAwWhcNMTcwMjE2MjM1
OTU5WjAgMR4wHAYDVQQDDBV3d3cuM242bWNvaWp6Z3Jzai5uZXQwgZ8wDQYJKoZI
hvcNAQEBBQADgY0AMIGJAoGBAKRkfrD4q5HNIkE9lglJjljlZoT15OE3VDE66GYT
hZ/FsCMtGPw1TKj+EB6NyjEYSxP7+EgJOOVGzxb3ybmEs8wJSbVhue8NeavbgcVY
X3UcVyPMFLSDGBKhOADrHztyznMRzDkmMx83OtJH5QpNZvNcMVP0H1QHCFB/YJMY
iIVnAgMBAAEwDQYJKoZIhvcNAQEFBQADgYEAvPVq5VcF/s9TCknZaxzDNHT/c0SQ
4UKQ0y1iZbMJeWlYeMqU3o1NwGnvJ7PEeVo+Cpst0A6accbStYpiXuhuuaFGcpFl
ZJWMlMpr5GSK9uxrxwa82M69hukW4eVP4o7rZARN0o5/ilNLmy3r/vJkvFPNX5us
80t7Euud8VkyVJU=
-----END CERTIFICATE-----

Child Tickets

Change History (16)

comment:1 Changed 6 months ago by mmcloughlin

Component: - Select a componentCore Tor/Tor

comment:2 Changed 6 months ago by nickm

Milestone: Tor: 0.3.0.x-final

comment:3 Changed 6 months ago by nickm

Owner: set to nickm
Status: newassigned

comment:4 Changed 6 months ago by nickm

Keywords: 029-backport added
Status: assignedneeds_review

Hm. It looks like we started using that approach in 0196647970a91d, but I'm not at all sure that's right. I think we wanted to do something like choosing a start time at the start of a day, between this most recent midnight, and up to cert_lifetime in the past, but making sure that we don't wind up with an expiration time in the past.

My branch bug21420_029 in my public git repository [1] tries to fix this. I've marked it as a possible backport to 0.2.9, but I believe it's safe to leave this as-is in existing tors, since check_cert_lifetime_internal() is called with a 30-day future tolerance.

[1] https://gitweb.torproject.org/nickm/tor.git/commit/?h=bug21420_029&id=d839f798a5812fc81fcc5b4b06604ed08dc2e558 for the HTML version;
https://git.torproject.org/nickm/tor.git for the repository itself.

comment:5 Changed 6 months ago by arma

I think you're right that commit 0196647 is the problem here.

I suspect you're right that fixing the underlying math is the right answer.

However, I'm unable to follow the math here:

-  start_time = crypto_rand_time_range(now - cert_lifetime, now) + 2*24*3600;
+  const time_t min_real_lifetime = 2*24*3600;
+  time_t earliest_start_time = now - cert_lifetime + min_real_lifetime;
+  if (earliest_start_time < now)
+    earliest_start_time = now;
+  start_time = crypto_rand_time_range(earliest_start_time, now);

Maybe some more comments to explain what we're computing, and *why*, would help?

Looking at the origin commit, it seems maybe I wanted to say "- 2 days", not "+ 2 days". Would that resolve everything here?

Maybe I am deeply confused, but won't

+  if (earliest_start_time < now)
+    earliest_start_time = now;
+  start_time = crypto_rand_time_range(earliest_start_time, now);

trigger the assert in crypto_rand_time_range() that min < max, since we'll be passing it "now, now"?

comment:6 in reply to:  5 Changed 6 months ago by nickm

Status: needs_reviewneeds_revision

Replying to arma:

Maybe some more comments to explain what we're computing, and *why*, would help?

Okay, will do.

Looking at the origin commit, it seems maybe I wanted to say "- 2 days", not "+ 2 days". Would that resolve everything here?

I thought about it, but if you had done that, it would be possible to have the start time be "now - lifetime - 2 days". And the end time would be "start + lifetime", which would result in an already-expired certificate.

Maybe I am deeply confused, but won't

+  if (earliest_start_time < now)
+    earliest_start_time = now;
+  start_time = crypto_rand_time_range(earliest_start_time, now);

trigger the assert in crypto_rand_time_range() that min < max, since we'll be passing it "now, now"?

ohhh, yeah. Better fix that too.

comment:7 Changed 6 months ago by nickm

Status: needs_revisionneeds_review

I've commented stuff, renamed stuff, and fixed the comparison in a fixup commit on the same branch bug21420_029 .

comment:8 Changed 6 months ago by dgoulet

Status: needs_reviewneeds_revision

This comment, I can't understand the why nor the what (starting at the "instead"):

  /* Our certificate lifetime will be cert_lifetime no matter what, but if we
   * start cert_lifetime in the past, we'll have 0 real lifetime.  instead we
   * start up to (cert_lifetime - min_real_lifetime - start_granularity) in
   * the past. */

I do understand that we absolutely want "cert_lifetime" but then the explanation for how we do that is confusing to me. We "start up to" what exactly? and what is this "in the past"? Trying to understand: we use the lifetime value we want minus some values which are the minimum real lifetime (basically the minimum allowed for lifetime of a cert?) and then a "granularity" that I don't know why we use that. I see this comment Lastly, be sure to start on a day boundary. but no why.

And then the code is kind of the same thing but intuitively is reverse :).

  time_t earliest_start_time = now - cert_lifetime + min_real_lifetime + start_granularity;

The math aren't that difficult but are easily confusing especially with a lifetime concept so I would really love to see a unit test testing the boundaries. And this whole snippet of code could even be extracted in a separate function for clarity, documentation and easier testing.

comment:9 Changed 6 months ago by mmcloughlin

Should we be concerned that none of these certificates were rejected as invalid?

https://github.com/torproject/torspec/blob/b5c4adb0c2b04d90e14d75dbe8fcae4bfc738b0f/tor-spec.txt#L570-L571

comment:10 Changed 6 months ago by nickm

Keywords: review-group-16 added

comment:11 Changed 6 months ago by mmcloughlin

Looks like the certificate verification allows wiggle room at either end: 2 days on the expiry and 30 days on the start date.

https://github.com/torproject/tor/blob/67eb6470d711b36d1b855e6423ce7bbb302af834/src/common/tortls.c#L897-L900

comment:12 Changed 6 months ago by nickm

Status: needs_revisionneeds_review

I've tried to add more explanatory comments in bug21420_029, with an explanation of why we're trying to do it this way (basically: we thought it would help with fingerprinting).

comment:13 Changed 6 months ago by nickm

Reviewer: dgoulet

comment:14 Changed 6 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;!

comment:15 Changed 6 months ago by nickm

Keywords: review-group-16 removed
Milestone: Tor: 0.3.0.x-finalTor: 0.2.9.x-final

Squashed as bug21420_029_squashed ; merged to 0.3.0; marking for possible 0.2.9 backport. (I suggest no backport here.)

comment:16 Changed 2 weeks ago by nickm

Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final
Resolution: fixed
Status: merge_readyclosed

Not backporting.

Note: See TracTickets for help on using tickets.