Subject: [Cellar] Benjamin Kaduk's Discuss on draft-ietf-cellar-ebml-15: (with DISCUSS and COMMENT)

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Thu, 19 Dec 2019 07:35:09 -0800

Benjamin Kaduk has entered the following ballot position for
draft-ietf-cellar-ebml-15: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-cellar-ebml/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

Section 7.7 says:

   A Master Element MUST declare a length in octets from zero to
   VINTMAX.  The Master Element MAY also use an unknown length.  See
   Section 6 for rules that apply to elements of unknown length.

but the second sentence contradicts the immediately prior MUST.  We need
to resolve the internal inconsistency.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

I support Adam's Discuss regarding the Abstract.

Section 2

   "Parent Element": A relative term to describe the "Master Element"
   which contains a specified element.  For any specified "EBML Element"
   that is not at "Root Level", the "Parent Element" refers to the
   "Master Element" in which that "EBML Element" is contained.

It sounds like this is intended to be "directly" or "immediately"
contained (in order to be unique), right?  If not, then it sould be
''refers to a "Master Element" in which [...]''

Section 4.1

   Each Variable Size Integer begins with a VINT_WIDTH which consists of
   zero or many zero-value bits.  The count of consecutive zero-values
   of the VINT_WIDTH plus one equals the length in octets of the
   Variable Size Integer.  [...]

Does the following attempted rewording change the meaning?

%  Each Variable Size Integer begins with a VINT_WIDTH which consists of
%  zero or more bits set to zero.  The length in octets of the entire
%  Variable Size Integer is determined as one plus the number of
%  consecutive bits set to zero.

(I find the current formulation rather hard to parse.)

Section 6.2

    | "\root\level1\level2\<global>"     | Global Element cannot be   |
    |                                    | assumed to have this path, |
    |                                    | while parsing "elt" it can |
    |                                    | only be a child of "elt"   |

Cannot be assumed by who/what?  My brain is trying to parse this as just
"cannot assume this path".

Section 7.5

Should we say anything about termination of a UTF-8 string needing to
still result in valid UTF-8 (i.e., not insert NULs in the middle of a
codepoint)?

Section 7.7

   stored within Master Elements SHOULD only consist of EBML Elements
   and SHOULD NOT contain any data that is not part of an EBML Element.

When might this SHOULD (NOT) be violated?

Section 8.2

   part of an EBML Element.  This document defines precisely which EBML
   Elements are to be used within the EBML Header, but does not name or

(for EBMLVersion 1 only, right?)

Section 11.1

   Element; for example matroska or webm (see Section 11.2.6).  The
   DocType value for an EBML Document Type MUST be unique and
   persistent.

It might be appropriate to refer to Section 17.2 and/or the IANA
registry for DocType values, here.

   EBMLVersion to only support a value of "1".  If an EBML Schema adopts
   the EBML Header Element as-is, then it is not required to document
   that Element within the EBML Schema.  If an EBML Schema constrains

Does "as-is" imply some level of future-compatibility/extensibility for
when EBMLVersions other than "1" are defined?

Section 11.1.1

It's a little amusing that we bother to provide "default" attributes
when the "range" attribute uniquely determines the allowed value.

Section 11.1.4

   Each "<element>" defines one EBML Element through the use of several
   attributes that are defined in Section 11.1.3.  EBML Schemas MAY

I think this makes more sense as "Section 11.1.5".

Section 11.1.5.2

This ABNF seems to only allow "direct" recursion where element <x>
appears directly inside element <x>, without any intermediate elements.
I assume that's the intent, though it would be surprising in a
general-purpose markup language.

   In some cases the EBMLLastParent part of the path is an
   EBMLGlobalParent.  A path with a EBMLGlobalParent defines a
   Section 11.3.  Any path that starts with the EBMLFixedParent of the

