[tor-bugs] #26228 [Core Tor/Tor]: Clarify/determine specification for padding bytes, PADDING cell

[Tor Bug Tracker & Wiki]

#26228: Clarify/determine specification for padding bytes, PADDING cell
--------------------------+------------------------
 Reporter:  dmr           |          Owner:  (none)
     Type:  defect        |         Status:  new
 Priority:  Medium        |      Milestone:
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:  tor-spec      |  Actual Points:
Parent ID:                |         Points:
 Reviewer:                |        Sponsor:
--------------------------+------------------------

Comment (by dmr):

 > ==== Inconsistency: `PADDING` cell payload

 Responding on the `PADDING` cells / family. I don't think I can further
 add to the design discussion on the padding portion of the other cells.
 //(I now feel like I might have mixed two independent topics into a single
 ticket. Oops. They still need to roughly not conflict with each other,
 though.)//

 I don't want to get pedantic here, but the terminology is a bit confusing
 throughout.
 I'm looking to make it easier to understand (and thus implement).


 > Unless the contents of a cell include payload and padding.

 Thanks for pointing that out! I think we should make the meaning of
 "contents" here clearer. That's what tripped me up and seems to be the
 biggest point of misunderstanding for me.

 Right now, the spec's terminology mixes a bit between contents, payload,
 and padding. It's it bit hard to figure out what is what.

 Since "contents" is a pretty general term, and is used in the spec in
 different scopes (along with the verb "contains" which often implies
 contents), **it might be better for section 7.2 to //explicitly// say what
 SHOULD be randomized.**


 > No, the payload of a cell is the data that an implementation SHOULD
 parse. Calling the [V]PADDING and DROP content a "payload" adds to the
 confusion, because implementations SHOULD NOT parse the content of these
 cells. (It's meaningless.)

 I agree about adding to the confusion. I think some amount of confusion
 will remain in the spec, because of how "payload" is introduced (see bit
 below). In the `RELAY_DROP` case, for instance, the `Data` is probably the
 "contents" that section 7.2 refers to.

 So overall, again, I think it's probably best to explicitly say what
 fields SHOULD be randomized.

 I'm happy to write a patch for this if you think that approach is best.
 Please let me know!

 ==== English is hard (feel free to skip over this, or read for additional
 details)

 Unfortunately the word "contents" could be interpreted as any of the
 following:
 * payload only
 * padding only
 * payload and padding
 * payload, padding, and CircID (for `[V]PADDING` cells)


 > Instead, let's say that these cells have zero-length payloads, and then
 define padding bytes consistently.

 I think that would be harder in practice, based on the way "Payload" is
 introduced, which makes it a bit confusing already.

 `Payload (padded with 0 bytes)` is defined to be `[PAYLOAD_LEN bytes]` in
 fixed-width cells or `Payload` to be `[Length bytes]` in the variable-
 length cells.

 Note that in the latter case, the `Payload` bytes of a `VPADDING` cell is
 exactly what teor said shouldn't be considered payload: padding.

 It gets a bit more confusing when considering `RELAY_DROP` cells, which
 have the fixed-width cell "layer" of payload, and within that, the `Data`
 of the `RELAY_DROP` cell.

 So overall, I think the easiest thing is to define what SHOULD be
 randomized for each of the cell types, in the context of section 7.2.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/26228#comment:2>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online