Merge pull request #124 from tomato42/backport-sig-decode

tomato42 · web-flow · commit 5c4c74a454c8 · 2019-10-07T15:51:23.000+02:00
Backport signature decode
diff --git a/.coveragerc b/.coveragerc
@@ -7,3 +7,5 @@ omit =
  ecdsa/six.py
  ecdsa/_version.py
  ecdsa/test_ecdsa.py
+ ecdsa/test_der.py
+ ecdsa/test_malformed_sigs.py
diff --git a/.travis.yml b/.travis.yml
@@ -1,4 +1,11 @@
+# workaround for 3.7 not available in default configuration
+# travis-ci/travis-ci#9815
+dist: trusty
+sudo: false
 language: python
+cache: pip
+before_cache:
+  - rm -f $HOME/.cache/pip/log/debug.log
 python:
   - "2.6"
   - "2.7"
@@ -11,6 +18,8 @@ install:
   - if [[ -e build-requirements-${TRAVIS_PYTHON_VERSION}.txt ]]; then travis_retry pip install -r build-requirements-${TRAVIS_PYTHON_VERSION}.txt; else travis_retry pip install -r build-requirements.txt; fi
 script:
   - coverage run setup.py test
+  - if [[ $TRAVIS_PYTHON_VERSION != 3.2 && ! $TRAVIS_PYTHON_VERSION =~ py ]]; then tox -e py${TRAVIS_PYTHON_VERSION/./}; fi
+  - if [[ $TRAVIS_PYTHON_VERSION =~ py ]]; then tox -e $TRAVIS_PYTHON_VERSION; fi
   - python setup.py speed
 after_success:
   - coveralls
diff --git a/README.md b/README.md
@@ -73,6 +73,7 @@ There are four test suites, three for the original Pearson module, and one
 more for the wrapper. To run them all, do this:
 
     python setup.py test
+    tox -e coverage
 
 On my 2014 Mac Mini, the combined tests take about 20 seconds to run. On a
 2.4GHz P4 Linux box, they take 81 seconds.
@@ -118,7 +119,8 @@ is to call `s=sk.to_string()`, and then re-create it with
 `SigningKey.from_string(s, curve)` . This short form does not record the
 curve, so you must be sure to tell from_string() the same curve you used for
 the original key. The short form of a NIST192p-based signing key is just 24
-bytes long.
+bytes long. If the point encoding is invalid or it does not lie on the
+specified curve, `from_string()` will raise MalformedPointError.
 
     from ecdsa import SigningKey, NIST384p
     sk = SigningKey.generate(curve=NIST384p)
@@ -132,7 +134,8 @@ formats that OpenSSL uses. The PEM file looks like the familiar ASCII-armored
 is a shorter binary form of the same data.
 `SigningKey.from_pem()/.from_der()` will undo this serialization. These
 formats include the curve name, so you do not need to pass in a curve
-identifier to the deserializer.
+identifier to the deserializer. In case the file is malformed `from_der()`
+and `from_pem()` will raise UnexpectedDER or MalformedPointError.
 
     from ecdsa import SigningKey, NIST384p
     sk = SigningKey.generate(curve=NIST384p)
diff --git a/build-requirements-2.6.txt b/build-requirements-2.6.txt
@@ -1,3 +1,4 @@
 tox
 coveralls<1.3.0
 idna<2.8
+unittest2
diff --git a/build-requirements-3.2.txt b/build-requirements-3.2.txt
@@ -7,3 +7,4 @@ PyYAML<3.13
 tox<3.0
 wheel<0.30
 virtualenv==15.2.0
+pytest>2.7.3
diff --git a/build-requirements-3.3.txt b/build-requirements-3.3.txt
@@ -2,3 +2,4 @@ python-coveralls
 tox<3.0
 wheel<0.30
 virtualenv==15.2.0
+pluggy<0.6
diff --git a/build-requirements.txt b/build-requirements.txt
@@ -1 +1,4 @@
 python-coveralls
+pytest>3.0.7
+pytest-cov<2.7.0
+tox
diff --git a/ecdsa/__init__.py b/ecdsa/__init__.py
@@ -1,9 +1,12 @@
 __all__ = ["curves", "der", "ecdsa", "ellipticcurve", "keys", "numbertheory",
            "test_pyecdsa", "util", "six"]
-from .keys import SigningKey, VerifyingKey, BadSignatureError, BadDigestError
+from .keys import SigningKey, VerifyingKey, BadSignatureError, BadDigestError,\
+        MalformedPointError
 from .curves import NIST192p, NIST224p, NIST256p, NIST384p, NIST521p, SECP256k1
