Subject: [Cellar] AD Comments on draft-ietf-cellar-ebml-09

From: Ben Campbell <[email-masked]>
Date: Sat, 2 Mar 2019 18:05:39 -0600

Hi,

Thanks for submitting the update. Version 9 is an improvement on version 8, but I still have a few remaining (or new) comments. Please note that I have not repeated all of the comments I made yesterday in response to the discussion thread from my original review.

Thanks!

Ben.

---------------------

*** Substantive Comments ***

- A number of the pull requests the authors referenced in response to my original review have not made it into this version. Please check; if these have been left out on purpose, then please explain the reasoning.

§1: After some side discussions with a few people, I think it would be best to remove the sentence, "It MAY be used for use cases similar to those.”.  That sentence is not necessary to allow usage for cases similar to Matroska, and it is likely to still be a red flag for reviewers who are concerned about the draft scope.

If people really want to keep that sentence, note that there is now a plural disagreement between “Matroska” and “those”.

§3: The changes here do not seem responsive to my previous comment: "Can you offer guidance on the specific impacts of the mentioned attacks, and how to mitigate them? (If there is no way to mitigate an attack, it's okay to say that.)” In particular, I’d like to see a brief discussion of the potential security-related harms that could result from the issues in the three bullet lists.

Additionally, the new final paragraph "An "EBML Reader" MAY use the data if it considers it doesn't create any security issue.” seems underspecified. The security considerations should offer guidance to implementers about how to think about whether the data might create a security issue. (I assume that paragraph refers to the side-channel attack list immediately preceding it, but that’s not clear from the text.

Finally, the security considerations section is still in the wrong place. The RFC style guide requires the security considerations and the IANA considerations to be the last two sections in main body of the textr (not counting things like references, authors’s addresses, acknowledgements, etc.)

§13.1.10: The document still needs to identify (and cite) which XML schema format it uses. (For example, is this XSD? RELAX NG? Something else?)

I don’t think we resolved the question about whether the schema has been validated.

*** Editorial Comments ***

§3, first paragraph: Please expand CRC on first mention. (Which may or may not be this instance once the Security Considerations are moved to the proper place.)

§5.4, paragraph after first table: "Data encoded as a "Variable Size Integer" MAY be rendered at octet
lengths larger than needed to store the data."
Is that intended as permission, or a statement of fact? If the latter, then the normative keyword is not appropriate.

§13.1.5.10:
- "A boolean to express if an "EBML Element" MAY be used as an "Unknown-
Sized Element""
Statement of fact.

§15.1: "Numbers are be allocated within this range”

s/are be/be

---

From: Steve Lhomme <[email-masked]>
Date: Sun, 10 Mar 2019 15:06:59 +0100

> Thanks for submitting the update. Version 9 is an improvement on version 8, but I still have a few remaining (or new) comments. Please note that I have not repeated all of the comments I made yeste…

Do you have any in mind ? They are at least discussed on Github. Some
may be in version 10 (unpublished yet).

> §1: After some side discussions with a few people, I think it would be best to remove the sentence, "It MAY be used for use cases similar to those.”.  That sentence is not necessary to allow usage…

I agree, I created this PR
https://github.com/Matroska-Org/ebml-specification/pull/249

> §3: The changes here do not seem responsive to my previous comment: "Can you offer guidance on the specific impacts of the mentioned attacks, and how to mitigate them? (If there is no way to mitiga…

Mh, OK. This is tricky though because it's easy to tell how things
could be abused (and security researchers may find others), but it's
another thing to explain how that could be exploited.

The first list already mentions that the elements could be used and
consider there's no issue.

As for mitigating it depends a lot on what the parser is trying to
achieve and what level of strictness it wants to achieve. I'm not sure
we want this to be normative. For example "Use of Void Elements" could
be used to hide content from regular parsers, create fake resync
points that are seen by some parsers and some others. And there's
probably other creative ways to use it. But I don't think it's
possible to be exhaustive here.

> Additionally, the new final paragraph "An "EBML Reader" MAY use the data if it considers it doesn't create any security issue.” seems underspecified. The security considerations should offer guidan…