That second sentence doesn't parse.

   As an example, a "path" of "1*(\Segment\Info)" means the element Info
   is found inside the Segment elements at least once and with no
   maximum iteration.  An element SeekHead with path
   "0*2(\Segment\SeekHead)" may not be found at all in its Segment
   parent, once or twice but no more than that.

The way this text is written makes me want to interpret the path
occurence counts more like the (regular) minOccurs/maxOccurs element
attributes, as opposed to applying to the path components to get to the
specific element in question.

Section 11.1.9.2

   <element name="Item" path="1*1(\Items)" id="0x4025" type="master"
     minOccurs="1" maxOccurs="1">
     <documentation lang="en" purpose="definition">
       A set of items.

Is this "name" supposed to be "Item" or "Items"?

Section 11.1.10-11.1.12

I'm not sure I have a full understanding of how <restriction>/<enum> are
used; perhaps a reference to the corresponding XML behavior is in order?

Section 11.1.13-11.1.14

The <extention type="..."> usage seems underspecified.

Section 11.1.15

       <xs:attribute name="path" use="required">
         <!-- <xs:simpleType>
           <xs:restriction base="xs:integer">
             <xs:pattern value="[0-9]*\*[0-9]*()"/>
           </xs:restriction>
         </xs:simpleType> -->
       </xs:attribute>

Why do we include this commented-out snippet?

       <xs:attribute name="unknownsizeallowed" type="xs:boolean"/>
       <xs:attribute name="recurring" type="xs:boolean"/>

Don't we effectively set default values for these two in the prose
description?

Section 11.1.16

   Identically Recurring Elements SHOULD include a CRC-32 Element as a
   Child Element; this is especially recommended when EBML is used for
   long-term storage or transmission.  If a Parent Element contains more

I'm not sure if the "long-term" is intended to also bind as "long-term
transmission" (though I'm not sure what it would mean in that case).
It's also not entirely clear what kinds of transmission would benefit
from this, as reliable media presumably don't need redundancy for
reliability, but unreliable media can't really be used to carry EBML
without some framing requirements to know when elements start.

Section 11.1.18

   If a Mandatory EBML Element has no default value declared by an EBML
   Schema and its Parent Element is present then the EBML Element MUST
   be present as well.  If a Mandatory EBML Element has a default value
   declared by an EBML Schema and its Parent Element is present and the
   value of the EBML Element is NOT equal to the declared default value
   then the EBML Element MUST be present.

This seems almost tautological, in that how would an EBML Element have a
value if it was not present?  (The following paragraph that talks about
when to write such elements, does make more sense.)

Section 11.3.1

   path: "*1((1*\)\CRC-32)"

Using backslash as both an escape character and a path separator makes
my head hurt, and I did not have enough caffeine yet this morning to
figure it out.

   8.1.1.6.2 of [ITU.V42.1994], with initial value of 0xFFFFFFFF.  The
   CRC value MUST be computed on a little endian bitstream and MUST use
   little endian storage.

bitstream or bytestream?

Section 12

   If a Master Element contains a CRC-32 Element that doesn't validate,
   then the EBML Reader MAY ignore all contained data except for
   Descendant Elements that contain their own valid CRC-32 Element.

Ignoring only part of the known questionable content could have
significant security considerations, if (e.g.) security-relevant
restrictions are in the garbled part of the document but the sensitive
content has a (valid) redundant CRC.

[review terminated early]

---

From: Benjamin Kaduk <[email-masked]>
Date: Thu, 19 Dec 2019 13:32:20 -0800

> [review terminated early]

It turns out there would only have been a little more if it hadn't
terminated early:

Section 14.1.2

   The same value for Element Data Size MAY be written in variable
   lengths, so for minor reductions in octet length the Element Data
   Size MAY be written to a longer octet length to fill the freed space.

Is "reductions" the right word, here?

Section 16

   Side channel attacks could exploit:

The following list of items do all involve potential side channels, but
there's not a whole lot of clarity on what exactly is being attacked.

-Ben

---

From: Steve Lhomme <[email-masked]>
Date: Tue, 24 Dec 2019 16:20:25 +0100

