Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4548 closed defect (implemented)

Implement dynamic (rakshasa) primes (part of proposal 179)

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

Description

Implement dynamic primes as part of proposal 179 (#3972).

The original name of this idea was 'Rakshasa primes'.

Child Tickets

TicketStatusOwnerSummaryComponent
#4746closedAdd a header to 'keys/dynamic_dh_params'.Core Tor/Tor

Change History (20)

comment:1 Changed 8 years ago by asn

Status: newneeds_review

Check out branch bug4548 in mytor.git.

comment:2 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:3 Changed 8 years ago by nickm

Your master branch is screwed up. All your branches these days seem to have this junk at the root:

commit 0c11230a55cfac45bfd81d3ad353638255484b77
Merge: 90ca87d 53dac6d
Author: George Kadianakis <desnacked@gmail.com>
Date:   Sat Nov 19 23:12:05 2011 +0100

    Merge branch 'master' of git://git.torproject.org/tor

commit 90ca87db46431bf38fb3886f4f398285f72676f0
Merge: af12a7a 45307ff
Author: George Kadianakis <desnacked@gmail.com>
Date:   Sat Nov 19 23:11:56 2011 +0100

    Merge branch 'master' of gitorious.org:mytor/mytor

commit 45307ff98038b2bed7263194c8d5cf56d87f4de4
Author: George Kadianakis <desnacked@gmail.com>
Date:   Mon Oct 17 22:46:44 2011 +0200

    Port managed proxy launching code to the new subprocess API.

This has nothing to do with this feature. The patch is only half as big when I ignore these commits.

This patch series also seems to include the serial number hackery, which isn't part of this feature. (It might not belong at all: to conform with the goal of being able to include user-provided certs, we probably can't actually include that at all, unless somebody has a brilliant idea.) It's also got some time fuzzing, which doesn't belong.

Exposing BIGNUM outside of crypto.c is not so good; the point of crypto.c and tortls.c is to isolate OpenSSL structures from the rest of Tor as much as possible.

When loading a new prime, we probably want to double-check that it makes a good DH group of not-too-small size. For compatiblity, also, we might want to just save the whole parameter set, not just the prime, in case we want to have it support non-2 generators as well.

DynamicDHGroups is probably a better name. There are lots of primes used for lots of stuff.

Generating groups is expensive; do we log "generating DH prime for TLS; it might take a while" before we start?

How often does this patch regenerate the DH group, if ever? "Never" is I think an acceptable answer, unless we decide that bridges need to regen it whenever their IP changes.

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

Let's try this again. Branch bug4548_take2.

Replying to nickm:

Your master branch is screwed up. All your branches these days seem to have this junk at the root:
...
This has nothing to do with this feature. The patch is only half as big when I ignore these commits.

Fixed my master. Should be fine now.

This patch series also seems to include the serial number hackery, which isn't part of this feature. (It might not belong at all: to conform with the goal of being able to include user-provided certs, we probably can't actually include that at all, unless somebody has a brilliant idea.) It's also got some time fuzzing, which doesn't belong.

Hmm, true. Didn't include these in edec9409e85ba4a8b5d0575b23046d83d7562b87 this time.

Exposing BIGNUM outside of crypto.c is not so good; the point of crypto.c and tortls.c is to isolate OpenSSL structures from the rest of Tor as much as possible.

Sloppy me! Fixed.

When loading a new prime, we probably want to double-check that it makes a good DH group of not-too-small size.

Done. If we find a corrupted stored dynamic DH modulus, what should we do? Should we unlink() the file and rewrite it with a new one? I'm currently simply logging the event. This should not happen, and if it ever happens it would be good to have the corrupted file to see the error.

For compatiblity, also, we might want to just save the whole parameter set, not just the >prime, in case we want to have it support non-2 generators as well.

