Fix OSSL_STORE to consider cached info in the EOF check. #28016

levitte · 2025-07-10T16:03:18Z

OSSL_STORE_load() called OSSL_STORE_eof() before checking if there is
cached OSSL_STORE_INFO to consider. To fix this issue, the cached info
check is moved to OSSL_STORE_eof(), as that seems to make most common
sense.

This solves an issue with PKCS#12 files, where the cached info was never
considered because the underlying file IO layer signaled that EOF is
reached.

Fixes #28010

OSSL_STORE_load() called OSSL_STORE_eof() before checking if there is cached OSSL_STORE_INFO to consider. To fix this issue, the cached info check is moved to OSSL_STORE_eof(), as that seems to make most common sense. This solves an issue with PKCS#12 files, where the cached info was never considered because the underlying file IO layer signaled that EOF is reached. Fixes openssl#28010

levitte · 2025-07-10T16:04:29Z

Please note that #28010 shows a regression. The same PKCS#12 was loaded flawlessly in OpenSSL 1.1.1

DDvO · 2025-07-10T16:15:31Z

Would be good to add a test case demonstrating that the issue is fixed and flagging any future regression.

levitte · 2025-07-10T16:34:04Z

Would be good to add a test case demonstrating that the issue is fixed and flagging any future regression.

I agree, but will need a triggering test case. I'm currently checking an unpatched 3.5 to see if the regression is visible there somehow

levitte · 2025-07-10T16:48:07Z

Would be good to add a test case demonstrating that the issue is fixed and flagging any future regression.

I agree, but will need a triggering test case. I'm currently checking an unpatched 3.5 to see if the regression is visible there somehow

Nope, our tests are fine, so it's quite clear that the unusual form of the PKCS#12 file you tested with was part of the issue, and that we have never seen that case before.

t8m

Very interesting. I wonder if there are any other consequences of this bug not related to PKCS12 at all. For example in terms of a file containing multiple PEM encoded objects of a different type.

levitte · 2025-07-10T17:12:16Z

... For example in terms of a file containing multiple PEM encoded objects of a different type.

Nope. They are genuinely loaded from the file one at a time. PKCS#12 makes for a unique situation, where all objects are wrapped into one container, and because of how the rest of the flow works, the only way to handle those is to unpack them and cache them.

DDvO · 2025-07-12T06:13:33Z

The fix looks good to me.

Would be good to add a test case demonstrating that the issue is fixed and flagging any future regression.

I agree, but will need a triggering test case. I'm currently checking an unpatched 3.5 to see if the regression is visible there somehow

Nope, our tests are fine, so it's quite clear that the unusual form of the PKCS#12 file you tested with was part of the issue, and that we have never seen that case before.

So PKCS#12 with non-DER indefinite-length encodings have never been explicitly addressed before,
but happened to be supported before OpenSSL 3.0.

Why not simply add a CLI-based test like this?

openssl x509 -passin pass:12345 -in test-BER.p12

Unfortunately cannot use the more general storeutl app here because
it does not have a -pass(in) option, which IMO should be added
for general usability, in particular for non-interactive scenarios.

levitte · 2025-07-15T16:44:03Z

@DDvO, may I consider the PKCS#12 file from #28010 as a contribution for testing with?

DDvO · 2025-07-16T09:29:37Z

Actually this is what I had in mind. I'd just suggest renaming it (to e.g., test-BER.p12 as written above)
because as you found the point here is the encoding, not the encryption alg.

DDvO

Thanks for the fix, including the test case.

The test file (test-BER.p12) was given to us by David von Oheimb Co-Authored-By: David von Oheimb <david.von.oheimb@siemens.com>

levitte · 2025-07-17T07:04:56Z

Unfortunately, test-BER.p12 causes errors when DES or EC are disabled. I've modified the test accordingly.

DDvO

reconfirmed

openssl-machine · 2025-07-25T18:00:40Z

This pull request is ready to merge

OSSL_STORE_load() called OSSL_STORE_eof() before checking if there is cached OSSL_STORE_INFO to consider. To fix this issue, the cached info check is moved to OSSL_STORE_eof(), as that seems to make most common sense. This solves an issue with PKCS#12 files, where the cached info was never considered because the underlying file IO layer signaled that EOF is reached. Fixes #28010 Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> (Merged from #28016)

The test file (test-BER.p12) was given to us by David von Oheimb Co-Authored-By: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> (Merged from #28016)

OSSL_STORE_load() called OSSL_STORE_eof() before checking if there is cached OSSL_STORE_INFO to consider. To fix this issue, the cached info check is moved to OSSL_STORE_eof(), as that seems to make most common sense. This solves an issue with PKCS#12 files, where the cached info was never considered because the underlying file IO layer signaled that EOF is reached. Fixes #28010 Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> (Merged from #28016) (cherry picked from commit 1f3af48)