Hi Benjamin,

Thanks a lot for your detailed review. Below are my comments and fixes:

> https://datatracker.ietf.org/doc/draft-ietf-cellar-ebml/

I think this has been address with this change
https://github.com/cellar-wg/ebml-specification/commit/a76fcb5a42ff543698d7292ee8a7752fe44ea1a0

"A Master Element MUST declare a length in octets from zero to VINTMAX
or be of unknown length."

Added the word "directly"here
https://github.com/cellar-wg/ebml-specification/pull/318

> Does the following attempted rewording change the meaning?

Nope, maybe the meaning wasn't clear before. Hopefully it's better now.

> "cannot assume this path".

Rephrased to
Global Element cannot be interpreted with this path, while parsing
`elt` a Global Element can only be a child of `elt`

https://github.com/cellar-wg/ebml-specification/pull/319

> still result in valid UTF-8 (i.e., not insert NULs in the middle of a

We already say:

A UTF-8 Element contains only a valid Unicode string as defined in
[RFC3629], with an exception made for termination

We also describe in Section 13 how NUL can be added at the end of
strings. That's probably where we should mention it should still be a
valid string. See this Pull Request
https://github.com/cellar-wg/ebml-specification/pull/320

> When might this SHOULD (NOT) be violated?

I think the idea is that a reader should not assume data are always
correct/clean. Files can get damaged, in which case some parts of them
may not look valid anymore.
I'm OK with changing it to MUST NOT, since error recovery is described
elsewhere. It would not be an error if junk data was considered OK.

> (for EBMLVersion 1 only, right?)

Yes, we kind of mention it here:
The EBML Header of an EBML Document that uses an EBMLVersion of 1 MUST
only contain EBML Elements that are defined as part of this document.

> registry for DocType values, here.

Added in https://github.com/cellar-wg/ebml-specification/pull/321

> when EBMLVersions other than "1" are defined?

That's currently a grey area. If there was an EBML Header version 2 we
can't assume the Matroska Schema to use it because we don't know if we
would want modifications (as we already do) on top of it or not.

So the EBML Schema should probably tell which EBML Header version it's
based on. Probably a ebml="1" attribute in the EBMLSchema element.
See Pull Request https://github.com/cellar-wg/ebml-specification/pull/322

> I think this makes more sense as "Section 11.1.5".

You're right. Fixed in https://github.com/cellar-wg/ebml-specification/pull/323

> general-purpose markup language.

Are you referring to the EBMLPathAtomRecursive ? It still has a parent
so it can be in another element as well.

> That second sentence doesn't parse.

Fixed in https://github.com/cellar-wg/ebml-specification/commit/55876fc0c306a54b08c79963a85d6aa3d4f9cfd6

> specific element in question.

In fact it's a remaining part of text from when the path did include
minOccurs/maxOccurs . This is not the case anymore and this text will
be removed:
https://github.com/cellar-wg/ebml-specification/pull/324

> Is this "name" supposed to be "Item" or "Items"?

It's a typo. Also the path should not contain the occurrences. Fixed
in the #324 PR.

> used; perhaps a reference to the corresponding XML behavior is in order?

Ping Dave Rice on that one.

> Why do we include this commented-out snippet?

It's an include of a XSD file that is actually usable to validate an
EBML Schema. This commented out code is there because we should fix it
to parse properly the path values. Should it be removed ?

> Don't we effectively set default values for these two in the prose

"If the unknownsizeallowed attribute is not used then that EBML
Element is not allowed to use an unknown Element Data Size."
I guess we could use false as the default value.

"If the recurring attribute is not present then the EBML Element is
not an Identically Recurring Element."
Same here

See https://github.com/cellar-wg/ebml-specification/pull/325

> without some framing requirements to know when elements start.

Actually, as long as you have the "packets" in the right order you can
use the EBML stream. You can also use it if you're missing some
packets. The Checksum can help determine if the data are valid or not,
even if the underlying transport loses some bits. It could technically
be used as-is as protocol on top of IP, like TCP or UDP.

> when to write such elements, does make more sense.)