+from .der import UnexpectedDER
 
 _hush_pyflakes = [SigningKey, VerifyingKey, BadSignatureError, BadDigestError,
+                  MalformedPointError, UnexpectedDER,
                   NIST192p, NIST224p, NIST256p, NIST384p, NIST521p, SECP256k1]
 del _hush_pyflakes
 
diff --git a/ecdsa/der.py b/ecdsa/der.py
@@ -60,10 +60,15 @@ def remove_constructed(string):
     return tag, body, rest
 
 def remove_sequence(string):
+    if not string:
+        raise UnexpectedDER("Empty string does not encode a sequence")
     if not string.startswith(b("\x30")):
-        n = string[0] if isinstance(string[0], integer_types) else ord(string[0])
-        raise UnexpectedDER("wanted sequence (0x30), got 0x%02x" % n)
+        n = string[0] if isinstance(string[0], integer_types) else \
+                ord(string[0])
+        raise UnexpectedDER("wanted type 'sequence' (0x30), got 0x%02x" % n)
     length, lengthlength = read_length(string[1:])
+    if length > len(string) - 1 - lengthlength:
+        raise UnexpectedDER("Length longer than the provided buffer")
     endseq = 1+lengthlength+length
     return string[1+lengthlength:endseq], string[endseq:]
 
@@ -96,14 +101,33 @@ def remove_object(string):
     return tuple(numbers), rest
 
 def remove_integer(string):
+    if not string:
+        raise UnexpectedDER("Empty string is an invalid encoding of an "
+                            "integer")
     if not string.startswith(b("\x02")):
-        n = string[0] if isinstance(string[0], integer_types) else ord(string[0])
-        raise UnexpectedDER("wanted integer (0x02), got 0x%02x" % n)
+        n = string[0] if isinstance(string[0], integer_types) \
+                else ord(string[0])
+        raise UnexpectedDER("wanted type 'integer' (0x02), got 0x%02x" % n)
     length, llen = read_length(string[1:])
+    if length > len(string) - 1 - llen:
+        raise UnexpectedDER("Length longer than provided buffer")
+    if length == 0:
+        raise UnexpectedDER("0-byte long encoding of integer")
     numberbytes = string[1+llen:1+llen+length]
     rest = string[1+llen+length:]
-    nbytes = numberbytes[0] if isinstance(numberbytes[0], integer_types) else ord(numberbytes[0])
-    assert nbytes < 0x80 # can't support negative numbers yet
+    msb = numberbytes[0] if isinstance(numberbytes[0], integer_types) \
+            else ord(numberbytes[0])
+    if not msb < 0x80:
+        raise UnexpectedDER("Negative integers are not supported")
+    # check if the encoding is the minimal one (DER requirement)
+    if length > 1 and not msb:
+        # leading zero byte is allowed if the integer would have been
+        # considered a negative number otherwise
+        smsb = numberbytes[1] if isinstance(numberbytes[1], integer_types) \
+                else ord(numberbytes[1])
+        if smsb < 0x80:
+            raise UnexpectedDER("Invalid encoding of integer, unnecessary "
+                                "zero padding bytes")
     return int(binascii.hexlify(numberbytes), 16), rest
 
 def read_number(string):
@@ -133,15 +157,23 @@ def encode_length(l):
     return int2byte(0x80|llen) + s
 
 def read_length(string):
+    if not string:
+        raise UnexpectedDER("Empty string can't encode valid length value")
     num = string[0] if isinstance(string[0], integer_types) else ord(string[0])
     if not (num & 0x80):
         # short form
         return (num & 0x7f), 1
     # else long-form: b0&0x7f is number of additional base256 length bytes,
     # big-endian
     llen = num & 0x7f
+    if not llen:
+        raise UnexpectedDER("Invalid length encoding, length of length is 0")
     if llen > len(string)-1:
-        raise UnexpectedDER("ran out of length bytes")
+        raise UnexpectedDER("Length of length longer than provided buffer")
+    # verify that the encoding is minimal possible (DER requirement)
+    msb = string[1] if isinstance(string[1], integer_types) else ord(string[1])
+    if not msb or llen == 1 and msb < 0x80:
+        raise UnexpectedDER("Not minimal encoding of length")
     return int(binascii.hexlify(string[1:1+llen]), 16), 1+llen
 
 def remove_bitstring(string):