Didn't do this one. Couldn't find OpenSSL functions that will store/load DH parameters to disk.
DHparams_print_fp() and d2i_DHparams() don't work together. If for some reason we ever decide to move away from 2, we can discard all 'dynamic_dh_modulus` files with the old format. It's not like relays have a special connection to their dynamic DH modulus.

DynamicDHGroups is probably a better name. There are lots of primes used for lots of stuff.

Done.

Generating groups is expensive; do we log "generating DH prime for TLS; it might take a while" before we start?

We did in log_info. We now do in log_notice.

How often does this patch regenerate the DH group, if ever? "Never" is I think an acceptable answer, unless we decide that bridges need to regen it whenever their IP changes.

We don't rotate our dynamic DH modulus atm. Do you think we should rotate it every time the bridge IP changes?

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

Replying to asn:

Let's try this again. Branch bug4548_take2.

Ok, will review.

Replying to nickm:

When loading a new prime, we probably want to double-check that it makes a good DH group of not-too-small size.

Done. If we find a corrupted stored dynamic DH modulus, what should we do? Should we unlink() the file and rewrite it with a new one? I'm currently simply logging the event. This should not happen, and if it ever happens it would be good to have the corrupted file to see the error.

I'd say, "move it aside, log the event, and write a new one."

For compatiblity, also, we might want to just save the whole parameter set, not just the >prime, in case we want to have it support non-2 generators as well.

Didn't do this one. Couldn't find OpenSSL functions that will store/load DH parameters to disk.
DHparams_print_fp() and d2i_DHparams() don't work together. If for some reason we ever decide to move away from 2, we can discard all 'dynamic_dh_modulus` files with the old format. It's not like relays have a special connection to their dynamic DH modulus.

Okay.

How often does this patch regenerate the DH group, if ever? "Never" is I think an acceptable answer, unless we decide that bridges need to regen it whenever their IP changes.

We don't rotate our dynamic DH modulus atm. Do you think we should rotate it every time the bridge IP changes?

Do we currently rotate server-side link key when the bridge IP changes? I think that's a "no", right? Also, I think that the DH parameters only get specified by the server side of the connection. If I'm right about both of those, there's no additional harm to keeping the same DH params across a bridge IP change.

comment:6 Changed 8 years ago by nickm

Remaining issues, in addition to those above, after second review:

  • If this new option is going to be on-by-default, then clients really shouldn't pay attention to it, since they shouldn't actually need to have a group at all.
  • DH_GENERATOR should probably be internal to crypto.c; I don't see a great reason to have it in crypto.
  • spelling error in crypto_set_tls_dh_prime: "moduluss"
  • Why not call crypto_store_dynamic_dh_modulus from crypto_set_tls_dh_prime immediately after generating and checking the new modulus?
  • Checking a file status right before opening it is prone to race-conditions; it's better just to open the file and see if you get an error. There should be functions in util.c to do this. (This one could get cleaned up later)
  • The branch is super-long: the "git log -p" output is over 6x as long as the actual diff with the changes in it. I think this implies I should do some rebasing and squashing pre-merge; suggestions there would be welcome.

comment:7 Changed 8 years ago by nickm

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

comment:8 Changed 8 years ago by asn

Parent ID: #3972

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

Replying to nickm:

Replying to asn:

Let's try this again. Branch bug4548_take2.

Ok, will review.

Replying to nickm:

When loading a new prime, we probably want to double-check that it makes a good DH group of not-too-small size.

Done. If we find a corrupted stored dynamic DH modulus, what should we do? Should we unlink() the file and rewrite it with a new one? I'm currently simply logging the event. This should not happen, and if it ever happens it would be good to have the corrupted file to see the error.

I'd say, "move it aside, log the event, and write a new one."

Done. I only added support for a single '.broken' file. If a relay continuously breaks its DH moduli, we will only have access to the last one.

For compatiblity, also, we might want to just save the whole parameter set, not just the >prime, in case we want to have it support non-2 generators as well.

