Opened 8 years ago

Closed 2 years ago

#4549 closed defect (wontfix)

Implement user-defined certificate strings through torrc (part of the proposal 179 efforts)

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-bridge
Cc: Actual Points:
Parent ID: #3972 Points:
Reviewer: Sponsor:

Description

Implement torrc options that allow users to specify their own values in certificate fields (like 'CN', 'C', 'ST', 'O', the lifetime of the cert., and the serial number).

This is *not* documented in proposal 179.

Child Tickets

Change History (28)

comment:1 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:2 Changed 8 years ago by asn

Status: newneeds_review

Check out branch bug4549 in git://gitorious.org/mytor/mytor.git.

comment:3 Changed 8 years ago by nickm

This is based on the same messed-up master as #4548 was, and is all mixed up with #4548 changes. Next time I'd prefer have something where it's easy to tell which actual commits I'm supposed to review. As near as I can tell here, it's supposed to be everything in bug4549 starting with fb8b31c3fb26d03.

Notes on review:

  • wrt the tor_tls_set_custom_cert_strings interface: There is a great quote from Alan Perlis: "If you have a procedure with ten parameters, you probably missed some." These arguments probably want to turn into a single argument of type tor_cert_cfg_t, to be defined in tortls.h
  • Customizeable start and end time and serial number: these are probably a mistake. Serial number needs to be different for every generated cert with the same issuer; start time and end time seem like they're begging you to have your Tor not work because you made your own cert expire. Instead let's go with certs that expire after a year or something; that seems to be the standard lifetime.
  • tor_X509_name_new now has tons of copy-and-paste code. That's not so good; let's use a function or a macro to make it shorter.
  • There's no need to use BN_pseudo_rand to get a random number; just use Tor's crypto_rand_int or whatever it's called.
  • The documentation for tor_decorate_cert makes no sense to me. How does stuff "float around" ? What does it mean to "decorate" a cert? Which certs get decorated? Why is this a separate function?
  • Looks like the covert-channel-in-the-serialnumber junk is still in here too; it'll need to come out.

Questions/ideas:

  • It looks like you apply the issuer name stuff to be the name of EVERY issuer, and the subject name to be subject name on EVERY cert? That isn't right, if so. This stuff only wants to apply to the initial presented-in-cleartext cert, which now wants to be different from the cert chain presented after in the v2 or v3 handshake. The cert chain probably wants to stay unchanged.
  • How have you tested this?
  • Maybe we should have the ability to store these and the corresponding link keys in the datadir. If so, that should probably get done along with the "load a custom cert from disk" work.

comment:4 Changed 8 years ago by nickm

(Also, I can't remember if I saw a changes file)

comment:5 Changed 8 years ago by asn

Parent ID: #3972

comment:6 in reply to:  3 ; Changed 8 years ago by asn

Replying to nickm:

  • Customizeable start and end time and serial number: these are probably a mistake. Serial number needs to be different for every generated cert with the same issuer; start time and end time seem like they're begging you to have your Tor not work because you made your own cert expire. Instead let's go with certs that expire after a year or something; that seems to be the standard lifetime.

You think so? I see the validity options as a poor man's solution to #4390#comment:1
and a much better way to do "start time fuzzing".

We can enforce correct use by doing appropriate checks.
I'm already checking that CertEndDate > CertStartDate. We can also check that CertEndDate and CertStartDate are bigger than time(NULL).
We can also add a LOG_WARN message in the startup that "the options you set are advanced options.".

Of course, they should also be considered advanced options (as the rest of the TLSCert* options are). My point is that if at some point censors find a way to detect our serial numbers, and *don't* have a way to check that a relay's serial number has been the same, bridge operators can circumvent blocking by setting their own SNs. The same goes for validity times.

I don't see us losing much by implementing this, and we increase our versatility.

  • tor_X509_name_new now has tons of copy-and-paste code. That's not so good; let's use a function or a macro to make it shorter.
  • There's no need to use BN_pseudo_rand to get a random number; just use Tor's crypto_rand_int or whatever it's called.
  • The documentation for tor_decorate_cert makes no sense to me. How does stuff "float around" ? What does it mean to "decorate" a cert? Which certs get decorated? Why is this a separate function?
  • Looks like the covert-channel-in-the-serialnumber junk is still in here too; it'll need to come out.

Questions/ideas:

  • It looks like you apply the issuer name stuff to be the name of EVERY issuer, and the subject name to be subject name on EVERY cert? That isn't right, if so. This stuff only wants to apply to the initial presented-in-cleartext cert, which now wants to be different from the cert chain presented after in the v2 or v3 handshake. The cert chain probably wants to stay unchanged.

I agree, I'll try to implement it properly.. I did it like this, because I saw the cname, cname_sign stuff getting applied to all certificates, and I didn't want to complicate the implementation.

  • How have you tested this?

I ran wireshark and checked out the TLS certificates. I also ran a local relay/client and made sure that nothing is screwed up in normal Tor operation. I'll do more checks in the latter scenario.

  • Maybe we should have the ability to store these and the corresponding link keys in the datadir. If so, that should probably get done along with the "load a custom cert from disk" work.

Yes, this can be done along with #4550.

comment:7 in reply to:  6 Changed 8 years ago by nickm

Replying to asn:

Replying to nickm:

  • Customizeable start and end time and serial number: these are probably a mistake. Serial number needs to be different for every generated cert with the same issuer; start time and end time seem like they're begging you to have your Tor not work because you made your own cert expire. Instead let's go with certs that expire after a year or something; that seems to be the standard lifetime.

You think so?

I don't say stuff I don't think on trac tickets :)

