Subject: [Cellar] [PATCH] FFV1: Add security considerations

From: Michael Niedermayer <[email-masked]>
Date: Mon, 11 Jul 2016 10:16:26 +0200

Hi all

The attached patch adds security considerations based on the OPUS
RFC.
Is it customary to add references to / copyrights from the RFC text is
borrowed from ? Or is it correct/intended to just take text randomly
when needed ?

Thanks
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
From 559755e4cc9505b8b02559d8c4e831b2f3c75a56 Mon Sep 17 00:00:00 2001
From: Michael Niedermayer <[email-masked]>
Date: Mon, 11 Jul 2016 10:00:48 +0200
Subject: [PATCH] FFV1: Add security considerations

Based on RFC6716

Github: Closes #20

Signed-off-by: Michael Niedermayer <[email-masked]>
---
 ffv1.md | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/ffv1.md b/ffv1.md
index 16f7230..825b409 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -902,6 +902,23 @@ For each frame, each position in the slice raster MUST be filled by one and only
 
 For each Frame with keyframe value of 0, each slice MUST have the same value of slice\_x, slice\_y, slice\_width, slice\_height as a slice in the previous frame, except if reset\_contexts is 1.
 
+# Security Considerations
+
+Like any other video codec, FFV1 should not be used with insecure ciphers or cipher-modes that are vulnerable to known plaintext attacks. Some of the header bits as well as the padding is easily predictable.
+
+Implementations of the FFV1 codec need to take appropriate security considerations into account, as outlined in [DOS](#references). 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.  Although problems in encoders are typically rarer, the same applies to the encoder.  Malicious video streams must not cause the encoder to misbehave because this would allow an attacker to attack transcoding gateways. A frequent security problem in image and video codecs is also to not check for integer overflows in pixel count computations, that is to allocate width * height without considering that the multiplication result may have overflowed the arithmetic types range.
+
+The reference implementation contains no known buffer overflow or cases where a specially crafted packet or video segment could cause a significant increase in CPU load.
+
+The reference implementation was validated in the following conditions:
+
+* Sending the decoder valid packets generated by the reference encoder and verifying that the decoder's output matches the encoders input.
+* Sending the decoder packets generated by the reference encoder and then subjected to random corruption.
+* Sending the decoder random packets that are not FFV1.
+
+In all of the conditions above, the decoder and encoder was run inside the Valgrind [VALGRIND](#references) memory debugger as well as clangs address sanitizer, which track reads and writes to invalid memory regions as well as the use of uninitialized memory.  There were no errors reported on any of the tested conditions.
+
+
 # Appendixes
 
 ## Decoder implementation suggestions
@@ -965,6 +982,10 @@ Information technology Coding of audio-visual objects Part 12: ISO base media fi
 
 NUT Open Container Format <http://www.ffmpeg.org/~michael/nut.txt>
 
+DOS Handley, M., Rescorla, E., and IAB, "Internet Denial-of-Service Considerations", RFC 4732, December 2006.
+
+VALGRIND "Valgrind website", <http://valgrind.org/>.
+
 # Copyright
 
 Copyright 2003-2013 Michael Niedermayer \<[email-masked]\>
-- 
2.7.4

---

From: Luca Barbato <[email-masked]>
Date: Mon, 11 Jul 2016 10:55:59 +0200

On 11/07/16 10:16, Michael Niedermayer wrote:
> +In all of the conditions above, the decoder and encoder was run
> inside the Valgrind [VALGRIND](#references) memory debugger as well
> as clangs address sanitizer, which track reads and writes to invalid
> memory regions as well as the use of uninitialized memory.  There
> were no errors reported on any of the tested conditions.

The address sanitizer might enjoy a link as well. Beside that looks fine.

You might reference the rfc paragraph as in

   Like any other codec, such as [opus](reference), ...

lu

---

From: "Timothy B. Terriberry" <[email-masked]>
Date: Mon, 11 Jul 2016 06:10:14 -0700

> borrowed from ? Or is it correct/intended to just take text randomly

According to RFC 5738, Section 6, "...additional copyright notices are 
not permitted in IETF Documents except in the case where such document 
is the product of a joint development effort between the IETF and 
another standards development organization or is a republication of the 
work of another standards development organization. Such exceptions must 
be approved on an individual basis by the IAB."

Based on the rights the authors grant the IETF when submitting their 
document, the IETF Trust Legal Provisions grant back to all IETF 
participants the right to "modify or prepare derivative works of such 
IETF Contributions or IETF Documents, in whole or in part, as part of 
the IETF Standards Process," "...unless explicitly disallowed in the 
notices contained in an IETF Contribution or IETF Document" [1]. As far 
as I am aware, this grant does not require preserving any copyright 
notices, so there is no conflict with RFC 5738.

Use of material from outside the IETF or allowing use of your draft 
outside of the IETF is a more complicated subject.

[1] https://trustee.ietf.org/documents/IETF-TLP-5_001.html Section 3.a.iii

---

From: Dave Rice <[email-masked]>
Date: Mon, 11 Jul 2016 10:10:36 -0400

> On Jul 11, 2016, at 4:16 AM, Michael Niedermayer <[email-masked]> wrote:
> 
> Hi all
> 

> +Like any other video codec, FFV1 should not be used with insecure ciphers or cipher-modes that are vulnerable to known plaintext attacks. Some of the header bits as well as the padding is

are

> easily predictable.
> +
> +Implementations of the FFV1 codec need to take appropriate security considerations into account, as outlined in [DOS](#references). It is extremely important for the decoder to be robust against malicious payloads. Malicious payloads must not

Can this be a `must not`? Seems too ideal as the decoder author can not know all types of malicious payloads.

> cause the decoder to overrun its allocated memory or to take an excessive amount of resources to decode.  Although problems in encoders are typically rarer, the same applies to the encoder.  Malicious video streams must not cause the encoder to misbehave

word choice. Perhaps 'fail' instead of 'misbehave'.

> because this would allow an attacker to attack transcoding gateways. A frequent security problem in image and video codecs is also to not check for integer overflows in pixel count computations, that is to allocate width * height without considering that the multiplication result may have overflowed the arithmetic types range.
> +

I think both here and https://github.com/FFmpeg/FFV1/blob/master/ffv1.md#alternative-state-transition-table the 'reference implementation' should use a citation so the identify of the reference implementation is clear.

Relatedly, should the ffv1 spec include a reference implementation as the opus rfc does?

> +In all of the conditions above, the decoder and encoder was run inside the Valgrind [VALGRIND](#references) memory debugger as well as clangs address sanitizer, which track reads and writes to inv…

[....]

Best Regards,
Dave Rice

---

From: Michael Niedermayer <[email-masked]>
Date: Mon, 11 Jul 2016 20:44:21 +0200

> Can this be a `must not`? Seems too ideal as the decoder author can not know all types of malicious payloads.

That same text should also be in the OPUS RFC unless i messed up,
i dont see a problem here one must not trigger undefined behavior,
especially not due to received malicious data

> word choice. Perhaps 'fail' instead of 'misbehave'.

The decoder can fail to decode a malicious stream, it should not misbehave
as in crashing or allowing the attacker to take control of anything
she should not have control of.

ill send a new patch soon

[...]

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin

---

From: Michael Niedermayer <[email-masked]>
Date: Mon, 11 Jul 2016 20:45:25 +0200

> Relatedly, should the ffv1 spec include a reference implementation as the opus rfc does?

forgot to reply to this, i dont know, seems a bit odd though

[...]

No snowflake in an avalanche ever feels responsible. -- Voltaire

---

From: Michael Niedermayer <[email-masked]>
Date: Mon, 11 Jul 2016 20:57:19 +0200

On Mon, Jul 11, 2016 at 08:44:21PM +0200, Michael Niedermayer wrote:
[...]
> ill send a new patch soon

attached

[...]

Democracy is the form of government in which you can choose your dictator
From a291eb17bb34a3663150523aa12096cc0db0324d Mon Sep 17 00:00:00 2001
From: Michael Niedermayer <[email-masked]>
Date: Mon, 11 Jul 2016 10:00:48 +0200
Subject: [PATCH] FFV1: Add security considerations

Based on RFC6716

Github: Closes #20

Signed-off-by: Michael Niedermayer <[email-masked]>
---
 ffv1.md | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/ffv1.md b/ffv1.md
index 16f7230..6db8875 100644
--- a/ffv1.md
+++ b/ffv1.md
@@ -902,6 +902,23 @@ For each frame, each position in the slice raster MUST be filled by one and only

+# Security Considerations
+
+Like any other codec, (such as [OPUS](#references)), FFV1 should not be used with insecure ciphers or cipher-modes that are vulnerable to known plaintext attacks. Some of the header bits as well as the padding are easily predictable.
+
+Implementations of the FFV1 codec need to take appropriate security considerations into account, as outlined in [DOS](#references). 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.  Although problems in encoders are typically rarer, the same applies to the encoder.  Malicious video streams must not cause the encoder to misbehave because this would allow an attacker to attack transcoding gateways. A frequent security problem in image and video codecs is also to not check for integer overflows in pixel count computations, that is to allocate width * height without considering that the multiplication result may have overflowed the arithmetic types range.
+
+The reference implementation [REFIMPL](#references) contains no known buffer overflow or cases where a specially crafted packet or video segment could cause a significant increase in CPU load.
+
+The reference implementation [REFIMPL](#references) was validated in the following conditions:
+
+* Sending the decoder valid packets generated by the reference encoder and verifying that the decoder's output matches the encoders input.
+* Sending the decoder packets generated by the reference encoder and then subjected to random corruption.
+* Sending the decoder random packets that are not FFV1.
+
+In all of the conditions above, the decoder and encoder was run inside the Valgrind [VALGRIND](#references) memory debugger as well as clangs address sanitizer [ASAN](#references), which track reads and writes to invalid memory regions as well as the use of uninitialized memory.  There were no errors reported on any of the tested conditions.
+
+
 # Appendixes
 
 ## Decoder implementation suggestions
@@ -965,6 +982,16 @@ Information technology Coding of audio-visual objects Part 12: ISO base media fi

+DOS Handley, M., Rescorla, E., and IAB, "Internet Denial-of-Service Considerations", RFC 4732, December 2006.
+
+VALGRIND "Valgrind website", <http://valgrind.org/>.
+
+ASAN Addresss Sanitizer, <http://clang.llvm.org/docs/AddressSanitizer.html>.
+
+REFIMPL, The reference FFV1 implementation / the FFV1 codec in FFmpeg, <https://ffmpeg.org/>.
+
+OPUS, Definition of the Opus Audio Codec, <https://www.ietf.org/rfc/rfc6716.txt>
+
 # Copyright

---

From: Dave Rice <[email-masked]>
Date: Tue, 12 Jul 2016 11:23:36 -0400

> On Jul 11, 2016, at 2:57 PM, Michael Niedermayer <[email-masked]> wrote:
> 

LGTM

[...]

Dave

---

From: Michael Niedermayer <[email-masked]>
Date: Tue, 12 Jul 2016 17:42:00 +0200

> >> ill send a new patch soon

applied

thanks

[...]

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin