Subject: Secdir early review of draft-ietf-cellar-ffv1-02

From: Liang Xia <[email-masked]>
Date: Fri, 01 Jun 2018 02:23:20 -0700

Reviewer: Liang Xia
Review result: Ready

The whole draft is in good shape and well written.
Some nits:
1. every word should start with capital letter for the section title;
2. section 2.2.4: / ceil(a) the largest integer less than or equal to a /
ceil(a) the smallest integer larger than or equal to a / 3. section 3.7.2:
[ISO.15444-1.2016]? 4. section 12.1: [I-D.ietf-cellar-ffv1]? 5. section 12.2:
should all the RFC move to the Normative References (section 12.1)?

Issues for clarification:
In Security Considerations, besides the DoS attacks brought by the malicious
payloads, is there any other kinds of attack possibly? For example, virus or
worm are hidden in the malicious payloads to attack the system for more
damages? Does it make sense and what's the consideration?

---

From: Dave Rice <[email-masked]>
Date: Fri, 1 Jun 2018 10:37:49 -0400

Hi,
Thanks for this review.

> On Jun 1, 2018, at 5:23 AM, Liang Xia <[email-masked]> wrote:
> 
> Reviewer: Liang Xia
> Review result: Ready
> 

I added a pull request at https://github.com/FFmpeg/FFV1/pull/116 <https://github.com/FFmpeg/FFV1/pull/116> that addresses many of these issues.

Regarding 
> section 12.1: [I-D.ietf-cellar-ffv1]?
I found that Media Type Definitions within RFC’s tend to be self-referencing. Since there is no RFC number here on this draft, I’ve used [I-D.ietf-cellar-ffv1] as a temporary self reference.

> damages? Does it make sense and what's the consideration?

I haven’t done any update for this. Nudge to Michael Niedermayer and Jérôme Martinez.
Best Regards,
Dave Rice

---

From: Jerome Martinez <[email-masked]>
Date: Fri, 1 Jun 2018 17:12:54 +0200

> damages? Does it make sense and what's the consideration?

IMO transport of virus or worm is doable in the bitstream and could 
attack the system if there are buffer overflows in the decoding 
software, but not more dangerous than any other protocol or format (it 
depends on bugs in the decoding software).

Checking e.g. Opus spec (I tried AV1 draft, but no security chapter 
right now if I well searched), I see generic sentences like:
"It is extremely
    important for the decoder to be robust against malicious payloads.
    Malicious payloads must not cause the decoder to overrun its
    allocated memory or to take an excessive amount of resources to
    decode."
"The reference implementation contains no known buffer overflow or
    cases where a specially crafted packet or audio segment could cause a
    significant increase in CPU load. "
"The reference implementation was validated in the following
    conditions: (...)" (note: we ran same tests on our side)

We could add such sentences in FFV1 security section.
About the reference decoder, there are some hard coded limitations (e.g. 
maximum 1024 slices per frame, arbitrary choice which is sometimes 
increased in the code) for dropping frames which could use too much 
memory, and the decoder tries to allocate memory for big frames (e.g. if 
you try to decode de 1,000,000x1,000,000 pixel frames, FFmpeg will try 
to allocate corresponding memory as for any other format, and rejects 
the frame because memory can not be allocated. I don't think it is worth 
it to put details about that in spec, as FFmpeg code may change, maybe 
the generic sentences are enough?

Thank you for your review.

Jérôme