Skip to content
This repository has been archived by the owner on Jan 4, 2019. It is now read-only.

Retire Tor circuit isolation keys #611

Closed
riastradh-brave opened this issue Jun 15, 2018 · 1 comment
Closed

Retire Tor circuit isolation keys #611

riastradh-brave opened this issue Jun 15, 2018 · 1 comment
Assignees
@darkdh @riastradh-brave
Labels
feature/tor

Comments

@riastradh-brave
Copy link
Contributor

We should decide on a heuristic by which to retire Tor circuit isolation keys in proxy_config_service_tor.cc so they don't remain in memory longer than is useful.

  • We should probably entries when the user has closed all private tabs with Tor enabled.
  • We should maybe retire entries when the user has closed all tabs derived from whichever one first opened the site.
  • We should perhaps even just retire entries when the user has closed all tabs whose first party is that site.
  • Alternatively, we should maybe retire entries after a delay: there's no benefit to keeping them longer than tor will keep the circuits around, which by default (unless we explicitly reconfigure it, which we don't) is ten minutes.
riastradh-brave referenced this issue Jun 15, 2018
@darkdh
Use network delegate to intercept every requests and apply proxy config
cc8fe17 
fix brave/browser-laptop#14390

Auditors: @bridiver, @riastradh-brave
@darkdh
Copy link
Member

darkdh commented Jun 19, 2018

10 mins delay makes more sense, after all the random password cache is for the tor circuit

darkdh added a commit that referenced this issue Jun 22, 2018
@darkdh
Retire tor circuit isolation key only when it is about to be used
f296727 
because we don't want to spend extra cycles to keep track of expired
keys

fix #611

Auditors: @riastradh-brave
darkdh added a commit that referenced this issue Jun 24, 2018
@darkdh
Retire tor circuit isolation key only when it is about to be used
5159e0c 
because we don't want to spend extra cycles to keep track of expired
keys

fix #611

Auditors: @riastradh-brave
riastradh-brave added a commit that referenced this issue Jun 25, 2018
@riastradh-brave
Use a timer to expire circuits promptly.
5cbb67d 
The timer is scheduled to run ten minutes after the last circuit that
was created.  This way, the last ten minutes of circuits are not
guaranteed to stick around in memory indefinitely.

Caveat: This doesn't _zero_ the memory, so it may still appear in
`strings /proc/N/mem`.  But it does make the memory available to be
recycled, so it's not _guaranteed_ to still appear in `strings
/proc/N/mem`.

Also, timestamp the map entries.  If we explicitly create a new map
entry for a site by requesting a new identity, the old expiry queue
entry will not delete it, but a new expiry queue entry will delete
it.  This way, circuits created by requesting a new identity are not
shorter-lived than other circuits.

We leave the old entries in the priority queue because there's no
convenient way to delete them with std::priority_queue.  In
principle, this might leak space if you repeatedly request a new
identity, but it can only leak as much space as you use by repeatedly
requesting a new identity for a maximum of ten minutes.

fix #611 real good this time

Auditors: @darkdh

Test Plan:
1. Search DDG for `what is my ip address'.
2. Record the IP address it reports.
3. Reload.
4. Confirm it's the same IP address.
5. Full-reload.
6. Confirm it's a different IP address.  Record the new IP address.
7. Wait >10min.
8. Reload.
9. Confirm it's a different IP address again.
@riastradh-brave riastradh-brave mentioned this issue Jun 25, 2018
Merged
riastradh-brave added a commit that referenced this issue Jun 25, 2018
@riastradh-brave
Use a timer to expire circuits promptly.
c22c8e7 
The timer is scheduled to run ten minutes after the last circuit that
was created.  This way, the last ten minutes of circuits are not
guaranteed to stick around in memory indefinitely.

Caveat: This doesn't _zero_ the memory, so it may still appear in
`strings /proc/N/mem`.  But it does make the memory available to be
recycled, so it's not _guaranteed_ to still appear in `strings
/proc/N/mem`.

Also, timestamp the map entries.  If we explicitly create a new map
entry for a site by requesting a new identity, the old expiry queue
entry will not delete it, but a new expiry queue entry will delete
it.  This way, circuits created by requesting a new identity are not
shorter-lived than other circuits.

We leave the old entries in the priority queue because there's no
convenient way to delete them with std::priority_queue.  In
principle, this might leak space if you repeatedly request a new
identity, but it can only leak as much space as you use by repeatedly
requesting a new identity for a maximum of ten minutes.

fix #611 real good this time

Auditors: @darkdh

Test Plan:
1. Search DDG for `what is my ip address'.
2. Record the IP address it reports.
3. Reload.
4. Confirm it's the same IP address.
5. Full-reload.
6. Confirm it's a different IP address.  Record the new IP address.
7. Wait >10min.
8. Reload.
9. Confirm it's a different IP address again.
riastradh-brave added a commit that referenced this issue Jun 26, 2018
@riastradh-brave
Use a timer to expire circuits promptly.
32f46c5 
The timer is scheduled to run ten minutes after the last circuit that
was created.  This way, the last ten minutes of circuits are not
guaranteed to stick around in memory indefinitely.

Caveat: This doesn't _zero_ the memory, so it may still appear in
`strings /proc/N/mem`.  But it does make the memory available to be
recycled, so it's not _guaranteed_ to still appear in `strings
/proc/N/mem`.

Also, timestamp the map entries.  If we explicitly create a new map
entry for a site by requesting a new identity, the old expiry queue
entry will not delete it, but a new expiry queue entry will delete
it.  This way, circuits created by requesting a new identity are not
shorter-lived than other circuits.

We leave the old entries in the priority queue because there's no
convenient way to delete them with std::priority_queue.  In
principle, this might leak space if you repeatedly request a new
identity, but it can only leak as much space as you use by repeatedly
requesting a new identity for a maximum of ten minutes.

fix #611 real good this time

Auditors: @darkdh

Test Plan:
1. Search DDG for `what is my ip address'.
2. Record the IP address it reports.
3. Reload.
4. Confirm it's the same IP address.
5. Full-reload.
6. Confirm it's a different IP address.  Record the new IP address.
7. Wait >10min.
8. Reload.
9. Confirm it's a different IP address again.
darkdh added a commit that referenced this issue Jun 27, 2018
@darkdh
Retire tor circuit isolation key only when it is about to be used
d2ac045 
because we don't want to spend extra cycles to keep track of expired
keys

fix #611

Auditors: @riastradh-brave
darkdh pushed a commit that referenced this issue Jun 27, 2018
@riastradh-brave @darkdh
Use a timer to expire circuits promptly.
cf215a0 
The timer is scheduled to run ten minutes after the last circuit that
was created.  This way, the last ten minutes of circuits are not
guaranteed to stick around in memory indefinitely.

Caveat: This doesn't _zero_ the memory, so it may still appear in
`strings /proc/N/mem`.  But it does make the memory available to be
recycled, so it's not _guaranteed_ to still appear in `strings
/proc/N/mem`.

Also, timestamp the map entries.  If we explicitly create a new map
entry for a site by requesting a new identity, the old expiry queue
entry will not delete it, but a new expiry queue entry will delete
it.  This way, circuits created by requesting a new identity are not
shorter-lived than other circuits.

We leave the old entries in the priority queue because there's no
convenient way to delete them with std::priority_queue.  In
principle, this might leak space if you repeatedly request a new
identity, but it can only leak as much space as you use by repeatedly
requesting a new identity for a maximum of ten minutes.

fix #611 real good this time

Auditors: @darkdh

Test Plan:
1. Search DDG for `what is my ip address'.
2. Record the IP address it reports.
3. Reload.
4. Confirm it's the same IP address.
5. Full-reload.
6. Confirm it's a different IP address.  Record the new IP address.
7. Wait >10min.
8. Reload.
9. Confirm it's a different IP address again.
darkdh pushed a commit that referenced this issue Jul 6, 2018
@riastradh-brave @darkdh
Use a timer to expire circuits promptly.
c59eec1 
The timer is scheduled to run ten minutes after the last circuit that
was created.  This way, the last ten minutes of circuits are not
guaranteed to stick around in memory indefinitely.

Caveat: This doesn't _zero_ the memory, so it may still appear in
`strings /proc/N/mem`.  But it does make the memory available to be
recycled, so it's not _guaranteed_ to still appear in `strings
/proc/N/mem`.

Also, timestamp the map entries.  If we explicitly create a new map
entry for a site by requesting a new identity, the old expiry queue
entry will not delete it, but a new expiry queue entry will delete
it.  This way, circuits created by requesting a new identity are not
shorter-lived than other circuits.

We leave the old entries in the priority queue because there's no
convenient way to delete them with std::priority_queue.  In
principle, this might leak space if you repeatedly request a new
identity, but it can only leak as much space as you use by repeatedly
requesting a new identity for a maximum of ten minutes.

fix #611 real good this time

Auditors: @darkdh

Test Plan:
1. Search DDG for `what is my ip address'.
2. Record the IP address it reports.
3. Reload.
4. Confirm it's the same IP address.
5. Full-reload.
6. Confirm it's a different IP address.  Record the new IP address.
7. Wait >10min.
8. Reload.
9. Confirm it's a different IP address again.
@darkdh darkdh closed this as completed in 5657527 Jul 18, 2018
darkdh added a commit that referenced this issue Jul 25, 2018
@darkdh
Retire tor circuit isolation key only when it is about to be used
c2336b6 
because we don't want to spend extra cycles to keep track of expired
keys

fix #611

Auditors: @riastradh-brave
darkdh pushed a commit that referenced this issue Jul 25, 2018
@riastradh-brave @darkdh
Use a timer to expire circuits promptly.
44f858a 
The timer is scheduled to run ten minutes after the last circuit that
was created.  This way, the last ten minutes of circuits are not
guaranteed to stick around in memory indefinitely.

Caveat: This doesn't _zero_ the memory, so it may still appear in
`strings /proc/N/mem`.  But it does make the memory available to be
recycled, so it's not _guaranteed_ to still appear in `strings
/proc/N/mem`.

Also, timestamp the map entries.  If we explicitly create a new map
entry for a site by requesting a new identity, the old expiry queue
entry will not delete it, but a new expiry queue entry will delete
it.  This way, circuits created by requesting a new identity are not
shorter-lived than other circuits.

We leave the old entries in the priority queue because there's no
convenient way to delete them with std::priority_queue.  In
principle, this might leak space if you repeatedly request a new
identity, but it can only leak as much space as you use by repeatedly
requesting a new identity for a maximum of ten minutes.

fix #611 real good this time

Auditors: @darkdh

Test Plan:
1. Search DDG for `what is my ip address'.
2. Record the IP address it reports.
3. Reload.
4. Confirm it's the same IP address.
5. Full-reload.
6. Confirm it's a different IP address.  Record the new IP address.
7. Wait >10min.
8. Reload.
9. Confirm it's a different IP address again.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@darkdh darkdh

@riastradh-brave riastradh-brave

Labels
feature/tor
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@darkdh @riastradh-brave