Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 43 additions & 1 deletion ecdsa/keys.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import binascii

from . import ecdsa
from . import ellipticcurve
from . import der
from . import rfc6979
from .curves import NIST192p, find_curve
from .numbertheory import square_root_mod_prime
from .six import PY3, b
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 .six import PY3, b
from hashlib import sha1


Expand Down Expand Up @@ -78,6 +80,32 @@ def from_der(klass, string):
assert point_str.startswith(b("\x00\x04"))
return klass.from_string(point_str[2:], curve)

@classmethod
def from_sec(klass, string, curve=NIST192p, hashfunc=sha1,
validate_point=True):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this couldn't be folded into from_string() method—autodetect the format and use the necessary decoding

"""Convert a public key in SEC binary format to a verifying key."""
# based on code from https://github.com/richardkiss/pycoin
if string.startswith(b('\x04')):
# uncompressed
return klass.from_string(string[1:], curve, hashfunc,
validate_point)
elif string.startswith(b('\x02')) or string.startswith(b('\x03')):
# compressed
is_even = string.startswith(b('\x02'))
x = string_to_number(string[1:])
order = curve.order
p = curve.curve.p()
alpha = (pow(x, 3, p) + (curve.curve.a() * x) + curve.curve.b()) % p
beta = square_root_mod_prime(alpha, p)
if is_even == bool(beta & 1):
y = p - beta
else:
y = beta
if validate_point:
assert ecdsa.point_is_valid(curve.generator, x, y)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertions are optimised out when compiling

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, what this means is that the klass.from_public_point() call will return a RuntimeError("Generator point has x or y out of range.") from ecdsa.Public_key initialization. What change would you make here?

Copy link
Contributor Author

@mdxs mdxs Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I just add a "pass" it will work just fine, right? (similar issue around line 50)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a small test using:

print "a"
if True:
    assert False
print "b"

and when I run that with -O it prints "a" and "b" showing that it "pass"es, while a non-optimised run will throw an AssertionError after printing "a" (as expected in this example).

So I actually think there is no need to change this PR, nor the current https://github.com/warner/python-ecdsa/blob/master/src/ecdsa/keys.py#L49

@tomato42 and @techguy613 can you confirm that there is no need to change the PR, or clarify why it should and how?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't change this PR as it is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does need to to be changed - as asserts are optimised out, the call to ecdsa.point_is_valid() will be optimised out

now, either this call is necessary - then it's a big issue as the inputs may be attacker controlled - or this call is not necessary and then the whole if is superfluous

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so why not remove the whole if statement then and the issue is resolved? The point is checked to be within the curve at a deeper level and will still throw an exception as I pointed out a little earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomato42: with your arguments that it needs to change, the current https://github.com/warner/python-ecdsa/blob/master/src/ecdsa/keys.py#L49 construct also needs a change. I will adjust this PR based on how that is (proposed to be) changed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdxs: that line 49 need to actually be changed to not use assert but the check needs to stay - the uncompressed point format can be malformed so checking if the point is on curve is needed

point = ellipticcurve.Point(curve.curve, x, y, order)
return klass.from_public_point(point, curve, hashfunc)

def to_string(self):
# VerifyingKey.from_string(vk.to_string()) == vk as long as the
# curves are the same: the curve itself is not included in the
Expand All @@ -99,6 +127,20 @@ def to_der(self):
self.curve.encoded_oid),
der.encode_bitstring(point_str))

def to_sec(self, compressed=True):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, to_string() could be extended too—with an optional argument that specifies if the encoding should be "raw" (default for backwards compatibility), "X9.62 uncompressed" or "X9.62 compressed"

"""Convert verifying key to the SEC binary format (as used by OpenSSL)."""
# based on code from https://github.com/richardkiss/pycoin
order = self.pubkey.order
x_str = number_to_string(self.pubkey.point.x(), order)
if compressed:
if self.pubkey.point.y() & 1:
return b('\x03') + x_str
else:
return b('\x02') + x_str
else:
y_str = number_to_string(self.pubkey.point.y(), order)
return b('\x04') + x_str + y_str

def verify(self, signature, data, hashfunc=None, sigdecode=sigdecode_string):
hashfunc = hashfunc or self.default_hashfunc
digest = hashfunc(data).digest()
Expand Down
25 changes: 25 additions & 0 deletions ecdsa/test_pyecdsa.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,21 @@ def test_basic(self):
pub2 = VerifyingKey.from_string(pub.to_string())
self.assertTrue(pub2.verify(sig, data))

def test_sec(self):
for i in range (20):
skey = SigningKey.generate()
pkey0 = skey.get_verifying_key()
sec0 = pkey0.to_sec()
pkey1 = VerifyingKey.from_sec (sec0)
self.assertEqual(pkey0.to_string(), pkey1.to_string())
pkey2 = VerifyingKey.from_sec(unhexlify(b(
'045b9dfc2af65bd5fb0bd01103ab21e9cfeb1eeafa10795d6801b14e09beadd7f8'
'55981f2803fc3c07edfc2435fbf2326d65d3f237f0bcc2399514255b8d4285c5')),
curve=SECP256k1)
self.assertEqual(
pkey2.to_sec(compressed=True), unhexlify(b(
'035b9dfc2af65bd5fb0bd01103ab21e9cfeb1eeafa10795d6801b14e09beadd7f8')))

def test_deterministic(self):
data = b("blahblah")
secexp = int("9d0219792467d7d37b4d43298a7d0c05", 16)
Expand Down Expand Up @@ -253,6 +268,16 @@ def test_pubkey_strings(self):
VerifyingKey.from_der, pub1_der + b("junk"))
badpub = VerifyingKey.from_der(pub1_der)

pub1_sec = pub1.to_sec(compressed=False)
self.assertEqual(type(pub1_sec), binary_type)
pub2 = VerifyingKey.from_sec(pub1_sec, curve=NIST256p)
self.assertTruePubkeysEqual(pub1, pub2)

pub1_sec = pub1.to_sec(compressed=True)
self.assertEqual(type(pub1_sec), binary_type)
pub2 = VerifyingKey.from_sec(pub1_sec, curve=NIST256p)
self.assertTruePubkeysEqual(pub1, pub2)

class FakeGenerator:
def order(self):
return 123456789
Expand Down