Merge pull request python-hyper#23 from python-hyper/issue/22

CVE-2016-6580

Author: Lukasa

HISTORY.rst

@@ -1,6 +1,23 @@
 Changelog
 =========
 
+1.2.0 (2016-08-04)
+------------------
+
+**Security Fixes**
+
+- CVE-2016-6580: All versions of this library prior to 1.2.0 are vulnerable to
+  a denial of service attack whereby a remote peer can cause a user to insert
+  an unbounded number of streams into the priority tree, eventually consuming
+  all available memory.
+
+  This version adds a ``TooManyStreamsError`` exception that is raised when
+  too many streams are inserted into the priority tree. It also adds a keyword
+  argument to the priority tree, ``maximum_streams``, which limits how many
+  streams may be inserted. By default, this number is set to 1000.
+  Implementations should strongly consider whether they can set this value
+  lower.
+
 1.1.1 (2016-05-28)
 ------------------
 

docs/source/api.rst

@@ -17,3 +17,5 @@ Exceptions
 .. autoclass:: priority.DuplicateStreamError
 
 .. autoclass:: priority.MissingStreamError
+
+.. autoclass:: priority.TooManyStreamsError

docs/source/index.rst

@@ -23,6 +23,7 @@ Contents:
    installation
    using-priority
    api
+   security/index
    license
    authors
 

docs/source/security/CVE-2016-6580.rst

@@ -0,0 +1,64 @@
+:orphan:
+
+DoS via Unlimited Stream Insertion
+==================================
+
+Hyper Project security advisory, August 4th 2016.
+
+Vulnerability
+-------------
+
+A HTTP/2 implementation built using the priority library could be targetted by
+a malicious peer by having that peer assign priority information for every
+possible HTTP/2 stream ID. The priority tree would happily continue to store
+the priority information for each stream, and would therefore allocate
+unbounded amounts of memory. Attempting to actually *use* a tree like this
+would also cause extremely high CPU usage to maintain the tree.
+
+We are not aware of any active exploits of this vulnerability, but as this
+class of attack was publicly described in `this report`_, users should assume
+that they are at imminent risk of this kind of attack.
+
+Info
+----
+
+This issue has been given the name CVE-2016-6580.
+
+Affected Versions
+-----------------
+
+This issue affects all versions of the priority library prior to 1.2.0.
+
+The Solution
+------------
+
+In version 1.2.0, the priority library limits the maximum number of streams
+that can be inserted into the tree. By default this limit is 1000, but it is
+user-configurable.
+
+If it is necessary to backport a patch, the patch can be found in
+`this GitHub pull request`_.
+
+Recommendations
+---------------
+
+We suggest you take the following actions immediately, in order of preference:
+
+1. Update priority to 1.2.0 immediately, and consider revising the maximum
+   number of streams downward to a suitable value for your application.
+2. Backport the patch made available on GitHub.
+3. Manually enforce a limit on the number of priority settings you'll allow at
+   once.
+
+Timeline
+--------
+
+This class of vulnerability was publicly reported in `this report`_ on the
+3rd of August. We requested a CVE ID from Mitre the same day.
+
+Priority 1.2.0 was released on the 4th of August, at the same time as the
+publication of this advisory.
+
+
+.. _this report: http://www.imperva.com/docs/Imperva_HII_HTTP2.pdf
+.. _this GitHub pull request: https://github.com/python-hyper/priority/pull/23

docs/source/security/index.rst

@@ -0,0 +1,19 @@
+Vulnerability Notifications
+===========================
+
+This section of the page contains all known vulnerabilities in the priority
+library. These vulnerabilities have all been reported to us via our
+`vulnerability disclosure policy`_.
+
+Known Vulnerabilities
+---------------------
+
++----+---------------------------+----------------+---------------+--------------+---------------+
+| \# |       Vulnerability       | Date Announced | First Version | Last Version |      CVE      |
++====+===========================+================+===============+==============+===============+
+| 1  | :doc:`DoS via unlimited   | 2016-08-04     | 1.0.0         | 1.1.1        | CVE-2016-6580 |
+|    | stream insertion.         |                |               |              |               |
+|    | <CVE-2016-6580>`          |                |               |              |               |
++----+---------------------------+----------------+---------------+--------------+---------------+
+
+.. _vulnerability disclosure policy: http://python-hyper.org/en/latest/security.html#vulnerability-disclosure