diff --git a/ecdsa/keys.py b/ecdsa/keys.py
@@ -3,10 +3,11 @@
 from . import ecdsa
 from . import der
 from . import rfc6979
+from . import ellipticcurve
 from .curves import NIST192p, find_curve
 from .util import string_to_number, number_to_string, randrange
 from .util import sigencode_string, sigdecode_string
-from .util import oid_ecPublicKey, encoded_oid_ecPublicKey
+from .util import oid_ecPublicKey, encoded_oid_ecPublicKey, MalformedSignature
 from .six import PY3, b
 from hashlib import sha1
 
@@ -15,6 +16,11 @@ class BadSignatureError(Exception):
 class BadDigestError(Exception):
     pass
 
+
+class MalformedPointError(AssertionError):
+    pass
+
+
 class VerifyingKey:
     def __init__(self, _error__please_use_generate=None):
         if not _error__please_use_generate:
@@ -33,17 +39,21 @@ def from_public_point(klass, point, curve=NIST192p, hashfunc=sha1):
     def from_string(klass, string, curve=NIST192p, hashfunc=sha1,
                     validate_point=True):
         order = curve.order
-        assert len(string) == curve.verifying_key_length, \
-               (len(string), curve.verifying_key_length)
+        if len(string) != curve.verifying_key_length:
+            raise MalformedPointError(
+                "Malformed encoding of public point. Expected string {0} bytes"
+                " long, received {1} bytes long string".format(
+                    curve.verifying_key_length, len(string)))
         xs = string[:curve.baselen]
         ys = string[curve.baselen:]
-        assert len(xs) == curve.baselen, (len(xs), curve.baselen)
-        assert len(ys) == curve.baselen, (len(ys), curve.baselen)
+        if len(xs) != curve.baselen:
+            raise MalformedPointError("Unexpected length of encoded x")
+        if len(ys) != curve.baselen:
+            raise MalformedPointError("Unexpected length of encoded y")
         x = string_to_number(xs)
         y = string_to_number(ys)
-        if validate_point:
-            assert ecdsa.point_is_valid(curve.generator, x, y)
-        from . import ellipticcurve
+        if validate_point and not ecdsa.point_is_valid(curve.generator, x, y):
+            raise MalformedPointError("Point does not lie on the curve")
         point = ellipticcurve.Point(curve.curve, x, y, order)
         return klass.from_public_point(point, curve, hashfunc)
 
@@ -65,13 +75,18 @@ def from_der(klass, string):
         if empty != b(""):
             raise der.UnexpectedDER("trailing junk after DER pubkey objects: %s" %
                                     binascii.hexlify(empty))
-        assert oid_pk == oid_ecPublicKey, (oid_pk, oid_ecPublicKey)
+        if oid_pk != oid_ecPublicKey:
+            raise der.UnexpectedDER(
+                "Unexpected OID in encoding, received {0}, expected {1}"
+                .format(oid_pk, oid_ecPublicKey))
         curve = find_curve(oid_curve)
         point_str, empty = der.remove_bitstring(point_str_bitstring)
         if empty != b(""):
             raise der.UnexpectedDER("trailing junk after pubkey pointstring: %s" %
                                     binascii.hexlify(empty))
-        assert point_str.startswith(b("\x00\x04"))
+        if not point_str.startswith(b("\x00\x04")):
+            raise der.UnexpectedDER(
+                    "Unsupported or invalid encoding of pubcli key")
         return klass.from_string(point_str[2:], curve)
 
     def to_string(self):
@@ -106,11 +121,14 @@ def verify_digest(self, signature, digest, sigdecode=sigdecode_string):
                                  "for your digest (%d)" % (self.curve.name,
                                                            8*len(digest)))
         number = string_to_number(digest)
-        r, s = sigdecode(signature, self.pubkey.order)
+        try:
+            r, s = sigdecode(signature, self.pubkey.order)
+        except (der.UnexpectedDER, MalformedSignature) as e:
+            raise BadSignatureError("Malformed formatting of signature", e)
         sig = ecdsa.Signature(r, s)
         if self.pubkey.verifies(number, sig):
             return True
-        raise BadSignatureError
+        raise BadSignatureError("Signature verification failed")
 
 class SigningKey:
     def __init__(self, _error__please_use_generate=None):
@@ -134,7 +152,10 @@ def from_secret_exponent(klass, secexp, curve=NIST192p, hashfunc=sha1):
         self.default_hashfunc = hashfunc
         self.baselen = curve.baselen
         n = curve.order
-        assert 1 <= secexp < n
+        if not 1 <= secexp < n:
+            raise MalformedPointError(
+                "Invalid value for secexp, expected integer between 1 and {0}"
+                .format(n))
         pubkey_point = curve.generator*secexp
         pubkey = ecdsa.Public_key(curve.generator, pubkey_point)
         pubkey.order = n
@@ -146,7 +167,10 @@ def from_secret_exponent(klass, secexp, curve=NIST192p, hashfunc=sha1):
 
     @classmethod
     def from_string(klass, string, curve=NIST192p, hashfunc=sha1):
-        assert len(string) == curve.baselen, (len(string), curve.baselen)
+        if len(string) != curve.baselen:
+            raise MalformedPointError(
+                "Invalid length of private key, received {0}, expected {1}"
+                .format(len(string), curve.baselen))
         secexp = string_to_number(string)
         return klass.from_secret_exponent(secexp, curve, hashfunc)
 
diff --git a/ecdsa/test_der.py b/ecdsa/test_der.py
@@ -0,0 +1,88 @@
+
+# compatibility with Python 2.6, for that we need unittest2 package,
+# which is not available on 3.3 or 3.4
+try:
+    import unittest2 as unittest
+except ImportError:
+    import unittest
+from .der import remove_integer, UnexpectedDER, read_length
+from .six import b
+
+class TestRemoveInteger(unittest.TestCase):
+    # DER requires the integers to be 0-padded only if they would be
+    # interpreted as negative, check if those errors are detected
+    def test_non_minimal_encoding(self):
+        with self.assertRaises(UnexpectedDER):
+            remove_integer(b('\x02\x02\x00\x01'))
+
+    def test_negative_with_high_bit_set(self):
+        with self.assertRaises(UnexpectedDER):
+            remove_integer(b('\x02\x01\x80'))
+
+    def test_two_zero_bytes_with_high_bit_set(self):
+        with self.assertRaises(UnexpectedDER):
+            remove_integer(b('\x02\x03\x00\x00\xff'))
+
+    def test_zero_length_integer(self):
+        with self.assertRaises(UnexpectedDER):
+            remove_integer(b('\x02\x00'))
+
+    def test_empty_string(self):
+        with self.assertRaises(UnexpectedDER):
+            remove_integer(b(''))
+
+    def test_encoding_of_zero(self):
+        val, rem = remove_integer(b('\x02\x01\x00'))
+
+        self.assertEqual(val, 0)
+        self.assertFalse(rem)
+
+    def test_encoding_of_127(self):
+        val, rem = remove_integer(b('\x02\x01\x7f'))
+
+        self.assertEqual(val, 127)
+        self.assertFalse(rem)
+
+    def test_encoding_of_128(self):
+        val, rem = remove_integer(b('\x02\x02\x00\x80'))
+
+        self.assertEqual(val, 128)
+        self.assertFalse(rem)
+
+
+class TestReadLength(unittest.TestCase):
+    # DER requires the lengths between 0 and 127 to be encoded using the short
+    # form and lengths above that encoded with minimal number of bytes
+    # necessary
+    def test_zero_length(self):
+        self.assertEqual((0, 1), read_length(b('\x00')))
+
+    def test_two_byte_zero_length(self):
+        with self.assertRaises(UnexpectedDER):
+            read_length(b('\x81\x00'))
+
+    def test_two_byte_small_length(self):
+        with self.assertRaises(UnexpectedDER):
+            read_length(b('\x81\x7f'))
+
+    def test_long_form_with_zero_length(self):
+        with self.assertRaises(UnexpectedDER):
+            read_length(b('\x80'))
+
+    def test_smallest_two_byte_length(self):
+        self.assertEqual((128, 2), read_length(b('\x81\x80')))
+
+    def test_zero_padded_length(self):
+        with self.assertRaises(UnexpectedDER):
+            read_length(b('\x82\x00\x80'))
+
+    def test_two_three_byte_length(self):
+        self.assertEqual((256, 3), read_length(b'\x82\x01\x00'))
+
+    def test_empty_string(self):
+        with self.assertRaises(UnexpectedDER):
+            read_length(b(''))
+
+    def test_length_overflow(self):
+        with self.assertRaises(UnexpectedDER):
+            read_length(b('\x83\x01\x00'))
diff --git a/ecdsa/test_malformed_sigs.py b/ecdsa/test_malformed_sigs.py
diff --git a/ecdsa/test_pyecdsa.py b/ecdsa/test_pyecdsa.py
diff --git a/ecdsa/util.py b/ecdsa/util.py
diff --git a/tox.ini b/tox.ini