I see the validity options as a poor man's solution to #4390#comment:1

and a much better way to do "start time fuzzing".

We can get a much better solution by keeping certs around on disk, so we actually have start/end times that mean something.

We can enforce correct use by doing appropriate checks.
I'm already checking that CertEndDate > CertStartDate. We can also check that CertEndDate and CertStartDate are bigger than time(NULL).
We can also add a LOG_WARN message in the startup that "the options you set are advanced options.".

Of course, they should also be considered advanced options (as the rest of the TLSCert* options are). My point is that if at some point censors find a way to detect our serial numbers, and *don't* have a way to check that a relay's serial number has been the same, bridge operators can circumvent blocking by setting their own SNs. The same goes for validity times.

Probably we should look into how self-signed certs' serial numbers are generated, and do ours like that instead. AFAICT, openssl's tools for making certs don't ask you for your own serial number: they instead just generate a random 64-bit value.

I don't see us losing much by implementing this, and we increase our versatility.

I do. It will make our certs follow a very weird pattern (changing every day, but having the same start and end time and serial number each time), and will make nodes suddenly stop working when the end date passes without people reconfiguring their Tor, and will force people to understand what an X509 serial number is.

Also, once we _do_ have a real solution to this problem (probably involving storing the same cert to disk and using it longer-term), we'll still be stuck with the code we added to support these options indefinitely.

  • It looks like you apply the issuer name stuff to be the name of EVERY issuer, and the subject name to be subject name on EVERY cert? That isn't right, if so. This stuff only wants to apply to the initial presented-in-cleartext cert, which now wants to be different from the cert chain presented after in the v2 or v3 handshake. The cert chain probably wants to stay unchanged.

I agree, I'll try to implement it properly.. I did it like this, because I saw the cname, cname_sign stuff getting applied to all certificates, and I didn't want to complicate the implementation.

  • How have you tested this?

I ran wireshark and checked out the TLS certificates. I also ran a local relay/client and made sure that nothing is screwed up in normal Tor operation. I'll do more checks in the latter scenario.

Okay. in the next round of tests, make sure that the 0.2.2 Tor clients and servers can interoperate with clients and servers running this patched code.

comment:8 in reply to:  3 ; Changed 8 years ago by asn

Replying to nickm:

  • It looks like you apply the issuer name stuff to be the name of EVERY issuer, and the subject name to be subject name on EVERY cert? That isn't right, if so. This stuff only wants to apply to the initial presented-in-cleartext cert, which now wants to be different from the cert chain presented after in the v2 or v3 handshake. The cert chain probably wants to stay unchanged.

Hm, what about the v1 handshake? There, the 'presented-in-cleartext certificate' has to be passed along with the 'identity certificate'. This means that in the v1 case, we have to customize the 'identity certificate' with the user-defined certificate strings so that the certificate chain remains valid with:
cleartext_cert.issuerDNs == id_cert.issuerDNs == id_cert.subjectDNs

I don't consider this necessarily wrong, since it seems logical from a PKI PoV.