src/priority/__init__.py

@@ -4,5 +4,5 @@
 """
 from .priority import (  # noqa
     Stream, PriorityTree, DeadlockError, PriorityLoop, DuplicateStreamError,
-    MissingStreamError
+    MissingStreamError, TooManyStreamsError
 )

src/priority/priority.py

@@ -43,6 +43,16 @@ class MissingStreamError(KeyError, Exception):
     pass
 
 
+class TooManyStreamsError(Exception):
+    """
+    An attempt was made to insert a dangerous number of streams into the
+    priority tree at the same time.
+
+    .. versionadded:: 1.2.0
+    """
+    pass
+
+
 class Stream(object):
     """
     Priority information for a given stream.
@@ -199,13 +209,33 @@ class PriorityTree(object):
     A HTTP/2 Priority Tree.
 
     This tree stores HTTP/2 streams according to their HTTP/2 priorities.
+
+    .. versionchanged:: 1.2.0
+       Added ``maximum_streams`` keyword argument.
+
+    :param maximum_streams: The maximum number of streams that may be active in
+        the priority tree at any one time. If this number is exceeded, the
+        priority tree will raise a :class:`TooManyStreamsError
+        <priority.TooManyStreamsError>` and will refuse to insert the stream.
+
+        This parameter exists to defend against the possibility of DoS attack
+        by attempting to overfill the priority tree. If any endpoint is
+        attempting to manage the priority of this many streams at once it is
+        probably trying to screw with you, so it is sensible to simply refuse
+        to play ball at that point.
+
+        While we allow the user to configure this, we don't really *expect*
+        them too, unless they want to be even more conservative than we are by
+        default.
+    :type maximum_streams: ``int``
     """
-    def __init__(self):
+    def __init__(self, maximum_streams=1000):
         # This flat array keeps hold of all the streams that are logically
         # dependent on stream 0.
         self._root_stream = Stream(stream_id=0, weight=1)
         self._root_stream.active = False
         self._streams = {0: self._root_stream}
+        self._maximum_streams = maximum_streams
 
     def _exclusive_insert(self, parent_stream, inserted_stream):
         """
@@ -233,6 +263,13 @@ def insert_stream(self,
         if stream_id in self._streams:
             raise DuplicateStreamError("Stream %d already in tree" % stream_id)
 
+        if (len(self._streams) + 1) > self._maximum_streams:
+            raise TooManyStreamsError(
+                "Refusing to insert %d streams into priority tree at once" % (
+                    self._maximum_streams + 1
+                )
+            )
+
         stream = Stream(stream_id, weight)
 
         if exclusive:

test/test_priority.py

@@ -292,6 +292,21 @@ def test_priority_raises_good_errors_for_missing_streams(self):
         with pytest.raises(priority.MissingStreamError):
             p.remove_stream(3)
 
+    @pytest.mark.parametrize('count', range(2, 10000, 100))
+    def test_priority_refuses_to_allow_too_many_streams_in_tree(self, count):
+        """
+        Attempting to insert more streams than maximum_streams into the tree
+        fails.
+        """
+        p = priority.PriorityTree(maximum_streams=count)
+
+        # This isn't an off-by-one error: stream 0 is in the tree by default.
+        for x in range(1, count):
+            p.insert_stream(x)
+
+        with pytest.raises(priority.TooManyStreamsError):
+            p.insert_stream(x + 1)
+
 
 class TestPriorityTreeOutput(object):
     """