OK. I'll try to come up with something more detailed.

> Finally, the security considerations section is still in the wrong place. The RFC style guide requires the security considerations and the IANA considerations to be the last two sections in main bo…

It was moved just above the IANA considerations in
1ec57eecf82e94f64a81ac8d0dd72d0d3f8c106d

> I don’t think we resolved the question about whether the schema has been validated.

Indeed, and I have no idea. Dave ?

> §3, first paragraph: Please expand CRC on first mention. (Which may or may not be this instance once the Security Considerations are moved to the proper place.)

Done in PR https://github.com/Matroska-Org/ebml-specification/pull/250

> Is that intended as permission, or a statement of fact? If the latter, then the normative keyword is not appropriate.

The former. Updated in PR
https://github.com/Matroska-Org/ebml-specification/pull/251

I think it was fixed in 0a26e3ac2c7767fb3509d914e5b57daac870d19f

> §15.1: "Numbers are be allocated within this range”

I think it was meant as "numbers are to be allocated within this
range". I did that in 0acceca50d63ceb46c1b94223f0da49f6c1ad580. Now I
wonder if it should be more normative than that, with a MUST.

---

From: Steve Lhomme <[email-masked]>
Date: Sun, 10 Mar 2019 15:50:35 +0100

> possible to be exhaustive here.

I added some text where it was not obvious (IMO) how the security
consideration items could be interpreted:
https://github.com/Matroska-Org/ebml-specification/pull/252

I did a commit for each, I may split in many PRs if there are too many
issues to merge.

---

From: Ben Campbell <[email-masked]>
Date: Sat, 23 Mar 2019 14:06:01 +0100

Hi, Apologies for the delay in responding. Your reply caught me on vacation. Also, I’m copying Alexey, who will likely take over as responsible AD for CELLAR when I step down this week.

Please see inline:

Thanks!

Ben.

> On Mar 10, 2019, at 3:06 PM, Steve Lhomme <[email-masked]> wrote:
> 

Here’s some that I could find that were not in 09, but it may not catch all of them. I see some have been merged since 09, but for the AD evaluation process, I prefer to see issues resolved in actually published draft versions before we consider them closed.

https://github.com/Matroska-Org/ebml-specification/pull/212

https://github.com/Matroska-Org/ebml-specification/pull/213

https://github.com/Matroska-Org/ebml-specification/pull/215

https://github.com/Matroska-Org/ebml-specification/pull/216

> https://github.com/Matroska-Org/ebml-specification/pull/249h

I agree with the pull request.

[…]

> another thing to explain how that could be exploited.

I may be making this sound harder than it needs to be. I don’t think we need detailed “security-expert” level analysis. I’m looking more along the line of why people thought a given “thing someone could do wrong” is security-relevant.

For example: There is a section labeled: “ An "EBML Document" that has the following issues may still be handled
 	   by the "EBML Reader" and the data accepted as such:”

Does that mean there are no (known) security problems related those issues?

For the section: "An "EBML Reader" may discard some or all data if the following errors
 	   are found in the "EBML Document”:”

It looks like at least some of these could result in buffer-overrun type errors. Is there a concern that such overruns could contain executable code? Or are we just talking about denial-of-service? Do missing mandatory elements create a security issue, or just useless data? Is it safe to say just a reader “may” discard data with these errors, or should it say something stronger? (For example, would it ever make sense to _not_ discard data with missing mandatory elements?)

In the side-channel section; what sort of information do you think can be inferred from the mentioned side-channel attacks? Is this mostly about implementation finger-printing?

> possible to be exhaustive here.

It doesn’t have to be normative guidance. (It’s pretty common for security considerations to be non-normative.)
> Indeed, and I have no idea. Dave ?

Any updates here?

>> *** Editorial Comments ***

[…]

> wonder if it should be more normative than that, with a MUST.

My git fu is not quite up to quickly finding the commit from just the string; can you send the URLs for these last two?

Thanks!

Ben.

---

From: Steve Lhomme <[email-masked]>
Date: Sun, 31 Mar 2019 10:28:10 +0200

