From 1bfb06fa94f3d61cc9dfc4048ddd471855503ed3 Mon Sep 17 00:00:00 2001 From: Hubert Kario Date: Sun, 3 Nov 2019 14:07:10 +0100 Subject: [PATCH 1/3] der: fix encoding and decoding OIDs make the code handle oids with a large second subidentifier (fixes #155) make the code raise correct exception when the encoded multi-byte subidentifier is missing the last byte move OID test coverage to test_der module and extend it update the OID generators in random DER generator to generate OIDs with large second subidentifier --- src/ecdsa/der.py | 28 +++++-- src/ecdsa/test_der.py | 138 ++++++++++++++++++++++++++++++- src/ecdsa/test_malformed_sigs.py | 5 +- src/ecdsa/test_pyecdsa.py | 11 --- 4 files changed, 158 insertions(+), 24 deletions(-) diff --git a/src/ecdsa/der.py b/src/ecdsa/der.py index 187b90bc..89cd7dde 100644 --- a/src/ecdsa/der.py +++ b/src/ecdsa/der.py @@ -3,6 +3,7 @@ import binascii import base64 import warnings +from itertools import chain from six import int2byte, b, text_type from ._compat import str_idx_as_int @@ -97,12 +98,10 @@ def encode_octet_string(s): def encode_oid(first, second, *pieces): - assert first <= 2 - assert second <= 39 - encoded_pieces = [int2byte(40*first+second)] + [encode_number(p) - for p in pieces] - body = b('').join(encoded_pieces) - return b('\x06') + encode_length(len(body)) + body + assert 0 <= first < 2 and 0 <= second <= 39 or first == 2 and 0 <= second + body = b''.join(chain([encode_number(40*first+second)], + (encode_number(p) for p in pieces))) + return b'\x06' + encode_length(len(body)) + body def encode_sequence(*encoded_pieces): @@ -157,20 +156,31 @@ def remove_octet_string(string): def remove_object(string): + if not string: + raise UnexpectedDER( + "Empty string does not encode an object identifier") if string[:1] != b"\x06": n = str_idx_as_int(string, 0) 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:] + if not body: + raise UnexpectedDER("Empty object identifier") + if len(body) != length: + raise UnexpectedDER( + "Length of object identifier longer than the provided buffer") numbers = [] while body: n, ll = read_number(body) numbers.append(n) body = body[ll:] n0 = numbers.pop(0) - first = n0//40 - second = n0-(40*first) + if n0 < 80: + first = n0 // 40 + else: + first = 2 + second = n0 - (40 * first) numbers.insert(0, first) numbers.insert(1, second) return tuple(numbers), rest @@ -209,7 +219,7 @@ def read_number(string): llen = 0 # base-128 big endian, with b7 set in all but the last byte while True: - if llen > len(string): + if llen >= len(string): raise UnexpectedDER("ran out of length bytes") number = number << 7 d = str_idx_as_int(string, llen) diff --git a/src/ecdsa/test_der.py b/src/ecdsa/test_der.py index a44befb1..b83cee28 100644 --- a/src/ecdsa/test_der.py +++ b/src/ecdsa/test_der.py @@ -1,16 +1,20 @@ # compatibility with Python 2.6, for that we need unittest2 package, # which is not available on 3.3 or 3.4 +import warnings +from binascii import hexlify try: import unittest2 as unittest except ImportError: import unittest -from .der import remove_integer, UnexpectedDER, read_length, encode_bitstring,\ - remove_bitstring from six import b +import hypothesis.strategies as st +from hypothesis import given, example import pytest -import warnings from ._compat import str_idx_as_int +from .curves import NIST256p, NIST224p +from .der import remove_integer, UnexpectedDER, read_length, encode_bitstring,\ + remove_bitstring, remove_object, encode_oid class TestRemoveInteger(unittest.TestCase): @@ -242,3 +246,131 @@ def test_bytes(self): def test_bytearray(self): self.assertEqual(115, str_idx_as_int(bytearray(b'str'), 0)) + + +class TestEncodeOid(unittest.TestCase): + def test_pub_key_oid(self): + oid_ecPublicKey = encode_oid(1, 2, 840, 10045, 2, 1) + self.assertEqual(hexlify(oid_ecPublicKey), b("06072a8648ce3d0201")) + + def test_nist224p_oid(self): + self.assertEqual(hexlify(NIST224p.encoded_oid), b("06052b81040021")) + + def test_nist256p_oid(self): + self.assertEqual(hexlify(NIST256p.encoded_oid), + b"06082a8648ce3d030107") + + def test_large_second_subid(self): + # from X.690, section 8.19.5 + oid = encode_oid(2, 999, 3) + self.assertEqual(oid, b'\x06\x03\x88\x37\x03') + + def test_with_two_subids(self): + oid = encode_oid(2, 999) + self.assertEqual(oid, b'\x06\x02\x88\x37') + + def test_zero_zero(self): + oid = encode_oid(0, 0) + self.assertEqual(oid, b'\x06\x01\x00') + + def test_with_wrong_types(self): + with self.assertRaises((TypeError, AssertionError)): + encode_oid(0, None) + + def test_with_small_first_large_second(self): + with self.assertRaises(AssertionError): + encode_oid(1, 40) + + def test_small_first_max_second(self): + oid = encode_oid(1, 39) + self.assertEqual(oid, b'\x06\x01\x4f') + + def test_with_invalid_first(self): + with self.assertRaises(AssertionError): + encode_oid(3, 39) + + +class TestRemoveObject(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.oid_ecPublicKey = encode_oid(1, 2, 840, 10045, 2, 1) + + def test_pub_key_oid(self): + oid, rest = remove_object(self.oid_ecPublicKey) + self.assertEqual(rest, b'') + self.assertEqual(oid, (1, 2, 840, 10045, 2, 1)) + + def test_with_extra_bytes(self): + oid, rest = remove_object(self.oid_ecPublicKey + b'more') + self.assertEqual(rest, b'more') + self.assertEqual(oid, (1, 2, 840, 10045, 2, 1)) + + def test_with_large_second_subid(self): + # from X.690, section 8.19.5 + oid, rest = remove_object(b'\x06\x03\x88\x37\x03') + self.assertEqual(rest, b'') + self.assertEqual(oid, (2, 999, 3)) + + def test_with_missing_last_byte_of_multi_byte(self): + with self.assertRaises(UnexpectedDER): + remove_object(b'\x06\x03\x88\x37\x83') + + def test_with_two_subids(self): + oid, rest = remove_object(b'\x06\x02\x88\x37') + self.assertEqual(rest, b'') + self.assertEqual(oid, (2, 999)) + + def test_zero_zero(self): + oid, rest = remove_object(b'\x06\x01\x00') + self.assertEqual(rest, b'') + self.assertEqual(oid, (0, 0)) + + def test_empty_string(self): + with self.assertRaises(UnexpectedDER): + remove_object(b'') + + def test_missing_length(self): + with self.assertRaises(UnexpectedDER): + remove_object(b'\x06') + + def test_empty_oid(self): + with self.assertRaises(UnexpectedDER): + remove_object(b'\x06\x00') + + def test_empty_oid_overflow(self): + with self.assertRaises(UnexpectedDER): + remove_object(b'\x06\x01') + + def test_with_wrong_type(self): + with self.assertRaises(UnexpectedDER): + remove_object(b'\x04\x02\x88\x37') + + def test_with_too_long_length(self): + with self.assertRaises(UnexpectedDER): + remove_object(b'\x06\x03\x88\x37') + + +@st.composite +def st_oid(draw, max_value=2**512, max_size=50): + """ + Hypothesis strategy that returns valid OBJECT IDENTIFIERs as tuples + + :param max_value: maximum value of any single sub-identifier + :param max_size: maximum length of the generated OID + """ + first = draw(st.integers(min_value=0, max_value=2)) + if first < 2: + second = draw(st.integers(min_value=0, max_value=39)) + else: + second = draw(st.integers(min_value=0, max_value=max_value)) + rest = draw(st.lists(st.integers(min_value=0, max_value=max_value), + max_size=max_size)) + return (first, second) + tuple(rest) + + +@given(st_oid()) +def test_oids(ids): + encoded_oid = encode_oid(*ids) + decoded_oid, rest = remove_object(encoded_oid) + assert rest == b'' + assert decoded_oid == ids diff --git a/src/ecdsa/test_malformed_sigs.py b/src/ecdsa/test_malformed_sigs.py index e7bf5831..22ae4550 100644 --- a/src/ecdsa/test_malformed_sigs.py +++ b/src/ecdsa/test_malformed_sigs.py @@ -220,7 +220,10 @@ def st_der_oid(draw): Hypothesis strategy that returns DER OBJECT IDENTIFIER objects. """ first = draw(st.integers(min_value=0, max_value=2)) - second = draw(st.integers(min_value=0, max_value=39)) + if first < 2: + second = draw(st.integers(min_value=0, max_value=39)) + else: + second = draw(st.integers(min_value=0, max_value=2**512)) rest = draw(st.lists(st.integers(min_value=0, max_value=2**512), max_size=50)) return encode_oid(first, second, *rest) diff --git a/src/ecdsa/test_pyecdsa.py b/src/ecdsa/test_pyecdsa.py index 6782a040..3d1247ab 100644 --- a/src/ecdsa/test_pyecdsa.py +++ b/src/ecdsa/test_pyecdsa.py @@ -952,17 +952,6 @@ def do_test_to_openssl(self, curve): class DER(unittest.TestCase): - def test_oids(self): - oid_ecPublicKey = der.encode_oid(1, 2, 840, 10045, 2, 1) - self.assertEqual(hexlify(oid_ecPublicKey), b("06072a8648ce3d0201")) - self.assertEqual(hexlify(NIST224p.encoded_oid), b("06052b81040021")) - self.assertEqual(hexlify(NIST256p.encoded_oid), - b("06082a8648ce3d030107")) - x = oid_ecPublicKey + b("more") - x1, rest = der.remove_object(x) - self.assertEqual(x1, (1, 2, 840, 10045, 2, 1)) - self.assertEqual(rest, b("more")) - def test_integer(self): self.assertEqual(der.encode_integer(0), b("\x02\x01\x00")) self.assertEqual(der.encode_integer(1), b("\x02\x01\x01")) From a127819a57fd2480e9f1af591b3c33ef0d22abc6 Mon Sep 17 00:00:00 2001 From: Hubert Kario Date: Mon, 4 Nov 2019 20:14:38 +0100 Subject: [PATCH 2/3] der: don't accept padded subidentifiers --- src/ecdsa/der.py | 2 ++ src/ecdsa/test_der.py | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/src/ecdsa/der.py b/src/ecdsa/der.py index 89cd7dde..7db20f71 100644 --- a/src/ecdsa/der.py +++ b/src/ecdsa/der.py @@ -217,6 +217,8 @@ def remove_integer(string): def read_number(string): number = 0 llen = 0 + if str_idx_as_int(string, 0) == 0x80: + raise UnexpectedDER("Non minimal encoding of OID subidentifier") # base-128 big endian, with b7 set in all but the last byte while True: if llen >= len(string): diff --git a/src/ecdsa/test_der.py b/src/ecdsa/test_der.py index b83cee28..e6cd593d 100644 --- a/src/ecdsa/test_der.py +++ b/src/ecdsa/test_der.py @@ -311,6 +311,14 @@ def test_with_large_second_subid(self): self.assertEqual(rest, b'') self.assertEqual(oid, (2, 999, 3)) + def test_with_padded_first_subid(self): + with self.assertRaises(UnexpectedDER): + remove_object(b'\x06\x02\x80\x00') + + def test_with_padded_second_subid(self): + with self.assertRaises(UnexpectedDER): + remove_object(b'\x06\x04\x88\x37\x80\x01') + def test_with_missing_last_byte_of_multi_byte(self): with self.assertRaises(UnexpectedDER): remove_object(b'\x06\x03\x88\x37\x83') From 2dac8eef2080ca16a04c7ee0a009602417aded49 Mon Sep 17 00:00:00 2001 From: Hubert Kario Date: Tue, 5 Nov 2019 16:28:12 +0100 Subject: [PATCH 3/3] der: make the comment unambiguous the X.690 refers to bits from 1 to 8 rather than from 0 to 7 use the "MSB" as an unambiguous name --- src/ecdsa/der.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ecdsa/der.py b/src/ecdsa/der.py index 7db20f71..ad75b37b 100644 --- a/src/ecdsa/der.py +++ b/src/ecdsa/der.py @@ -219,7 +219,8 @@ def read_number(string): llen = 0 if str_idx_as_int(string, 0) == 0x80: raise UnexpectedDER("Non minimal encoding of OID subidentifier") - # base-128 big endian, with b7 set in all but the last byte + # base-128 big endian, with most significant bit set in all but the last + # byte while True: if llen >= len(string): raise UnexpectedDER("ran out of length bytes")