If we do this, the 'link certificate' is equal to the 'presented-in-cleartext certificate', since we will also need to customize the 'link certificate' to form a valid certificate chain with the customized 'identity certificate' during the v2 renegotiation.
This probably means that we don't need an extra 'presented-in-cleartext certificate' and we can simply customize our 'link certificate'.

If we don't want to do this, we will have to either kill the v1 handshake (we probably don't like this), or we will have to change the logic with which we create our certificate chains since OpenSSL will not present a non-valid certificate chain during the SSL handshake [0]. Both solutions seem *much* dirtier than customizing all the certificates that need to be customized.

I believe that if a user has provided certificate strings, we have to customize all our SSL context certificates so that they make sense from a PKI point of view (and so that all of our handshakes work correctly.). What do you think?

[0]: Instead of doing X509_STORE_add_cert() we will have to do SSL_CTX_add_extra_chain_cert(), so that the invalid cert. chain gets presented during v1. And in the case of a v2, we will have to dive into the guts of OpenSSL and remove the cert from the SSL context ourselves, before re-adding it in the renegotiation stage. Sounds *very* ugly.
http://osdir.com/ml/encryption.openssl.cvs/2003-02/msg00032.html

comment:9 in reply to:  8 ; Changed 8 years ago by nickm

Replying to asn:

Replying to nickm:

  • It looks like you apply the issuer name stuff to be the name of EVERY issuer, and the subject name to be subject name on EVERY cert? That isn't right, if so. This stuff only wants to apply to the initial presented-in-cleartext cert, which now wants to be different from the cert chain presented after in the v2 or v3 handshake. The cert chain probably wants to stay unchanged.

Hm, what about the v1 handshake? There, the 'presented-in-cleartext certificate' has to be passed along with the 'identity certificate'. This means that in the v1 case, we have to customize the 'identity certificate' with the user-defined certificate strings so that the certificate chain remains valid with:
cleartext_cert.issuerDNs == id_cert.issuerDNs == id_cert.subjectDNs

I don't consider this necessarily wrong, since it seems logical from a PKI PoV.

If we do this, the 'link certificate' is equal to the 'presented-in-cleartext certificate', since we will also need to customize the 'link certificate' to form a valid certificate chain with the customized 'identity certificate' during the v2 renegotiation.
This probably means that we don't need an extra 'presented-in-cleartext certificate' and we can simply customize our 'link certificate'.

If we don't want to do this, we will have to either kill the v1 handshake (we probably don't like this), or we will have to change the logic with which we create our certificate chains since OpenSSL will not present a non-valid certificate chain during the SSL handshake [0]. Both solutions seem *much* dirtier than customizing all the certificates that need to be customized.

I believe that if a user has provided certificate strings, we have to customize all our SSL context certificates so that they make sense from a PKI point of view (and so that all of our handshakes work correctly.). What do you think?

Ah, okay. I think I'm convinced that all the certs probably need to have new names in them, if we're going to keep v1 alive (which we are for now). But part of my original point stands, though: when there's a 2-cert chain, we need to have the issuerDN of the link cert match with the subjectDN of the identity cert. I *believe* that your original patch did that oddly, and instead gave all certs the same issuerDN and all certs the same subjectDN.

So here's what I'd suggest:

Let the user configure an issuerDN "I" and a subjectDN "S".

When there's just one cert presented in plaintext, it should probably look self-signed. So it should have issuerDN=subjectDN=S. When there are two certificates, the link cert should have issuerDN=I and subjectDN=S, and the identity cert needs issuerDN=subjectDN=I.

Does that work out?

comment:10 in reply to:  9 Changed 8 years ago by asn

Replying to nickm:

So here's what I'd suggest:

Let the user configure an issuerDN "I" and a subjectDN "S".

When there's just one cert presented in plaintext, it should probably look self-signed. So it should have issuerDN=subjectDN=S. When there are two certificates, the link cert should have issuerDN=I and subjectDN=S, and the identity cert needs issuerDN=subjectDN=I.

Does that work out?

I think it's doable. It will require us to define two extra certificates. One 'self-signed presented-in-cleartext' certificate to be used in v2/v3, and one 'signed-by-the-identity-certificate presented-in-cleartext' certificate to be used in v1.

I'll try to do it today.

comment:11 Changed 8 years ago by asn

Nick, do you have any tips on testing the v2 handshake?

I'm trying to peak into the v2 handshake by using s_client(1SSL) and renegotiating, but I can't seem to be able to trigger the server sending me his certificate chain. I always get a single certificate out of the server during the renegotiation, and no certificate request.