Hi,

> Here’s some that I could find that were not in 09, but it may not catch all of them. I see some have been merged since 09, but for the AD evaluation process, I prefer to see issues resolved in actu…

I agree.

> https://github.com/Matroska-Org/ebml-specification/pull/212

Only this one in the list is still not merged. Dave seem to have some
disagreement on this, that EBML MUST strictly contain EBML but it
contradicts the streaming case were the data are sent
mid-element/level to simplify the code. As said in the comments
GStreamer used to do that but it may not be the case anymore. I know
in VLC/libavformat the header of the format is sent first and then
waits for keyframe to actually start sending the data to avoid this.
You don't lost any latency, you have a clean stream on the output. The
only drawback is that the receiver has to wait for a keyframe to start
seeing data. In any case it won't be able to start decoding before
that so it's not an issue at all.

So I may lean on the stricter (Dave) point of view here. That means we
have to change the streaming paragraph not to allow this anymore. I'll
close #212 and open another one.
https://github.com/Matroska-Org/ebml-specification/pull/254

For old streams using this feature, the error/bogus EBML data recovery
still apply and the reader will still have to look for valid data
after it finds bad data.

> Does that mean there are no (known) security problems related those issues?

Not exactly security issues, more abuse or bad uses of the format.
It's more a reminder of tricky things parser need to be able to cope
with, otherwise they might end up misbehaving and hiding/skipping
usable content. That might have an impact on other elements and
cascade into bigger issues at the semantic level.

> It looks like at least some of these could result in buffer-overrun type errors. Is there a concern that such overruns could contain executable code?

Given Matroska has attachments, such errors in the EBML could lead to
issues. Executables are not common as attachments (or if at all) but
Fonts are and compromising such data could lead to crashes in the
OS/program when it parses the font.

> Or are we just talking about denial-of-service? Do missing mandatory elements create a security issue, or just useless data?

Missing mandatory elements could lead to semantic incoherences.
Depending on how the parser is done it may lead to infinite loops
and/or crashes/

> Is it safe to say just a reader “may” discard data with these errors, or should it say something stronger? (For example, would it ever make sense to _not_ discard data with missing mandatory elements?)

I don't think it's good to tell how a parser should behave. Given one
of the goal of CELLAR is long term storage, what makes sense to a
player may not to an error recovery reader to figure out how to use
the data it finds. I think making the people who writes parsers aware
of the possible issues is enough for them to make the decision on how
to treat these errors.

And also the fact that giving one direction means we have to be sure
that security-wise it's actually the best thing to do.

> In the side-channel section; what sort of information do you think can be inferred from the mentioned side-channel attacks? Is this mostly about implementation finger-printing?

Some can lead to issues at the semantic level to compare values,
making parts of the data unusable. The Void element can be used to
hide or "void" content that should have been there.

> It doesn’t have to be normative guidance. (It’s pretty common for security considerations to be non-normative.)

OK

> > Indeed, and I have no idea. Dave ?

Nope

> My git fu is not quite up to quickly finding the commit from just the string; can you send the URLs for these last two?

Sure:
https://github.com/Matroska-Org/ebml-specification/commit/0a26e3ac2c7767fb3509d914e5b57daac870d19f

and

https://github.com/Matroska-Org/ebml-specification/commit/0acceca50d63ceb46c1b94223f0da49f6c1ad580

-- 
Steve Lhomme
Matroska association Chairman

---

From: Reto Kromer <[email-masked]>
Date: Sun, 31 Mar 2019 11:12:46 +0200

Steve Lhomme wrote:

>>Is it safe to say just a reader “may” discard data with these
>>errors, or should it say something stronger? (For example,
>>would it ever make sense to _not_ discard data with missing
>>mandatory elements?)
>
>I don't think it's good to tell how a parser should behave.
>Given one of the goal of CELLAR is long term storage, what
>makes sense to a player may not to an error recovery reader to
>figure out how to use the data it finds. I think making the
>people who writes parsers aware of the possible issues is
>enough for them to make the decision on how to treat these
>errors.

Full agreement on my end! Best regards, Reto