From 50e6d9f3788f7f442d8b44fcf71e4007d774d8d1 Mon Sep 17 00:00:00 2001 From: Hubert Kario Date: Thu, 26 Sep 2019 18:07:09 +0200 Subject: [PATCH 1/8] strict error checking in DER decoding of integers and sequences --- src/ecdsa/der.py | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/ecdsa/der.py b/src/ecdsa/der.py index f7a587ab..4ae2d3c7 100644 --- a/src/ecdsa/der.py +++ b/src/ecdsa/der.py @@ -75,10 +75,15 @@ def remove_constructed(string): 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:] @@ -114,14 +119,24 @@ def remove_object(string): 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 + nbytes = numberbytes[0] if isinstance(numberbytes[0], integer_types) \ + else ord(numberbytes[0]) + if not nbytes < 0x80: + raise UnexpectedDER("Negative integers are not supported") return int(binascii.hexlify(numberbytes), 16), rest From 9916c852d39309a93784e5e33e1360729fbfedaa Mon Sep 17 00:00:00 2001 From: Hubert Kario Date: Thu, 26 Sep 2019 18:07:54 +0200 Subject: [PATCH 2/8] consistent error messages for unexpected types so that they have the same formatting as the ones for integer and sequence types --- src/ecdsa/der.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ecdsa/der.py b/src/ecdsa/der.py index 4ae2d3c7..e4d64d56 100644 --- a/src/ecdsa/der.py +++ b/src/ecdsa/der.py @@ -65,8 +65,8 @@ def encode_number(n): def remove_constructed(string): s0 = string[0] if isinstance(string[0], integer_types) else ord(string[0]) if (s0 & 0xe0) != 0xa0: - raise UnexpectedDER("wanted constructed tag (0xa0-0xbf), got 0x%02x" - % s0) + raise UnexpectedDER("wanted type 'constructed tag' (0xa0-0xbf), " + "got 0x%02x" % s0) tag = s0 & 0x1f length, llen = read_length(string[1:]) body = string[1+llen:1+llen+length] @@ -91,7 +91,7 @@ def remove_sequence(string): def remove_octet_string(string): if not string.startswith(b("\x04")): n = string[0] if isinstance(string[0], integer_types) else ord(string[0]) - raise UnexpectedDER("wanted octetstring (0x04), got 0x%02x" % n) + raise UnexpectedDER("wanted type 'octetstring' (0x04), got 0x%02x" % n) length, llen = read_length(string[1:]) body = string[1+llen:1+llen+length] rest = string[1+llen+length:] @@ -101,7 +101,7 @@ def remove_octet_string(string): def remove_object(string): if not string.startswith(b("\x06")): n = string[0] if isinstance(string[0], integer_types) else ord(string[0]) - raise UnexpectedDER("wanted object (0x06), got 0x%02x" % n) + raise UnexpectedDER("wanted type 'object' (0x06), got 0x%02x" % n) length, lengthlength = read_length(string[1:]) body = string[1+lengthlength:1+lengthlength+length] rest = string[1+lengthlength+length:] From 487f6d3fd6e0e50133795764aaefcd04355379b4 Mon Sep 17 00:00:00 2001 From: Hubert Kario Date: Thu, 26 Sep 2019 18:22:09 +0200 Subject: [PATCH 3/8] translate the UnexpectedDER exception to BadSignatureError the users of VerifyingKey.verify() and VerifyingKey.verify_digest() should not need to use multiple exceptions to correctly catch invalid signatures --- src/ecdsa/keys.py | 7 +++- src/ecdsa/test_malformed_sigs.py | 68 ++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 src/ecdsa/test_malformed_sigs.py diff --git a/src/ecdsa/keys.py b/src/ecdsa/keys.py index aabc6b46..a0aac93a 100644 --- a/src/ecdsa/keys.py +++ b/src/ecdsa/keys.py @@ -136,11 +136,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 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: diff --git a/src/ecdsa/test_malformed_sigs.py b/src/ecdsa/test_malformed_sigs.py new file mode 100644 index 00000000..eaf022e4 --- /dev/null +++ b/src/ecdsa/test_malformed_sigs.py @@ -0,0 +1,68 @@ +from __future__ import with_statement, division + +import unittest +import os +import time +import shutil +import subprocess +import pytest +from binascii import hexlify, unhexlify +from hashlib import sha1, sha256, sha512 +import hashlib + +from six import b, binary_type +from .keys import SigningKey, VerifyingKey +from .keys import BadSignatureError +from . import util +from .util import sigencode_der, sigencode_strings +from .util import sigdecode_der, sigdecode_strings +from .curves import curves, NIST256p, NIST521p +from .ellipticcurve import Point +from . import der +from . import rfc6979 +import copy + +sigs = [] +example_data = b("some data to sign") + +# Just NIST256p with SHA256 is 560 test cases, all curves with all hashes is +# few thousand slow test cases; execute the most interesting only + +#for curve in curves: +for curve in [NIST256p]: + #for hash_alg in ["md5", "sha1", "sha224", "sha256", "sha384", "sha512"]: + for hash_alg in ["sha256"]: + key = SigningKey.generate(curve) + signature = key.sign(example_data, hashfunc=getattr(hashlib, hash_alg), + sigencode=sigencode_der) + for pos in range(len(signature)): + for xor in (1< Date: Thu, 26 Sep 2019 18:44:51 +0200 Subject: [PATCH 4/8] give the same handling to string encoded signatures as to DER Verify that strings of unexpected lengths are rejected with the same BadSignatureError exception --- src/ecdsa/keys.py | 4 +-- src/ecdsa/test_malformed_sigs.py | 61 +++++++++++++++++++++----------- src/ecdsa/util.py | 26 ++++++++++++-- 3 files changed, 65 insertions(+), 26 deletions(-) diff --git a/src/ecdsa/keys.py b/src/ecdsa/keys.py index a0aac93a..587d1d6c 100644 --- a/src/ecdsa/keys.py +++ b/src/ecdsa/keys.py @@ -7,7 +7,7 @@ from .ecdsa import RSZeroError 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 @@ -138,7 +138,7 @@ def verify_digest(self, signature, digest, sigdecode=sigdecode_string): number = string_to_number(digest) try: r, s = sigdecode(signature, self.pubkey.order) - except der.UnexpectedDER as e: + except (der.UnexpectedDER, MalformedSignature) as e: raise BadSignatureError("Malformed formatting of signature", e) sig = ecdsa.Signature(r, s) if self.pubkey.verifies(number, sig): diff --git a/src/ecdsa/test_malformed_sigs.py b/src/ecdsa/test_malformed_sigs.py index eaf022e4..f6bea9c8 100644 --- a/src/ecdsa/test_malformed_sigs.py +++ b/src/ecdsa/test_malformed_sigs.py @@ -1,28 +1,16 @@ from __future__ import with_statement, division -import unittest -import os -import time -import shutil -import subprocess import pytest -from binascii import hexlify, unhexlify -from hashlib import sha1, sha256, sha512 import hashlib from six import b, binary_type from .keys import SigningKey, VerifyingKey from .keys import BadSignatureError -from . import util -from .util import sigencode_der, sigencode_strings -from .util import sigdecode_der, sigdecode_strings +from .util import sigencode_der, sigencode_string +from .util import sigdecode_der, sigdecode_string from .curves import curves, NIST256p, NIST521p -from .ellipticcurve import Point -from . import der -from . import rfc6979 -import copy -sigs = [] +der_sigs = [] example_data = b("some data to sign") # Just NIST256p with SHA256 is 560 test cases, all curves with all hashes is @@ -37,14 +25,14 @@ sigencode=sigencode_der) for pos in range(len(signature)): for xor in (1< Date: Thu, 26 Sep 2019 19:12:45 +0200 Subject: [PATCH 5/8] fix length decoding the same issues as with decoding integers happen with the NIST521p curve as it's large enough that the length encoding of some fields needs to use multi-byte encoding --- src/ecdsa/der.py | 4 ++++ src/ecdsa/test_malformed_sigs.py | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/ecdsa/der.py b/src/ecdsa/der.py index e4d64d56..037af979 100644 --- a/src/ecdsa/der.py +++ b/src/ecdsa/der.py @@ -169,6 +169,8 @@ def encode_length(l): 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 @@ -176,6 +178,8 @@ def read_length(string): # 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 byte is 0") if llen > len(string)-1: raise UnexpectedDER("ran out of length bytes") return int(binascii.hexlify(string[1:1+llen]), 16), 1+llen diff --git a/src/ecdsa/test_malformed_sigs.py b/src/ecdsa/test_malformed_sigs.py index f6bea9c8..fffb1b3b 100644 --- a/src/ecdsa/test_malformed_sigs.py +++ b/src/ecdsa/test_malformed_sigs.py @@ -17,7 +17,7 @@ # few thousand slow test cases; execute the most interesting only #for curve in curves: -for curve in [NIST256p]: +for curve in [NIST521p]: #for hash_alg in ["md5", "sha1", "sha224", "sha256", "sha384", "sha512"]: for hash_alg in ["sha256"]: key = SigningKey.generate(curve) From 091ca06422fc0f24becee3715fd65e236ca688e7 Mon Sep 17 00:00:00 2001 From: Hubert Kario Date: Thu, 26 Sep 2019 19:15:54 +0200 Subject: [PATCH 6/8] explicitly specify the distro to get py26 and py33 --- .travis.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.travis.yml b/.travis.yml index 2aec5d1f..9151a873 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,3 +1,6 @@ +# workaround for 3.7 not available in default configuration +# travis-ci/travis-ci#9815 +dist: trusty sudo: false language: python cache: pip From 710e7c882437d4381b3ac14e523cdf59a295b084 Mon Sep 17 00:00:00 2001 From: Hubert Kario Date: Fri, 27 Sep 2019 13:58:32 +0200 Subject: [PATCH 7/8] make variable names in remove_integer more aproppriate --- src/ecdsa/der.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ecdsa/der.py b/src/ecdsa/der.py index 037af979..fec550d5 100644 --- a/src/ecdsa/der.py +++ b/src/ecdsa/der.py @@ -133,9 +133,9 @@ def remove_integer(string): 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) \ + msb = numberbytes[0] if isinstance(numberbytes[0], integer_types) \ else ord(numberbytes[0]) - if not nbytes < 0x80: + if not msb < 0x80: raise UnexpectedDER("Negative integers are not supported") return int(binascii.hexlify(numberbytes), 16), rest From 2c3db7c5724ff4983c744d4594935209e36400d3 Mon Sep 17 00:00:00 2001 From: Hubert Kario Date: Fri, 27 Sep 2019 17:50:10 +0200 Subject: [PATCH 8/8] ensure that the encoding is actually the minimal one for length and integer --- build-requirements-2.6.txt | 1 + src/ecdsa/der.py | 17 ++++++- src/ecdsa/test_der.py | 94 ++++++++++++++++++++++++++++++++++++++ tox.ini | 1 + 4 files changed, 111 insertions(+), 2 deletions(-) create mode 100644 src/ecdsa/test_der.py diff --git a/build-requirements-2.6.txt b/build-requirements-2.6.txt index de0022ee..980e00df 100644 --- 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/src/ecdsa/der.py b/src/ecdsa/der.py index fec550d5..3ed7f6c3 100644 --- a/src/ecdsa/der.py +++ b/src/ecdsa/der.py @@ -137,6 +137,15 @@ def remove_integer(string): 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 @@ -179,9 +188,13 @@ def read_length(string): # big-endian llen = num & 0x7f if not llen: - raise UnexpectedDER("Invalid length encoding, length byte is 0") + 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 diff --git a/src/ecdsa/test_der.py b/src/ecdsa/test_der.py new file mode 100644 index 00000000..76bdfd46 --- /dev/null +++ b/src/ecdsa/test_der.py @@ -0,0 +1,94 @@ + +# 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_minimal_with_high_bit_set(self): + val, rem = remove_integer(b('\x02\x02\x00\x80')) + + self.assertEqual(val, 0x80) + self.assertFalse(rem) + + 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/tox.ini b/tox.ini index 6c20854a..c9b848c1 100644 --- a/tox.ini +++ b/tox.ini @@ -6,6 +6,7 @@ envlist = py26, py27, py33, py34, py35, py36, py37, pypy, pypy3 deps = py{33}: py<1.5 py{33}: pytest<3.3 + py{26}: unittest2 py{26,27,34,35,36,37,py,py3}: pytest py{33}: wheel<0.30 coverage