The difference here is whether the element has a value and is present.
If you read the negative of that sentence, an EBML Element MAY NOT be
present if it has the default value even if it's mandatory.

> my head hurt, and I did not have enough caffeine yet this morning to

All the pathes actually have the remaining minOccurs/maxOccurs that
were removed recently. I'll remove them via #324

After that the two global elements (which used to be tricky to read
even for a trained eye) do not match the ABNF. Which also adds extra
parenthesis a little too much. I created an issue to fix once #324 is
merge https://github.com/cellar-wg/ebml-specification/issues/326

>    CRC value MUST be computed on a little endian bitstream and MUST use

Should be bytestream.
https://github.com/cellar-wg/ebml-specification/pull/327

> content has a (valid) redundant CRC.

That's why it's a MAY. If a Matroska Segment has a CRC and each frame
in it has a CRC. If the top CRC is invalid, we can still use some of
the frames that have a valid CRC. It's not a requirement but a
possibility.

> _______________________________________________
> Cellar mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/cellar

-- 
Steve Lhomme
Matroska association Chairman

---

From: Steve Lhomme <[email-masked]>
Date: Tue, 24 Dec 2019 16:34:10 +0100

> Is "reductions" the right word, here?

Yes, it's the data that is reduced, and thus the storage of the size
value that is increased.
I rewrote the paragraph. Hopefully it's clearer
https://github.com/cellar-wg/ebml-specification/pull/328

> there's not a whole lot of clarity on what exactly is being attacked.

Mostly on the data parser which could be made slow or not able to use
the data by misleading or bogus files.

---

From: Benjamin Kaduk <[email-masked]>
Date: Tue, 24 Dec 2019 10:41:47 -0800

> or be of unknown length."

I agree, and have already updated my ballot position in the datatracker to
reflect just the non-blocking COMMENTs that we discuss (sic) below.

> https://github.com/cellar-wg/ebml-specification/pull/318

Thanks.  I think that all three uses of "parent" in the non-table parts of
Section 6.2 are consistent with this definition and would not need a
broader definition (e.g., "ancestor element"), but please double check.

> Nope, maybe the meaning wasn't clear before. Hopefully it's better now.

I think Barry had some good suggestions here as well, so I think it's
better, yes.

> https://github.com/cellar-wg/ebml-specification/pull/320

(I'm leaving minor comments on github; the general trend of these is
looking good.)

> only contain EBML Elements that are defined as part of this document.

So it does, sorry for missing the forest for the trees.

> See Pull Request https://github.com/cellar-wg/ebml-specification/pull/322

That seems reasonable.

> so it can be in another element as well.

The EBMLPathAtomRecursive construction, yes.  It looks like the stuff
inside the (1*()) is limited to a single EBMLPathAtom, and if the atom is
only one path component, then there is no opportunity to do x\y\x\y\x\y\...
as "co-recursion".  But maybe I am misreading things.  Regardless, I am not
proposing any change to the document, so this discussion would just be for
my own edification.

> https://github.com/cellar-wg/ebml-specification/pull/324

Oh!  That makes me feel less bad about being confused by it; thanks.

> to parse properly the path values. Should it be removed ?

My preference would be to not include it in the immutable RFC, but I defer
to Alexey.

> be used as-is as protocol on top of IP, like TCP or UDP.

Huh, interesting.  Though I thought that UDP (and IP itself) didn't
guarantee in-order delivery.

> present if it has the default value even if it's mandatory.

Ah, I see it now; thanks.

> merge https://github.com/cellar-wg/ebml-specification/issues/326

Thanks :)

> the frames that have a valid CRC. It's not a requirement but a

I agree that it's an implementation choice for whether or not to do this,
but please add some text in the Security Considerations that mentions the
risk of handling incomplete-but-interdependent data when implementations
choose to do this sort of thing.

