Last Call Review of draft-ietf-cellar-ebml-09
review-ietf-cellar-ebml-09-genart-lc-sparks-2019-02-20-00

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-cellar-ebml-09
Reviewer: Robert Sparks
Review Date: 2019-02-20
IETF LC End Date: None
IESG Telechat date: Not scheduled for a telechat

Summary: Not ready for publication as a Proposed Standard

Note that this is really an early review - IETF LC has not yet been issued for
this draft.

Issues:

It's not clear that this group is chartered to produce a general purpose
binary equivalent to XML. Instead, it appears to be chartered to document FFV1
and Matroska. EBML as it is currently used for those things needs to be
documented, but rather than try to make it into a format that other things
besides the work of this group appears out of scope. If I'm correct, then this
document shouldn't need to create an IANA registry - it need only document
what the group needs (and if the group needs more later, it can update this
document). The abstract and introduction would need to be adjusted to scope
the purpose of the format to supporting the work of this group. My review
assumes a scope of "documenting these existing formats" rather than providing a
general purpose markup language. If I'm wrong and this group is chartered to
produce an alternative for other protocols to use, this needs review from
people who are more expert in that kind of representational design than me.

The document is very hard to absorb given its current structure. It requires
multi-pass reading to figure out. While I like the idea of leaping directly
into security considerations, the draft really needs an introduction that lays
out the high-level structural concepts more linearly. A diagram of the
inherent tree structure would help reinforce the definitions. Showing the
defined types (13.1.5.9) in that diagram would be useful. A shorter
introduction to VINT (not the details of its structure, but why you have such
a construct in the first place) would also help. Also consider examples
showing the end of Unknown-Sized elements is determined (The description in
the first partial paragraph at the top of page 12 is complicated). It would
also help to introduce the idea of null terminated strings here to smooth the
forward reference from section 8.4 to section 9. CRC-32 elements aren't
defined until towards the end of the document, but non-trivial things are done
with them throughout. Having those introduced, at least conceptually, much
earlier would be good.

The third paragraph of the Security Considerations section (including the
bullets) appears to say its ok to treat the following invalid things as valid.
If that's the case, why are they invalid in the first place? I think for some
of them, perhaps text has drifted and they are no longer invalid, but just
to be avoided when it is reasonable to do so? For those that really are invalid,
some text should be added describing _why_ they are ok to accept.

In 5.4 you claim the Size column in the first table refers to the size of the
"VINT_DATA" in bits. The values there are not how many bits of VINT_DATA you
have. If that's what you really want to show, the values should be 7, 14, etc.
rather that 2^7, 2^14. If you mean for the column to show how large an integer
can be represented by a VINT with this octet length, the values need to be
2^7-1, 2^14-1, etc. It would be helpful to reduce the width of the first
columns so that the last value in the Representation column does not wrap.

The last sentence of the 2nd paragraph of section 7 does not parse. Please
simplify it.

There is odd notation at the end of page 12. What does 2^(2_7) mean?

At section 8, the second sentence scans very badly. Consider breaking it into
two or more sentences so that there is no ambiguity around the concept being
defined. As written, its too easy to misunderstand that the element type
defines a concept that describes definition.

At section 11, you say that EBML Documents MUST NOT contain any data that is
not part of an EBML Element, but I think other sections allow that to happen
in data rewrite situations?

Why is the unique and persistent requirement SHOULD and not MUST at the top
of page 20?

At the definition of name (13.1.5.1), please talk about why this particular set
of characters were chosen.

Please expand on "the risk of false positives" in 13.1.5.3. The risk here is not
obvious from the current text.

At 13.1.5.6.1 (a section nesting 5 deep is a document structure warning sign
by itself), in the first bullet I suggest you say "are of the type of the
element" instead of "are integers or floats". What you have now lets me put an
integer on one side and a float on the other.

The range representations in 13.1.5.6.1 don't allow representing 0<=x<1, 0<x<1,
or any other range with one or more open ends. Why isn't that problematic?

The element descriptions in section 13 have range values that don't conform to
the range represnations in 13.1.5.6.1. See 13.2.5 for example (where the range
is "not 0").

The last sentence of section 13.1.12 looks like a security problem. Why would
you ever use _any_ elements containing a CRC-32 element that's bad? Some
discussion near the top of the document about what these CRC-32 elements are
supposed to accomplish seems necessary. More needs to be said about the similar
security issues at section 14.

At 13.2.9, what does "read upper values" mean?

Nits:

The definition of Master Element is not a definition - it describes one
propery Master Elements have. AFAICT, the concept isn't concisely defined
anywhere in the document.

The definition of Void Element doesn't need to say "damaged data". It
coule be perfectly good data that you are overwriting.

I suggest removing the word "official" from the Element Name definition.

The concept of an Identically Recurring Element is not clear throughout
the document. Some section should give a complete definition, and the
places that call out whether exceptions (such as where the security
considerations discusses Identically Recurring Elements with invalid
CRC-32 elements) are still Identically Recurring Elements or not.
What happens if the values in the invalid CRC-32 elements are different
from each other?

At section 8.7, second paragraph, first sentence. The sentence is long, and
"equals to" looks like a problem spot for non-native-English readers. Perhaps
"set to the same value as"?

Micronits:

Page 9: s/four octet./four octets./

Expanding the numbers in section 8.1 and 8.2 isn't really helpful. It would
be better to just say 2^64-1.