[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #26228 [Core Tor/Tor]: Clarify/determine specification for padding bytes, (formerly also PADDING cell) (was: Clarify/determine specification for padding bytes, PADDING cell)
#26228: Clarify/determine specification for padding bytes, (formerly also PADDING
cell)
--------------------------+------------------------------------
Reporter: dmr | Owner: dmr
Type: defect | Status: needs_review
Priority: Medium | Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor | Version:
Severity: Normal | Resolution:
Keywords: tor-spec | Actual Points:
Parent ID: | Points:
Reviewer: | Sponsor:
--------------------------+------------------------------------
Old description:
> ==== Background
> I was trying to interpret the tor-spec for padding bytes, and ending up
> asking nickm for some clarification over IRC.
> nickm suggested most of the cc'd for the ticket - I added atagar, too.
>
> ==== Unclear areas
> Here are the points that need clarification / specification:
> * spec for padding bytes does not clearly say what senders `MUST` or
> `SHOULD` do, [[https://gitweb.torproject.org/torspec.git/tree/tor-
> spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n412|only mentioning
> that padding is with 0 bytes]] or
> [[https://gitweb.torproject.org/torspec.git/tree/tor-
> spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n1480|(elsewhere)
> NUL bytes]]
> * spec for padding bytes does not say what receivers `MUST` or `SHOULD`
> do, when receiving non-zero bytes in the Cell (e.g. warn? ignore?)
> * spec is a bit inconsistent with `PADDING` cells ^^[1^^]^^[2^^]
>
> ==== Discussion: padding bytes
> For the padding bytes that are not part of `PADDING` cells, nickm offered
> the following as a non-exhaustive set of possible forward-compatible
> options:
> * "the [padding] bytes SHOULD be zero, and that implementations MUST
> ignore them"
> * "The first 8 padding bytes MUST be zero; all subsequent padding bytes
> SHOULD be randomized. Implementations MUST ignore padding bytes"
> * "All padding bytes should be randomized; implemenations MUST ignore
> unrecognized padding bytes"
> ... and mentioned that "[he doesn't] know enough of the argument in favor
> of randomization to have a very strong preference"
>
> ==== Inconsistency: `PADDING` cell payload
> (see bullet above)
>
> These references highlight the inconsistency:
>
> ^^[1^^] `PADDING: Payload is unused.` per
> [[https://gitweb.torproject.org/torspec.git/tree/tor-
> spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n469|sec 3 "Cell
> Packet format"]].
> implies 0 bytes of payload, so the rest should be padded per that
> section
> ^^[2^^] `The contents of a PADDING, VPADDING, or DROP cell SHOULD be
> chosen randomly, and MUST be ignored.` per
> [[https://gitweb.torproject.org/torspec.git/tree/tor-
> spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n1723|sec 7.2 "Link
> padding"]].
> implies the payload of a `PADDING` cell actually is the rest of the
> size of the cell, and that it SHOULD be chosen randomly
>
> The `PADDING` cells were mentioned in IRC but not discussed.
> I think a simple change to make the spec consistent between the two
> sections would be this:
> {{{
> PADDING: Payload contains random data. (See Sec 7.2)
> }}}
>
> However, given the other points here, is that correct?
New description:
**EDIT: strikethrough content below is now covered in #26870 instead**
==== Background
I was trying to interpret the tor-spec for padding bytes, and ending up
asking nickm for some clarification over IRC.
nickm suggested most of the cc'd for the ticket - I added atagar, too.
==== Unclear areas
Here are the points that need clarification / specification:
* spec for padding bytes does not clearly say what senders `MUST` or
`SHOULD` do, [[https://gitweb.torproject.org/torspec.git/tree/tor-
spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n412|only mentioning
that padding is with 0 bytes]] or
[[https://gitweb.torproject.org/torspec.git/tree/tor-
spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n1480|(elsewhere) NUL
bytes]]
* spec for padding bytes does not say what receivers `MUST` or `SHOULD`
do, when receiving non-zero bytes in the Cell (e.g. warn? ignore?)
* ~~spec is a bit inconsistent with `PADDING` cells ^^[1^^]^^[2^^]~~
==== Discussion: padding bytes
For the padding bytes that are not part of `PADDING` cells, nickm offered
the following as a non-exhaustive set of possible forward-compatible
options:
* "the [padding] bytes SHOULD be zero, and that implementations MUST
ignore them"
* "The first 8 padding bytes MUST be zero; all subsequent padding bytes
SHOULD be randomized. Implementations MUST ignore padding bytes"
* "All padding bytes should be randomized; implemenations MUST ignore
unrecognized padding bytes"
... and mentioned that "[he doesn't] know enough of the argument in favor
of randomization to have a very strong preference"
==== ~~Inconsistency: `PADDING` cell payload~~
~~(see bullet above)~~
~~These references highlight the inconsistency:~~
~~^^[1^^] `PADDING: Payload is unused.` per
[[https://gitweb.torproject.org/torspec.git/tree/tor-
spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n469|sec 3 "Cell
Packet format"]].~~
~~ implies 0 bytes of payload, so the rest should be padded per that
section~~
~~^^[2^^] `The contents of a PADDING, VPADDING, or DROP cell SHOULD be
chosen randomly, and MUST be ignored.` per
[[https://gitweb.torproject.org/torspec.git/tree/tor-
spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n1723|sec 7.2 "Link
padding"]].~~
~~ implies the payload of a `PADDING` cell actually is the rest of the
size of the cell, and that it SHOULD be chosen randomly~~
~~The `PADDING` cells were mentioned in IRC but not discussed.~~
~~I think a simple change to make the spec consistent between the two
sections would be this:~~
~~`PADDING: Payload contains random data. (See Sec 7.2)`~~
~~However, given the other points here, is that correct?~~
--
Comment (by dmr):
Replying to [comment:7 dmr]:
> I'm going to file another ticket (or tickets) for anything remaining.
Feel free to treat the above PR as the only changes under the scope of
//this// ticket, and close this ticket when the PR merged.
I've edited the description with strikethrough to clarify what is under
the scope of //this// ticket.
I've edited the title, too - would've used strikethrough if it supported
that.
Those portions of the ticket description are covered instead by ticket
#26870. (See that ticket's description for the non-strikethrough version
of its scope.)
It's also worth reiterating that the patch here doesn't resolve these -
//it may actually make things a bit less consistent in the interim//.
I tried to follow the feedback in this ticket for the patch, aside from
addressing the apparent inconsistency for [V]PADDING/DROP cells.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/26228#comment:9>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs