@@ -269,24 +269,42 @@ WOLFSSL_STACK* wolfSSL_PKCS7_get0_signers(PKCS7* pkcs7, WOLFSSL_STACK* certs,
269269 WOLFSSL_X509 * x509 = NULL ;
270270 WOLFSSL_STACK * signers = NULL ;
271271 WOLFSSL_PKCS7 * p7 = (WOLFSSL_PKCS7 * )pkcs7 ;
272+ byte * signerCert ;
273+ word32 signerCertSz ;
272274
273275 if (p7 == NULL )
274276 return NULL ;
275277
276- /* Only PKCS#7 messages with a single cert that is the verifying certificate
277- * is supported.
278- */
279278 if (flags & PKCS7_NOINTERN ) {
280279 WOLFSSL_MSG ("PKCS7_NOINTERN flag not supported" );
281280 return NULL ;
282281 }
283282
283+ /* Prefer the certificate that actually verified the signature. Falling
284+ * back to singleCert (cert[0]) would let an attacker that bundles a
285+ * trusted cert ahead of their own attacker cert have the trusted cert
286+ * reported as the signer even though it did not produce the signature.
287+ *
288+ * Copy the chosen pointer into a local before passing its address to
289+ * wolfSSL_d2i_X509; d2i_X509 advances *in by the DER length, and if
290+ * we handed it the address of the struct field directly it would
291+ * permanently corrupt the field, producing a heap-OOB read on the
292+ * next use (pointer advanced, singleCertSz unchanged). */
293+ if (p7 -> pkcs7 .verifyCert != NULL && p7 -> pkcs7 .verifyCertSz > 0 ) {
294+ signerCert = p7 -> pkcs7 .verifyCert ;
295+ signerCertSz = p7 -> pkcs7 .verifyCertSz ;
296+ }
297+ else {
298+ signerCert = p7 -> pkcs7 .singleCert ;
299+ signerCertSz = p7 -> pkcs7 .singleCertSz ;
300+ }
301+
284302 signers = wolfSSL_sk_X509_new_null ();
285303 if (signers == NULL )
286304 return NULL ;
287305
288- if (wolfSSL_d2i_X509 (& x509 , (const byte * * )& p7 -> pkcs7 . singleCert ,
289- p7 -> pkcs7 . singleCertSz ) == NULL ) {
306+ if (wolfSSL_d2i_X509 (& x509 , (const byte * * )& signerCert ,
307+ signerCertSz ) == NULL ) {
290308 wolfSSL_sk_X509_pop_free (signers , NULL );
291309 return NULL ;
292310 }
0 commit comments