Skip to content

Commit

Permalink
Fix ECDSA signature encoding/decoding (#150)
Browse files Browse the repository at this point in the history
According to https://www.w3.org/TR/xmldsig-core1/#sec-ECDSA,
"The signature value consists of the base64 encoding of the
concatenation of two octet-streams that respectively result
from the octet-encoding of the values r and s in that order."

Previously, the signature value was assumed to be a DER-encoded
signature, which is incorrect. There was already code for dealing
with a similiar situation for DSA signatures, but not ECDSA.
  • Loading branch information
RJPercival authored Jun 7, 2020
1 parent a76018e commit 01e783e
Showing 1 changed file with 31 additions and 4 deletions.
35 changes: 31 additions & 4 deletions signxml/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from lxml import etree
from lxml.etree import Element, SubElement

from cryptography.hazmat.primitives.asymmetric import rsa, dsa, ec
from cryptography.hazmat.primitives.asymmetric import dsa, ec, rsa, utils
from cryptography.hazmat.primitives.asymmetric.padding import PKCS1v15
from cryptography.hazmat.primitives.hashes import Hash, SHA1, SHA224, SHA256, SHA384, SHA512
from cryptography.hazmat.backends import default_backend
Expand Down Expand Up @@ -386,6 +386,12 @@ def sign(self, data, key=None, passphrase=None, cert=None, reference_uri=None, k
r = decoded_signature['r']
s = decoded_signature['s']
signature = long_to_bytes(r).rjust(32, b"\0") + long_to_bytes(s).rjust(32, b"\0")
elif self.sign_alg.startswith("ecdsa-"):
# Note: The output of the ECDSA signer is a DER-encoded ASN.1 sequence of two DER integers.
(r, s) = utils.decode_dss_signature(signature)
int_len = key.key_size // 8
signature = long_to_bytes(r, blocksize=int_len)
signature += long_to_bytes(s, blocksize=int_len)

signature_value_element.text = ensure_str(b64encode(signature))

Expand Down Expand Up @@ -553,9 +559,14 @@ def _verify_signature_with_pubkey(self, signed_info_c14n, raw_signature, key_val
y = bytes_to_long(key_data[len(key_data)//2:])
curve_class = self.known_ecdsa_curves[named_curve.get("URI")]
key = ec.EllipticCurvePublicNumbers(x=x, y=y, curve=curve_class()).public_key(backend=default_backend())
key.verify(raw_signature,
data=signed_info_c14n,
signature_algorithm=ec.ECDSA(self._get_signature_digest_method(signature_alg)))
dss_signature = self._encode_dss_signature(raw_signature, key.key_size)
key.verify(
dss_signature,
data=signed_info_c14n,
signature_algorithm=ec.ECDSA(
self._get_signature_digest_method(signature_alg)
),
)
elif "dsa-" in signature_alg:
dsa_key_value = self._find(key_value, "DSAKeyValue")
p = self._get_long(dsa_key_value, "P")
Expand All @@ -581,6 +592,18 @@ def _verify_signature_with_pubkey(self, signed_info_c14n, raw_signature, key_val
else:
raise NotImplementedError()

def _encode_dss_signature(self, raw_signature, key_size_bits):
want_raw_signature_len = key_size_bits // 8 * 2
if len(raw_signature) != want_raw_signature_len:
raise InvalidSignature(
"Expected %d byte SignatureValue, got %d"
% (want_raw_signature_len, len(raw_signature))
)
int_len = len(raw_signature) // 2
r = bytes_to_long(raw_signature[:int_len])
s = bytes_to_long(raw_signature[int_len:])
return utils.encode_dss_signature(r, s)

def _get_inclusive_ns_prefixes(self, transform_node):
inclusive_namespaces = transform_node.find("./ec:InclusiveNamespaces[@PrefixList]", namespaces=namespaces)
if inclusive_namespaces is None:
Expand Down Expand Up @@ -758,6 +781,10 @@ def verify(self, data, require_x509=True, x509_cert=None, cert_subject_name=None
raise InvalidSignature("Certificate subject common name mismatch")

signature_digest_method = self._get_signature_digest_method(signature_alg).name
if "ecdsa-" in signature_alg:
raw_signature = self._encode_dss_signature(
raw_signature, signing_cert.get_pubkey().bits()
)
try:
verify(signing_cert, raw_signature, signed_info_c14n, signature_digest_method)
except OpenSSLCryptoError as e:
Expand Down

0 comments on commit 01e783e

Please sign in to comment.