#25188 closed enhancement (fixed)

Spec bug in formal definition of Document in dir-spec.txt

Reported by: witchof0x20 Owned by:
Priority: Very Low Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Trivial Keywords: tor-spec, review-group-32, review-group-34
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: nickm Sponsor:

Description

I was attempting to write a parser that reads vote/consensus documents using the the formal definition here. However, I noticed an ambiguity.

Using only the formal definition, the following:

directory-signature 13241234321 12343234
-----BEGIN SIGNATURE-----
00thisisvalidbase64data12345
-----END SIGNATURE-----

Could be potentially parsed as

Document(
  Item(
    KeywordLine(
      Keyword(KeywordChar+("directory-signature")),
      WS,
      ArgumentChar+("13241234321 12343234"),
      NL
    )
  ),
  Item(
    KeywordLine(
      Keyword(KeywordChar+("-----BEGIN")),
      WS,
      ArgumentChar+("SIGNATURE-----"),
      NL
    )
  ),
  Item(
    KeywordLine(
      Keyword(KeywordChar+("00thisisvalidbase64data12345"))
      WS,
      ArgumentChar+("SIGNATURE-----"),
      NL
    )
  ),
  Item(
    KeywordLine(
      Keyword(KeywordChar+("-----END")),
      WS,
      ArgumentChar+("SIGNATURE-----"),
      NL
    )
  ),
)

When the correct parsing would be

Document(
  Item(
    KeywordLine(
      Keyword(KeywordChar+("directory-signature")),
      WS,
      ArgumentChar+("13241234321 12343234"),
      NL
    ),
    Object(
      BeginLine(
        "-----BEGIN ",
        Keyword(KeywordChar+("SIGNATURE")),
        "-----",
        NL
      ),
      Base64-encoded-data("00thisisvalidbase64data12345"),
      EndLine(
        "-----END ",
        Keyword(KeywordChar+("SIGNATURE")),
        "-----",
        NL
      ),
    ),
  ),
)

A potential change to the spec (assuming that keywords will never start with "-") that would prevent would be to replace

Keyword = KeywordChar+
KeywordChar ::= 'A' ... 'Z' | 'a' ... 'z' | '0' ... '9' | '-'

with

Keyword = KeywordStartChar KeywordChar*
KeywordStartChar ::= 'A' ... 'Z' | 'a' ... 'z' | '0' ... '9'
KeywordChar ::= 'A' ... 'Z' | 'a' ... 'z' | '0' ... '9' | '-'

Child Tickets

Attachments (1)

fix_document_ambiguity.patch (420 bytes) - added by witchof0x20 19 months ago.
Patch for this issue

Download all attachments as: .zip

Change History (14)

comment:1 Changed 19 months ago by teor

Milestone: Tor: 0.3.4.x-final
Points: 0.1
Status: newneeds_review

comment:2 Changed 19 months ago by nickm

Hm. I like this change, but it wouldn't match the current implementation. We should decide whether or not we like this better than the current behavior, and (if we want to change it) how to change it in a maximally privacy-friendly way.

The current implementation is something more like 'a keyword may not be "-----BEGIN"'.

comment:3 Changed 19 months ago by nickm

Keywords: review-group-32 added

comment:4 Changed 19 months ago by nickm

Reviewer: nickm

comment:5 Changed 19 months ago by nickm

Status: needs_reviewneeds_revision

I think, given the amount of existing code out there, we'd be better off changing the spec to match the implementation. Is that something you'd be able to write a patch for, or would you like somebody else to take this over?

Changed 19 months ago by witchof0x20

Patch for this issue

comment:6 Changed 19 months ago by witchof0x20

I attached a potential patch. How does this look?

comment:7 Changed 19 months ago by witchof0x20

I can't immediately think of an ambiguity that would be fixed by this, but would it be worth it to also say 'A Keyword cannot be "-----END"'?

comment:8 Changed 19 months ago by teor

Status: needs_revisionneeds_review

comment:9 Changed 19 months ago by nickm

Keywords: review-group-34 added

comment:10 Changed 18 months ago by dgoulet

Assigning reviewer for week 03/19.

comment:11 Changed 18 months ago by nickm

Sorry for the delay: yes, that looks fine. I'll apply it to torspec now.

comment:12 in reply to:  7 Changed 18 months ago by nickm

Replying to witchof0x20:

I can't immediately think of an ambiguity that would be fixed by this, but would it be worth it to also say 'A Keyword cannot be "-----END"'?

I think this change would make the current implementation incorrect, so I'm going to leave this part alone.

comment:13 Changed 18 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Applied to torspec as 3e533e974b15d0. Thanks again!

Note: See TracTickets for help on using tickets.