Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tolerate NULL params in ECDSA SHA2 AlgorithmIdentifier #9002

Merged
merged 2 commits into from
Jun 1, 2023
Merged
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
2 changes: 2 additions & 0 deletions docs/development/test-vectors.rst
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,8 @@ Custom X.509 Vectors
* ``rsa_pss_sha256_no_null.pem`` - A certificate with an RSA PSS signature
with no encoded ``NULL`` for the PSS hash algorithm parameters. This certificate
was generated by LibreSSL.
* ``ecdsa_null_alg.pem`` - A certificate with an ECDSA signature with ``NULL``
algorithm parameters. This encoding is invalid, but was generated by Java 11.

Custom X.509 Request Vectors
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand Down
12 changes: 8 additions & 4 deletions src/rust/cryptography-x509/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,18 @@ pub enum AlgorithmParameters<'a> {
#[defined_by(oid::ED448_OID)]
Ed448,

// These ECDSA algorithms should have no parameters,
// but Java 11 (up to at least 11.0.19) encodes them
// with NULL parameters. The JDK team is looking to
// backport the fix as of June 2023.
#[defined_by(oid::ECDSA_WITH_SHA224_OID)]
EcDsaWithSha224,
EcDsaWithSha224(Option<asn1::Null>),
#[defined_by(oid::ECDSA_WITH_SHA256_OID)]
EcDsaWithSha256,
EcDsaWithSha256(Option<asn1::Null>),
#[defined_by(oid::ECDSA_WITH_SHA384_OID)]
EcDsaWithSha384,
EcDsaWithSha384(Option<asn1::Null>),
#[defined_by(oid::ECDSA_WITH_SHA512_OID)]
EcDsaWithSha512,
EcDsaWithSha512(Option<asn1::Null>),