My s_client command is:
openssl s_client -cert ./le_cert.pem -cipher ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES256-SHA:DHE-DSS-AES256-SHA:ECDH-RSA-AES256-SHA:ECDH-ECDSA-AES256-SHA:AES256-SHA:ECDHE-ECDSA-RC4-SHA:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-RC4-SHA:ECDHE-RSA-AES128-SHA:DHE-RSA-AES128-SHA:DHE-DSS-AES128-SHA:ECDH-RSA-RC4-SHA:ECDH-RSA-AES128-SHA:ECDH-ECDSA-RC4-SHA:ECDH-ECDSA-AES128-SHA:RC4-MD5:RC4-SHA:AES128-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:EDH-DSS-DES-CBC3-SHA:ECDH-RSA-DES-CBC3-SHA:ECDH-ECDSA-DES-CBC3-SHA:DES-CBC3-SHA -msg -showcerts -connect localhost:6666

comment:12 Changed 8 years ago by nickm

At the time, all I had in mind was testing it with 0.2.2.x as a client and making sure it worked.

Does that s_client line work with 0.2.2.x as a server, btw? If so, there could be a problem, possibly related to #4587 .

comment:13 in reply to:  12 Changed 8 years ago by asn

Replying to nickm:

At the time, all I had in mind was testing it with 0.2.2.x as a client and making sure it worked.

It seems to work, but I would like to make sure that everything looks as expected inside the renegotiation.

Does that s_client line work with 0.2.2.x as a server, btw? If so, there could be a problem, possibly related to #4587 .

That s_client line does not work with any kind of a relay. I tried random relays off the consensus, and maint-0.2.2 as well.

comment:14 Changed 8 years ago by asn

(I'm testing by setting up the bug4549 relay as a bridge and then in the client side I'm giving the bridge address, its identity fingerprint and UseBridges 1.)

comment:15 Changed 8 years ago by asn

Found the bug of comment:11. Filed it at #4591.

comment:16 Changed 8 years ago by asn

Pushed my progress in branch bug4549_progress.

It seems to work with all link protocols, but I'll do more testing tomorrow.

I haven't done SIGHUP support yet and manual page documentation (both are already done in bug4549.).

I won't have too many hours to work on this tomorrow, but I'll try to test it and add SIGHUP-support and manual documentation.

comment:17 Changed 8 years ago by nickm

Okay; please let me know where stuff stands before you depart. Also, it's okay to try to find a good stopping point and say "This is as much progress as I can make; if somebody can finish it, great!"

comment:18 Changed 8 years ago by asn

OK, I believe it's ready™.

Check branch bug4549_take2 in my gitorious repository. I tested it against maint-0.2.2 running v1/v2 link protocols and against the latest master with v3.

Apart from reviewing the code, please double test it with all our link protocols.

comment:19 Changed 8 years ago by nickm

Ugh. The "BUG BUG BUG BUG BUG BUG" thing makes me really nervous here. As you suspected the "too big" commit does indeed do a whole lot of stuff in a way that makes it hard to review. I will try to make sense of the last few commits here, and figure out whether to take this, but it might need to slip to 0.2.4 at this point unless it suddenly all looks great when I next examine it.

comment:20 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

Yeah, I'm really sorry, but this area of the code has gotten hairy enough, and this patch has gotten tricky enough, that I think we really need to postpone it to 0.2.4.x.

comment:21 in reply to:  20 Changed 8 years ago by asn

Replying to nickm:

Yeah, I'm really sorry, but this area of the code has gotten hairy enough, and this patch has gotten tricky enough, that I think we really need to postpone it to 0.2.4.x.

OK.
I'll clean up the branch. I'll update this ticket when it's ready.

comment:22 Changed 8 years ago by nickm

Status: needs_reviewneeds_revision

comment:23 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:24 Changed 7 years ago by nickm

Component: Tor BridgeTor

comment:25 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

comment:26 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

Moving nearly all needs_revision tickets into 0.2.6, as now untimely for 0.2.5.

comment:27 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: unspecified

Putting this into unspecified, with a lean towards wontfix. Pluggable transports seem to obviate the use case for this.

comment:28 Changed 2 years ago by nickm

Resolution: wontfix
Severity: Normal
Status: needs_revisionclosed

Superseded by the existence of PTs.

Note: See TracTickets for help on using tickets.