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

Tor Bug Tracker & Wiki blackhole at torproject.org
Thu Jul 19 01:01:35 UTC 2018


#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


More information about the tor-bugs mailing list