Opened 3 years ago

Closed 3 years ago

#13806 closed defect (implemented)

Count HSDir storage towards MaxMemInQueues

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay tor-hs oom security
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by nickm)

See branch "bug13806" in my public repository.

Child Tickets

Change History (10)

comment:1 Changed 3 years ago by dgoulet

Btw, we had this fellow on #tor-dev that showed me that he could in a few seconds create a valid descriptor for a specific HSDir and upload it (I didn't validate his PoC). The hs descriptor storage uses digest map which has a upper bound for memory but returns 0 if that limit is reached which means that rend_cache_store_v2_desc_as_dir() thinks that the descriptor was stored (when it fact it was not) thus not freeing the memory of the descriptor.

This could lead to a memory exhaustion attack which this ticket could mitigate!

comment:2 Changed 3 years ago by nickm

Description: modified (diff)
Status: newneeds_review

comment:3 Changed 3 years ago by nickm

whoops. I put that in the formerly-empty description, not in a comment. ah well. needs review.

comment:4 Changed 3 years ago by dgoulet

  • I would add a comment on top of this if (rend_cache_total > get_options()->MaxMemInQueues / 5) to document that if the rend cache is more than 20% of our memory limit, then cut it down to 10%. It's just not that clear right now why the "/ 5".
  • rend_cache_total - (get_options()->MaxMemInQueues / 10) );, extra space at the end?
  • I think I would put rend_cache_total_allocation += rend_cache_entry_allocation(e); in a function name something like "rend_cache_increment_allocation(e)" (which also follow the _decrement naming) in which overflow could be handled. Also, for possible future work of multi thread in tor, always good to have global variable updated in a single call site. If you prefer that way, rend_cache_get_total_allocation should at least be used.
  • last_served_cutoff += 1800;, this "1800" value, arbitrary or comes from somewhere? Maybe a define for it?

The rest looks good!

comment:5 Changed 3 years ago by nickm

Added a fixup commit. Better now?

comment:6 Changed 3 years ago by dgoulet

rend_cache_increment_allocation() no overflow check? I know it's unlikely but still for thoroughness?

Rest is great!

comment:7 Changed 3 years ago by nickm

thanks for the review! How does it look now?

(I was about to just go ahead and merge it, but then I realized that I'm doing arithmetic in C, and I'd better be careful.)

comment:8 Changed 3 years ago by nickm

Keywords: nickm-patch added

Add the nickm-patch keyword to a bunch of needs_review tickets.

comment:9 Changed 3 years ago by dgoulet

Keywords: nickm-patch removed

The only part I was still worried about is this one for overflow but I hardly see a way that it could realistically happen here unless we have actually X cache entries and that number times the size of a single object is bigger than size_t for which you'll do a couple more loops until the last served cuttoff is big enough.

bytes_removed += rend_cache_entry_allocation(ent);

So, looks good! here is my Acked-by!

comment:10 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Thanks! Squashed and merged.

Note: See TracTickets for help on using tickets.