Opened 4 months ago

Closed 4 months ago

#30958 closed defect (fixed)

Stop removing the ed25519 signature when the extra-info file is too big

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7.2-alpha
Severity: Normal Keywords: tor-bridge, tor-pt, no-backport
Cc: nickm Actual Points: 0.2
Parent ID: Points: 0.1
Reviewer: dgoulet Sponsor:

Description (last modified by teor)

In #30956, I discovered that the ed25519 signature extra-info line is
split across two chunks.

If the extra-info file gets too big, tor removes one chunk at a time. So each chunk needs to be a complete line.

Edit: but in this case, we should just stop removing the signature

Child Tickets

TicketStatusOwnerSummaryComponent
#30959closedteorAdd comments about the chunk format in extrainfo_dump_to_string()Core Tor/Tor

Change History (10)

comment:1 Changed 4 months ago by teor

Actual Points: 0.10.2
Cc: nickm added
Description: modified (diff)
Keywords: 029-backport-maybe 035-backport-maybe 040-backport-maybe 041-backport added
Milestone: Tor: 0.4.1.x-finalTor: 0.4.2.x-final
Parent ID: #30956
Status: assignedneeds_review
Summary: Add extra-info chunks as whole linesStop removing the ed25519 signature when the extra-info file is too big

It turns out that we need to split these chunks, because the ed25519 signature keyword is in the signed part of the document. So I made sure we don't remove any signature chunks.

Nick, do you think we should backport this fix any further than 0.4.1?

Please see my pull requests:

Clean merges:

comment:2 Changed 4 months ago by teor

Whoever reviews this PR should also review #30959.

comment:3 Changed 4 months ago by teor

Hmm, practracker doesn't like the master branch, but if I fix it, there will be conflicts.

comment:4 Changed 4 months ago by teor

I re-did the master merge, and cherry-picked #30959 onto it.

Please see my pull requests:

Clean merges:

Adds a commit with extre comments:

comment:5 Changed 4 months ago by dgoulet

Reviewer: dgoulet

comment:6 Changed 4 months ago by nickm

I'm thinking that this might not need a backport unless we are close to hitting it in practice.

comment:7 Changed 4 months ago by teor

Keywords: no-backport added; 029-backport-maybe 035-backport-maybe 040-backport-maybe 041-backport removed

I just did a spot-check on some large extra infos.
The largest one I found was about 4.5 KB, and Tor's limit is about 48.5 KB.
So I think 10x is an appropriate safety margin for the next 3 years.

(The biggest contributor to the size is the country list and client numbers. Each of ~250 countries can potentially take up 4 + log(client count) bytes. Even with a million clients per relay, that's only an extra 2.5 KB.)

comment:8 Changed 4 months ago by teor

So we can just review and merge the master branch here:
https://github.com/torproject/tor/pull/1136

comment:9 Changed 4 months ago by dgoulet

Status: needs_reviewmerge_ready

ACK.

comment:10 Changed 4 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged to master!

Note: See TracTickets for help on using tickets.