Fix: Fix possible Null Pointer Exception

[arthurscchan]

Summary

In PublicKeyFactory Line 108, SubjectPublicKeyInfo.getInstance method could return null and make PublicKeyFactory Line 148 throws a NullPointerException.

From InputStream javadoc, we see that if the end of the stream is reached, it will return -1. Which means that if an empty byte array is provided, the tag will be -1 and it will return null in ASN1InputStream. In this case, it will trigger the null pointer exception above. Thus this fix adds an additional checking to ensure the PEM content is not an empty array.

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=57247

Release Note

Documentation

[vlsi]

I think the verification should be performed by the downstream library, and adding 0 check here is wrong

[vlsi]

The PR title says NullPointerException while the fix checks for empty array. It does not sound aligned

[loosebazooka]

@vlsi, I believe there is a sufficient description to describe the issue. The title is aligned with the problem.

[vlsi]

@loosebazooka , we should not add arbitrary checks. Is there anything in ASN1Sequence.getInstance that says “empty byte array must not be passed there”?
If empty array triggers issues, it should be documented and tested at bc-java level, and we should not add those checks at random in sigstore-java.

Then, I do not like to merge changes without corresponding tests.
The PR add no tests, so it is possible that someone would remove the “useless” check tomorrow in a different PR.

[loosebazooka]

I mean I agree here, this is a bouncy castle issue, but it still causes crashes in our code.
I believe oss-fuzz caches issues that cause crashes and you can repeat those against the proejct (is this right @arthurscchan ?) So if these were removed, oss-fuzz would catch it. (but yeah tests could be added to the relevant test classes to track these)

[vlsi]

If it is a bouncy castle issue, it should be fixed and tested at bouncy castle side, so everybody wins from the improvement.

So if these were removed, oss-fuzz would catch it.

oss-fuzz is non-deterministic, so it is not guaranteed to catch it. The cases identified by oss-fuzz should be baked into reproducible tests to prevent regressions.

[arthurscchan]

@loosebazooka Indeed, this could be reproducible.

[loosebazooka]

@arthurscchan can you elaborate or point to the docs on how reproducing works for ossfuzz, it might also make sense to just bake these test cases into our testsuite anyway.

[arthurscchan]

In general, the oss-fuzz tool provides function to reproduce testcase, see https://google.github.io/oss-fuzz/advanced-topics/reproducing/#reproducing-bugs
The testcase that cause the problem could find in the bug page.

[DavidKorczynski]

In general, the oss-fuzz tool provides function to reproduce testcase, see https://google.github.io/oss-fuzz/advanced-topics/reproducing/#reproducing-bugs The testcase that cause the problem could find in the bug page.

In addition to this, we also added some documentation here: https://github.com/sigstore/sigstore-java/blob/main/DEVELOPMENT.md#reproduce-oss-fuzz-crash

To reproduce this crash:

git clone https://github.com/google/oss-fuzz
cd oss-fuzz
python3 infra/helper.py build_fuzzers sigstore-java

# Download reproducer testcase to a local file
#   From this page: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=57247
#   download `Reproducer Testcase: https://oss-fuzz.com/download?testcase_id=5658600826863616` to the current folder
python3 infra/helper.py reproduce sigstore-java KeysFuzzer ./clusterfuzz-testcase-minimized-KeysFuzzer-5658600826863616

[loosebazooka]

I think it still makes sense to include a testcase in KeysTest.java to document this behavior. The relationship between ossfuzz and this project is not guaranteed to be transferable. Anyone forking this project or keeping a private copy should be allowed the benefit of a full testsuite.

[DavidKorczynski]

I think it still makes sense to include a testcase in KeysTest.java to document this behavior

@arthurscchan can you do this?

[loosebazooka]

I can do it too, if arthur is busy

[arthurscchan]

Yes, I will create a unit test case for this issue and push to this PR. I am assuming the unit test case should be add in here (https://github.com/sigstore/sigstore-java/blob/main/sigstore-java/src/test/java/dev/sigstore/encryption/KeysTest.java), right?

[vlsi]

Please structure tests in such a way that they fail before you apply the fix.
The current test passes even with the current sigstore-java, so it does not look like the test is helpful

[vlsi]

The bug has nothing to do with sigstore-java, and the bug belongs to bc-java. If we commit a workaround, there should be a reference to bc-java issue, so we could drop the workaround once bc-java issue is resolved

[loosebazooka]

Sure, we can link to bcgit/bc-java#1370 for now.

[vlsi]

thanks