#[defined_by(oid::ECDSA_WITH_SHA3_224_OID)]
EcDsaWithSha3_224,
Expand Down
30 changes: 29 additions & 1 deletion src/rust/src/x509/certificate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::asn1::{
use crate::error::{CryptographyError, CryptographyResult};
use crate::x509::{extensions, sct, sign};
use crate::{exceptions, x509};
use cryptography_x509::common::Asn1ReadableOrWritable;
use cryptography_x509::common::{AlgorithmParameters, Asn1ReadableOrWritable};
use cryptography_x509::extensions::Extension;
use cryptography_x509::extensions::{
AuthorityKeyIdentifier, BasicConstraints, DisplayText, DistributionPoint,
Expand Down Expand Up @@ -391,6 +391,10 @@ fn load_der_x509_certificate(
// determine if the serial is negative and raise a warning if it is. We want to drop support
// for this sort of invalid encoding eventually.
warn_if_negative_serial(py, raw.borrow_value().tbs_cert.serial.as_bytes())?;
// determine if the signature algorithm has incorrect parameters and raise a warning if it
// does. this is a bug in JDK11 and we want to drop support for it eventually.
warn_if_invalid_ecdsa_params(py, raw.borrow_value().signature_alg.params.clone())?;
warn_if_invalid_ecdsa_params(py, raw.borrow_value().tbs_cert.signature_alg.params.clone())?;

Ok(Certificate {
raw,
Expand All @@ -413,6 +417,30 @@ fn warn_if_negative_serial(py: pyo3::Python<'_>, bytes: &'_ [u8]) -> pyo3::PyRes
Ok(())
}

fn warn_if_invalid_ecdsa_params(
py: pyo3::Python<'_>,
params: AlgorithmParameters<'_>,
) -> pyo3::PyResult<()> {
match params {
AlgorithmParameters::EcDsaWithSha224(Some(..))
| AlgorithmParameters::EcDsaWithSha256(Some(..))
| AlgorithmParameters::EcDsaWithSha384(Some(..))
| AlgorithmParameters::EcDsaWithSha512(Some(..)) => {
let cryptography_warning = py
.import(pyo3::intern!(py, "cryptography.utils"))?
.getattr(pyo3::intern!(py, "DeprecatedIn41"))?;
pyo3::PyErr::warn(
py,
cryptography_warning,
"The parsed certificate contains a NULL parameter value in its signature algorithm parameters. This is invalid and will be rejected in a future version of cryptography. If this certificate was created via Java, please upgrade to JDK16+ or the latest JDK11 once a fix is issued. If this certificate was created in some other fashion please report the issue to the cryptography issue tracker. See https://github.com/pyca/cryptography/issues/8996 for more details.",
2,
)?;
}
_ => {}
}
Ok(())
}

fn parse_display_text(
py: pyo3::Python<'_>,
text: DisplayText<'_>,
Expand Down
44 changes: 28 additions & 16 deletions src/rust/src/x509/sign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,19 +234,19 @@ pub(crate) fn compute_signature_algorithm<'p>(

(KeyType::Ec, HashType::Sha224) => Ok(common::AlgorithmIdentifier {
oid: asn1::DefinedByMarker::marker(),
params: common::AlgorithmParameters::EcDsaWithSha224,
params: common::AlgorithmParameters::EcDsaWithSha224(None),
}),
(KeyType::Ec, HashType::Sha256) => Ok(common::AlgorithmIdentifier {
oid: asn1::DefinedByMarker::marker(),
params: common::AlgorithmParameters::EcDsaWithSha256,
params: common::AlgorithmParameters::EcDsaWithSha256(None),
}),
(KeyType::Ec, HashType::Sha384) => Ok(common::AlgorithmIdentifier {
oid: asn1::DefinedByMarker::marker(),
params: common::AlgorithmParameters::EcDsaWithSha384,
params: common::AlgorithmParameters::EcDsaWithSha384(None),
}),
(KeyType::Ec, HashType::Sha512) => Ok(common::AlgorithmIdentifier {
oid: asn1::DefinedByMarker::marker(),
params: common::AlgorithmParameters::EcDsaWithSha512,
params: common::AlgorithmParameters::EcDsaWithSha512(None),
}),
(KeyType::Ec, HashType::Sha3_224) => Ok(common::AlgorithmIdentifier {
oid: asn1::DefinedByMarker::marker(),
Expand Down Expand Up @@ -483,10 +483,10 @@ fn identify_key_type_for_algorithm_params(
| common::AlgorithmParameters::RsaWithSha3_384(..)
| common::AlgorithmParameters::RsaWithSha3_512(..)
| common::AlgorithmParameters::RsaPss(..) => Ok(KeyType::Rsa),
common::AlgorithmParameters::EcDsaWithSha224
| common::AlgorithmParameters::EcDsaWithSha256
| common::AlgorithmParameters::EcDsaWithSha384
| common::AlgorithmParameters::EcDsaWithSha512
common::AlgorithmParameters::EcDsaWithSha224(..)
| common::AlgorithmParameters::EcDsaWithSha256(..)
| common::AlgorithmParameters::EcDsaWithSha384(..)
| common::AlgorithmParameters::EcDsaWithSha512(..)
| common::AlgorithmParameters::EcDsaWithSha3_224
| common::AlgorithmParameters::EcDsaWithSha3_256
| common::AlgorithmParameters::EcDsaWithSha3_384
Expand Down Expand Up @@ -616,10 +616,10 @@ pub(crate) fn identify_signature_algorithm_parameters<'p>(
.call0()?;
Ok(pkcs)
}
common::AlgorithmParameters::EcDsaWithSha224
| common::AlgorithmParameters::EcDsaWithSha256
| common::AlgorithmParameters::EcDsaWithSha384
| common::AlgorithmParameters::EcDsaWithSha512
common::AlgorithmParameters::EcDsaWithSha224(_)
| common::AlgorithmParameters::EcDsaWithSha256(_)
| common::AlgorithmParameters::EcDsaWithSha384(_)
| common::AlgorithmParameters::EcDsaWithSha512(_)
| common::AlgorithmParameters::EcDsaWithSha3_224
| common::AlgorithmParameters::EcDsaWithSha3_256
| common::AlgorithmParameters::EcDsaWithSha3_384
Expand Down Expand Up @@ -682,10 +682,22 @@ mod tests {
&common::AlgorithmParameters::RsaWithSha3_512(Some(())),
KeyType::Rsa,
),
(&common::AlgorithmParameters::EcDsaWithSha224, KeyType::Ec),
(&common::AlgorithmParameters::EcDsaWithSha256, KeyType::Ec),
(&common::AlgorithmParameters::EcDsaWithSha384, KeyType::Ec),
(&common::AlgorithmParameters::EcDsaWithSha512, KeyType::Ec),
(
&common::AlgorithmParameters::EcDsaWithSha224(None),
KeyType::Ec,
),
(
&common::AlgorithmParameters::EcDsaWithSha256(None),
KeyType::Ec,
),
(
&common::AlgorithmParameters::EcDsaWithSha384(None),
KeyType::Ec,
),
(
&common::AlgorithmParameters::EcDsaWithSha512(None),
KeyType::Ec,
),
(&common::AlgorithmParameters::EcDsaWithSha3_224, KeyType::Ec),
(&common::AlgorithmParameters::EcDsaWithSha3_256, KeyType::Ec),
(&common::AlgorithmParameters::EcDsaWithSha3_384, KeyType::Ec),
Expand Down
15 changes: 15 additions & 0 deletions tests/x509/test_x509.py
Original file line number Diff line number Diff line change
Expand Up @@ -5199,6 +5199,21 @@ def test_load_ecdsa_cert(self, backend):
cert.signature_algorithm_parameters,
)

def test_load_ecdsa_cert_null_alg_params(self, backend):
"""
This test verifies that we successfully load certificates with encoded
null parameters in the signature AlgorithmIdentifier. This is invalid,
but Java 11 (up to at least 11.0.19) generates certificates with this
encoding so we need to tolerate it at the moment.
"""
with pytest.warns(utils.DeprecatedIn41):
cert = _load_cert(
os.path.join("x509", "custom", "ecdsa_null_alg.pem"),
x509.load_pem_x509_certificate,
)
assert isinstance(cert.signature_hash_algorithm, hashes.SHA256)
assert isinstance(cert.public_key(), ec.EllipticCurvePublicKey)

def test_load_bitstring_dn(self, backend):
cert = _load_cert(
os.path.join("x509", "scottishpower-bitstring-dn.pem"),
Expand Down
9 changes: 9 additions & 0 deletions vectors/cryptography_vectors/x509/custom/ecdsa_null_alg.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
-----BEGIN CERTIFICATE-----
MIIBNDCB2aADAgECAgRnI7YfMAwGCCqGSM49BAMCBQAwDzENMAsGA1UEAxMEdGVz
dDAeFw0yMzA1MzExMjI5MDNaFw0yNDA1MjUxMjI5MDNaMA8xDTALBgNVBAMTBHRl
c3QwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAS2LuMFnF5OcuYcldiufvppacg2
8fF/KeJ/4QLMOTbnkatgx5wNPOUvlkzfT31MscwYyzkv1oTqe58iQ+R75C27oyEw
HzAdBgNVHQ4EFgQUD6COpW8C9Ns86r2BDE0jP0teCTswDAYIKoZIzj0EAwIFAANI
ADBFAiBKOlNsFpW6Bz7CK7Z5zXrCetnMiSH3NrbKSZBXJV62KQIhAKmjGu3rxlJr
xXpK+Uz8AsoFJ0BlgqPpdMtTGSrDq1AN
-----END CERTIFICATE-----