Count HSDir storage towards MaxMemInQueues

See branch "bug13806" in my public repository.

changed milestone to %Tor: 0.2.6.x-final

added component::core tor/tor milestone::Tor: 0.2.6.x-final oom priority::medium resolution::implemented security status::closed tor-hs tor-relay type::defect labels

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!

Trac:
Description: N/A

to

See branch "bug13806" in my public repository.
Status: new to needs_review

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

• 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!

Added a fixup commit. Better now?

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

Rest is great!

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.)

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

Trac:
Keywords: tor-relay tor-hs oom security deleted, oom, tor-relay, tor-hs, security, nickm-patch added

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!

Trac:
Keywords: oom, tor-relay, tor-hs, security, nickm-patch deleted, tor-relay tor-hs oom security added

Thanks! Squashed and merged.

Trac:
Status: needs_review to closed
Resolution: N/A to implemented

closed

moved to tpo/core/tor#13806 (closed)