Respond with TLS alert "bad_certificate" if certificate parsing fails

[mj-vivavis]

Improve the error handling in case a invalid (syntactically incorrect) certificate is received in a TLS handshake from the server.

Previously, the exception thrown during ASN.1 parsing was not caught and therefore caused a TLS alert "internal_error". This PR handles the exception to respond with a RFC-compliant TLS alert "bad_certificate".

[peterdettman]

The basic idea is good, but the patch is a bit simplistic:

• we should deal also with processing of the client certificate
• shouldn't swallow existing TlsFatalAlert (which is an IOException subclass)
• probably not all errors from Certificate.parse should be bad_certificate; I guess decode_error should be used for some cases and there may be others.

[mj-vivavis]

Thanks for your feedback.

we should deal also with processing of the client certificate

Agreed, I refactored my patch to have a common method which is used both for handling of received client and server certificates

shouldn't swallow existing TlsFatalAlert (which is an IOException subclass)

Fixed.

probably not all errors from Certificate.parse should be bad_certificate; I guess decode_error should be used for some cases and there may be others.

JcaTlsCrypto.createCertificate() detects if the certificate is not of type X.509 and responds with "unsupported_certificate".

bc-java/tls/src/main/java/org/bouncycastle/tls/crypto/impl/jcajce/JcaTlsCrypto.java

Lines 162 to 171 in 0520ebc

	public TlsCertificate createCertificate(short type, byte[] encoding)
	throws IOException
	{
	if (type != CertificateType.X509)
	{
	throw new TlsFatalAlert(AlertDescription.unsupported_certificate);
	}
	return new JcaTlsCertificate(this, encoding);
	}

If none or more than one ASN.1 object has been provided, TlsUtils.readASN1Object() throws a "decode_error" (the method is originally called by JcaTlsCertificate.parseCertificate())

bc-java/tls/src/main/java/org/bouncycastle/tls/TlsUtils.java

Lines 1080 to 1093 in 0520ebc

	public static ASN1Primitive readASN1Object(byte[] encoding) throws IOException
	{
	ASN1InputStream asn1 = new ASN1InputStream(encoding);
	ASN1Primitive result = asn1.readObject();
	if (null == result)
	{
	throw new TlsFatalAlert(AlertDescription.decode_error);
	}
	if (null != asn1.readObject())
	{
	throw new TlsFatalAlert(AlertDescription.decode_error);
	}
	return result;
	}

For everything else, "bad_certificate" is IMHO the right response at this place. Rationale:

• We extracted the portion of the handshake message, which is supposed to contain the certificate. No range check failed until now, and no other inconsistency or invalid value in the handshake message or its overall structure has been detected, which would have justified a "decode_error"
• We failed to process the certificate, which is what "bad_certificate" is supposed to signal
  • Any general IOException due to out-of-bounds access of the ByteArrayStream etc. indicates that the certificates internal structure is broken/invalid (or as a special case, the sender only sent a portion of it)
  • Any ASN1Exception indicates, as far as I can tell, explicitly a broken certificate