RFR: 8296442: EncryptedPrivateKeyInfo can be created with an uninitialized AlgorithmParameters

Xue-Lei Andrew Fan xuelei at openjdk.org
Thu Nov 10 06:18:35 UTC 2022


On Wed, 9 Nov 2022 19:59:08 GMT, Weijun Wang <weijun at openjdk.org> wrote:

> An `EncryptedPrivateKeyInfo` object can be created with an uninitialized `AlgorithmParameters`, but before you call `getEncoded` on it you need to remember to initialize the params. This is unfortunate but since this is a public API, I hesitate to make a change.
> 
> Instead, this code change fixes the much more widely used internal class `AlgorithmId` so that it cannot be created with an uninitialized `AlgorithmParameters`. `EncryptedPrivateKeyInfo` now works with both initialized and uninitialized params, and it's immutable.
> 
> No intention to make `AlgorithmId` immutable this time. It has a child class named `AlgIdDSA` which makes things complicated.

Changes requested by xuelei (Reviewer).

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 192:

> 190:             // create an AlgorithmId from it. This should usually be true
> 191:             // since we already require its getEncoded() returning the
> 192:             // encoded bytes of it.

It looks like the comment implies there is no need of  the catch block.  Did you want to add something like "But it may be not true if ..."?  Or, is the comment belong to AlgorithmId?

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 199:

> 197:         if (tmp != null) {
> 198:             this.algid = tmp;
> 199:             this.params = null;

There is a getAlgParameters() method in the class.  Does it make sense to cache the params?

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 238:

> 236:      */
> 237:     public AlgorithmParameters getAlgParameters() {
> 238:         return algid == null ? params : algid.getParameters();

Does it make sense to keep the params from constructor and return the field here?

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 412:

> 410:                 try {
> 411:                     // Let's hope params has been initialized by now.
> 412:                     AlgorithmId.get(params).encode(tmp);

It looks like that you want to take care of the cases that the AlgorithmId did not initialize yet.  See comments in  AlgorithmId.

src/java.base/share/classes/javax/crypto/EncryptedPrivateKeyInfo.java line 437:

> 435: 
> 436:     @SuppressWarnings("fallthrough")
> 437:     private static PKCS8EncodedKeySpec pKCS8EncodingToSpec(byte[] encodedKey)

Does "pkcs8..." (will lowercase) look like a better name for the method?

src/java.base/share/classes/sun/security/x509/AlgorithmId.java line 113:

> 111:             } catch (IOException ioe) {
> 112:                 throw new IllegalStateException(
> 113:                         "AlgorithmParameters not initialized", ioe);

Did you have a chance to look at the caller and make sure this behavioral update is safe?

src/java.base/share/classes/sun/security/x509/AlgorithmId.java line 177:

> 175:             // If still not initialized. Let the IOE be thrown.
> 176:         }
> 177: 

This could be a risk change if the caller was not coded like what you do in the EncryptedPrivateKeyInfo.java update.   Did you have a chance to check all caller codes and make sure it is a safe update.

-------------

PR: https://git.openjdk.org/jdk/pull/11067



More information about the security-dev mailing list