Thanks for all the updates!

-Ben

---

From: hubblec4 <[email-masked]>
Date: Wed, 25 Dec 2019 14:13:07 +0100

Hello

> I think Barry had some good suggestions here as well, so I think it's

Some ideas from me:

As I started to write my own Matroska parser it was not easy to
understand what is mean with all this and how I have to do.
After many reading and try, I understood it. For me is the VINT_MARKER
the key.

The position of the VINT_MARKER is the VINT_WIDTH.

Lazarus(my programming language) use a function "pos()", it gives me the
position of the first occurence of what I want.
In our case the VINT_MARKER is a "1".

VINT_WIDTH = pos(FIRST-BYTE-OF-VINT , VINT_MARKER);
Thats all what I have to code. No counting zero values, no adding one to
counted zero values.

---

From: Steve Lhomme <[email-masked]>
Date: Fri, 27 Dec 2019 10:17:58 +0100

> guarantee in-order delivery.

UDP and IP have no notion or order. But you can create protocols on top 
that do. Basically an EBML-based streaming format would just be:
[Packet Number][EBML element]
With EBML element smaller than 4000 or 1512 octets to fit nicely in 
Ethernet.
Anyway, that's definitely out of the scope of this document ;)
> choose to do this sort of thing.

I think it really depends on the dependency of the data at the semantic 
level. For example and EBML Element may define the type of data found in 
another EBML Element. If the type is damaged the interpretation of the 
data can create many kind of issues.

As for the CRC I think it's similar. It depends on what the CRC covers 
and the decision should probably be done at the semantic level, even 
with various options on how to handle the data.

---

From: Steve Lhomme <[email-masked]>
Date: Fri, 27 Dec 2019 10:18:07 +0100

> Thats all what I have to code. No counting zero values, no adding one to

For the record many processors have an instruction to find this value 
easily:
https://en.wikipedia.org/wiki/Find_first_set

---

From: Michael Richardson <[email-masked]>
Date: Fri, 27 Dec 2019 10:06:28 -0500

>>>> I'm not sure if the "long-term" is intended to also bind as
    >>>> "long-term transmission" (though I'm not sure what it would mean in
    >>>> that case).  It's also not entirely clear what kinds of transmission
    >>>> would benefit from this, as reliable media presumably don't need
    >>>> redundancy for reliability, but unreliable media can't really be
    >>>> used to carry EBML without some framing requirements to know when
    >>>> elements start.

    >>> Actually, as long as you have the "packets" in the right order you
    >>> can use the EBML stream. You can also use it if you're missing some
    >>> packets. The Checksum can help determine if the data are valid or
    >>> not, even if the underlying transport loses some bits. It could
    >>> technically be used as-is as protocol on top of IP, like TCP or UDP.
    >> 

    > [Packet Number][EBML element] With EBML element smaller than 4000 or
    > 1512 octets to fit nicely in Ethernet.  Anyway, that's definitely out
    > of the scope of this document ;)

Yes.  I read it as binding more to long-term storage than transmission.

But, not every transmission system is TCP, and EBML would certainly be appropriate
format for video transmissions from a multi-decade long flight of an
interstellar space-craft, and those don't do retransmissions :-)
This is an 'archival' format afterall, so it such a transmission would need
to survive multiple generations of base-station systems.

-- 
]               Never tell me the odds!                 | ipv6 mesh networks [
]   Michael Richardson, Sandelman Software Works        |    IoT architect   [
]     [email-masked]  http://www.sandelman.ca/        |   ruby on rails    [

---

From: Benjamin Kaduk <[email-masked]>
Date: Sun, 29 Dec 2019 19:50:23 -0800

> with various options on how to handle the data.

I entirely agree that it depneds on the semantics of the particular data in
question.  That said, even if it only would happen for a small fraction of
actual usage, typically we still see fit to document the existence of the
risk for the affected cases, in the security considerations.

-Ben