Didn't do this one. Couldn't find OpenSSL functions that will store/load DH parameters to disk.
DHparams_print_fp() and d2i_DHparams() don't work together. If for some reason we ever decide to move away from 2, we can discard all 'dynamic_dh_modulus` files with the old format. It's not like relays have a special connection to their dynamic DH modulus.

Okay.

How often does this patch regenerate the DH group, if ever? "Never" is I think an acceptable answer, unless we decide that bridges need to regen it whenever their IP changes.

We don't rotate our dynamic DH modulus atm. Do you think we should rotate it every time the bridge IP changes?

Do we currently rotate server-side link key when the bridge IP changes? I think that's a "no", right? Also, I think that the DH parameters only get specified by the server side of the connection. If I'm right about both of those, there's no additional harm to keeping the same DH params across a bridge IP change.

I think you are right about both of those.

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

Replying to nickm:

Remaining issues, in addition to those above, after second review:

  • If this new option is going to be on-by-default, then clients really shouldn't pay attention to it, since they shouldn't actually need to have a group at all.

True. I'm only doing dynamic DH stuff to bridges now.

  • DH_GENERATOR should probably be internal to crypto.c; I don't see a great reason to have it in crypto.

Done.

  • spelling error in crypto_set_tls_dh_prime: "moduluss"

Done.

  • Why not call crypto_store_dynamic_dh_modulus from crypto_set_tls_dh_prime immediately after generating and checking the new modulus?

Because I'm stupid. Done.

  • Checking a file status right before opening it is prone to race-conditions; it's better just to open the file and see if you get an error. There should be functions in util.c to do this. (This one could get cleaned up later)

I didn't find such functions in util.c. We need a FILE* to pass to BN_print_fp().
I thought of using open() or fdopen() with O_CREAT and O_EXCL, but open() seems to be a POSIX thing.

  • The branch is super-long: the "git log -p" output is over 6x as long as the actual diff with the changes in it. I think this implies I should do some rebasing and squashing pre-merge; suggestions there would be welcome.

Hm. Let's see:
Leave edec9409 on its own, since it's Jake's stuff.
Leave 659381e0 on its own, since it introduces config.c stuff.
Group up 375e55ea, 1797e0a3, fb38e58d and 0e71be5d as code improvements.
Leave 21babd15 on its own, since it adds stuff to the manual page.
Group up 42bda231 and 8a726dd0, since they are dependent on each other.
Group up 2ef68980 and 94076d9e, as migration to crypto.c
Group up 5f3f41c2, bdeb797a, 782c907c, 7c37a664 and 1d1d5ae7 as 'minor improvements'.
Group up 4938bcc0, 1df6b5a7, b3160197, f477ddcc as 'more minor improvements'.

I'm not sure if my logic makes sense, and I'm not sure of the golden ratio between "too many commits" and "too few commits", but I hope I helped you. If you need me to create a new branch, with different commit logic, tell me.

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

Replying to asn:

Replying to nickm:

Remaining issues, in addition to those above, after second review:

  • If this new option is going to be on-by-default, then clients really shouldn't pay attention to it, since they shouldn't actually need to have a group at all.

True. I'm only doing dynamic DH stuff to bridges now.

Hm. This seems like something all servers should want. I didn't see the part that made this bridges-only; where can I find it?

  • Checking a file status right before opening it is prone to race-conditions; it's better just to open the file and see if you get an error. There should be functions in util.c to do this. (This one could get cleaned up later)

I didn't find such functions in util.c. We need a FILE* to pass to BN_print_fp().
I thought of using open() or fdopen() with O_CREAT and O_EXCL, but open() seems to be a POSIX thing.

open is supported on Windows: http://msdn.microsoft.com/en-us/library/z0kc8e3z%28v=vs.71%29.aspx

The functions I meant in util.c are start_writing_to_stdio_file and finish/abort_writing to file; they do the open+fdopen thing you want.

BTW, you *can* do this with DH parameters: d2i_DHparams and i2d_DHparams convert DH params to and from strings, and the {d2i,i2d}_DHparams_fp variants read and write DH parameters on a FILE*

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

Replying to nickm:

Replying to asn:

Replying to nickm:

Remaining issues, in addition to those above, after second review:

  • If this new option is going to be on-by-default, then clients really shouldn't pay attention to it, since they shouldn't actually need to have a group at all.

True. I'm only doing dynamic DH stuff to bridges now.

Hm. This seems like something all servers should want. I didn't see the part that made this bridges-only; where can I find it?

f477ddcc20d5fc8c130b630854947a337881cd23 "Only bother with dynamic DH moduli if we are a bridge."
If tor is not a bridge, it generates the static DH prime modulus of Apache, like it used to.

Assuming that the Apache DH prime modulus is as safe as any other randomly generated DH modulus, why would a public relay operator want it? It takes time to generate and it writes gibberish about "dynamic DH stuff" in their logs.

  • Checking a file status right before opening it is prone to race-conditions; it's better just to open the file and see if you get an error. There should be functions in util.c to do this. (This one could get cleaned up later)

I didn't find such functions in util.c. We need a FILE* to pass to BN_print_fp().
I thought of using open() or fdopen() with O_CREAT and O_EXCL, but open() seems to be a POSIX thing.

open is supported on Windows: http://msdn.microsoft.com/en-us/library/z0kc8e3z%28v=vs.71%29.aspx

Seems like I don't know how to use a search engine!
OK will use open() then.

The functions I meant in util.c are start_writing_to_stdio_file and finish/abort_writing to file; they do the open+fdopen thing you want.

Will check them out.

BTW, you *can* do this with DH parameters: d2i_DHparams and i2d_DHparams convert DH params to and from strings, and the {d2i,i2d}_DHparams_fp variants read and write DH parameters on a FILE*

I want to do #4549 first, but I'll try to do this as well.

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

Replying to asn:

Replying to nickm:

Replying to asn:

Replying to nickm:

Remaining issues, in addition to those above, after second review:

  • If this new option is going to be on-by-default, then clients really shouldn't pay attention to it, since they shouldn't actually need to have a group at all.

True. I'm only doing dynamic DH stuff to bridges now.

Hm. This seems like something all servers should want. I didn't see the part that made this bridges-only; where can I find it?

f477ddcc20d5fc8c130b630854947a337881cd23 "Only bother with dynamic DH moduli if we are a bridge."
If tor is not a bridge, it generates the static DH prime modulus of Apache, like it used to.

Assuming that the Apache DH prime modulus is as safe as any other randomly generated DH modulus, why would a public relay operator want it?

Assuming that an adversary isn't distinguishing based on using the apache modulus, there is no point in this branch at all as far as I can tell.

Assuming that an adversary _is_ distinguishing based on the apache modulus, it's nice to get anti-fingerprinting features into the main Tor protocol. (This is one reason we did the v3 handshake as part of the main Tor protocol, rather than as a special bridge-only thing. This is also the reason we changed _everybody's_ DH parameters to the apache modulus, instead of only the bridges.)

comment:14 Changed 8 years ago by asn

Done!
Please check my branch again.

[You can squash f28014bf and 055d6c01 into one.]

comment:15 Changed 8 years ago by nickm

Seems good, I think. When I'm back online later in the day I plan to:

  • take another more careful review pass
  • squash as recommended
  • rename a couple of functions (stuff that stores/loads DHparams shouldn't be called "store modulus"),
  • Have a look at the "store stuff to a file" API set to see whether it makes sense, or if we can do with fewer functions and more flags.

comment:16 Changed 8 years ago by nickm

Hm. On second thought, this kind of squashing doesn't make sense. It seems like you're only suggesting grouping up changes into "here's a commit with a lot of changes". What I had in mind was more like "Commit X does something , and then later commit Y changes how X did it in the first place: let's regard Y as a fixup for X." Like, how f477ddcc20d5fc8c1 is purely a replacement for fa013e1bc520e11.

This is probably too complicated for me to untangle well atm, so I'm going to merge this branch as-is (assuming it tests out ok for me). Next time, please consider using git's "--squash" and --fixup" as you make commits that alter earlier commits.

comment:17 Changed 8 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Works for me; not seeing any new bugs. Merging.

comment:18 Changed 8 years ago by arma

asn: next time you're writing a changes file, please make sure it references the trac number. Thanks!

comment:19 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:20 Changed 7 years ago by nickm

Component: Tor BridgeTor
Note: See TracTickets for help on using tickets.