A mirror of https://github.com/libressl/openbsd.git https://git.lua4.win/openbsd/atom?h=libressl-v3.3.0 2020-11-18T17:54:46+00:00 2020-11-18T17:54:46+00:00 tb 2020-11-18T17:54:46+00:00 urn:sha1:d7ea65d1de4c5d0528a68eb9b33a6ef17ca79f14 x509_verify_chain_new() allocates a few members of a certificate chain: an empty stack of certificates, a list of errors encountered while validating the chain, and a list of name constraints. The function to copy a chain would allocate a new chain using x509_verify_chain_new() and then clobber its members by copies of the old chain. Fix this by replacing x509_verify_chain_new() with calloc(). Found by review while investigating the report by Hanno Zysik who found the same leak using valgrind. This is a cleaner version of my initial fix from jsing. ok jsing 2020-11-18T17:40:42+00:00 tb 2020-11-18T17:40:42+00:00 urn:sha1:fa7f97be6a425fa454c92d146ea0a205a46da2a0 The legacy validator would only call x509_vfy_check_policy() once at the very end after cobbling together a chain. Therefore it didn't matter that X509_policy_check() always allocates a new tree on top of the one that might have been passed in. This is in stark contrast to other, similar APIs in this code base. The new validator calls this function several times over while building its chains. This adds up to a sizable leak in the new validator. Reported with a reproducer by Hanno Zysik on github, who also bisected this to the commit enabling the new validator. Narrowed down to x509_vfy_check_policy() by jsing. We simultaenously came up with a functionally identical fix. ok jsing 2020-11-18T17:13:55+00:00 tb 2020-11-18T17:13:55+00:00 urn:sha1:644cb0c3bf194db22125d9f1f2bb4fbdad279d65 2020-11-18T17:08:59+00:00 tb 2020-11-18T17:08:59+00:00 urn:sha1:3bc85c50efd5da6e970a3a4b0bf1d312386c6327 a few lines after. stylistic nit from jsing 2020-11-18T17:00:59+00:00 tb 2020-11-18T17:00:59+00:00 urn:sha1:05555ab1add6e7a31798831da9ff788048d1b3cf 2020-11-18T11:10:08+00:00 bcook 2020-11-18T11:10:08+00:00 urn:sha1:047724b70558e5801fecc261c89babf45511d99e 2020-11-16T17:43:37+00:00 jsing 2020-11-16T17:43:37+00:00 urn:sha1:405976d31ae6b14d18567c95b7b569a4f840959c ok beck@ tb@ 2020-11-16T17:42:35+00:00 jsing 2020-11-16T17:42:35+00:00 urn:sha1:6d683ad99d2d002b7d27f3685fa75523eca88ce3 This was inadvertently removed in r1.19. Spotted by tb@ ok beck@ tb@ 2020-11-15T17:54:49+00:00 beck 2020-11-15T17:54:49+00:00 urn:sha1:80b086a4acc51fd3fe6aa757d59578dbe416f61d in order to be compatible with the openssl error craziness in the legacy verifier case. This will fix a regress problem noticed by znc ok tb@ 2020-11-11T18:49:34+00:00 jsing 2020-11-11T18:49:34+00:00 urn:sha1:d677ab3272ee7c58415094ef4162e2760b96be3c With the old verifier, the verify callback can always return 1 instructing the verifier to simply continue regardless of a certificate verification failure (e.g. the certificate is expired or revoked). This would result in a chain being built, however the first error encountered would be persisted, which allows the caller to build the chain, have the verification process succeed, yet upon inspecting the error code note that the chain is not valid for some reason. Mimic this behaviour by keeping track of certificate errors while building chains - when we finish verification, find the certificate error closest to the leaf certificate and expose that via the X509_STORE_CTX. There are various corner cases that we also have to handle, like the fact that we keep an certificate error until we find the issuer, at which point we have to clear it. Issue reported by Ilya Shipitcin due to failing haproxy regression tests. With much discussion and input from beck@ and tb@! ok beck@ tb@