1370778 - PBE and padded block cipher enhancements and fixes

Status: RESOLVED FIXED

(JSS Graveyard :: Library, defect)

Description

[Fraser Tweedale]

Attachment: jss-ftweedal-0006-PBEKeyGenParams-allow-specifying-encryption-algorith.patch

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170420084331

Steps to reproduce:

These patches address some shortcomings in PBE key derivation
and handling of EncryptionAlgorithms that use padding.

Specifically, there are separate patches to:

- Allow specifying an target encryption algorithm in PBEKeyGenParams;
 if the PBE algorithm does not imply a particular cipher, this is needed
 to determine the size of the key to generate

- Support the CKK_GENERIC_SECRET key type.  The PBKDF2 machinery
 generates a symmetric key with this key type, resulting in a mismatch
 and error when JSS tries to use the key to initialise a symmetric cipher.
 We need to allow this key type to be used.

- Update AES-CBC-PAD definitions to include the relevant OID and SEC_OID_TAG,
 so that they can be used as a target algorithm in PBE key derivation.

- Change symmetric cipher initialisation to use the padding variant of the
 PKCS #11 mechanism if the EncryptionAlgorithm indicates that padding is
 to be used.

Comment 1

[Fraser Tweedale]

Attachment: jss-ftweedal-0007-Support-the-CKK_GENERIC_SECRET-symmetric-key-type.patch

Comment 2

[Fraser Tweedale]

Attachment: jss-ftweedal-0008-PK11Cipher-improve-error-reporting.patch

Comment 3

[Fraser Tweedale]

Attachment: jss-ftweedal-0009-Update-AES-CBC-PAD-cipher-definitions.patch

Comment 4

[Fraser Tweedale]

Attachment: jss-ftweedal-0010-PK11Cipher-use-pad-mechanism-for-algorithms-that-use.patch

Comment 5

[Christina Fu]

Two things:

1. I see from my previous comment that it should be tested in FIPS mode.  Did you test it to work?
2. I have a suggestion in the KRA patch review to move some code into JSS; Please see the pagure review comments.

Comment 6

[Fraser Tweedale]

Attachment: jss-ftweedal-0012-Add-method-EncryptedPrivateKeyInfo.createPBES2.patch

Christina, yes it has been tested (and works) in FIPS mode.

I extract the implementation from Dogtag into EncryptedPrivateKeyInfo.createPBES2() as you have recommended.  See the
additional patch 0012 that I attached.

Comment 7

[Christina Fu]

Comment on attachment 8905453 [details] [diff] [review]
jss-ftweedal-0012-Add-method-EncryptedPrivateKeyInfo.createPBES2.patch

Review of attachment 8905453 [details] [diff] [review]:
-----------------------------------------------------------------

Overall very good.  Just a couple things relating to checking inputs:
1. in constructor of createPBES2, if input encAlg is null, you might want to set it to something such as EncryptionAlgorithm.AES_128_CBC_PAD, because otherwise it is defaulted to EncryptionAlgorithm.DES3_CBC in PBEKeyGenParams.java per your other patch.  encAlg is also further derefrenced in later code thus would cause NPE
2. also need to check null for pwd and privateKeyInfo upfront as well.  Especially for privateKeyInfo, you don't want to wait till the key has been generated to realize that the intended blob to be encrypted is null.

I think in JSS, in places we use IllegalArgumentException for illegal inputs.  Although there might be newer convention now that I'm not following but am open to them.

Comment 8

[Fraser Tweedale]

Attachment: jss-ftweedal-0012-2-Add-method-EncryptedPrivateKeyInfo.createPBES2.patch

Ah yes.  Java has that null thing -_-.  Updated patch 0012-2 attached.  I'll also attach 0013 which has some improvements to error reporting.  Thanks Christina for reviewing.

Comment 9

[Fraser Tweedale]

Attachment: jss-ftweedal-0013-Improve-error-reporting.patch

Comment 10

[Christina Fu]

Comment on attachment 8905763 [details] [diff] [review]
jss-ftweedal-0012-2-Add-method-EncryptedPrivateKeyInfo.createPBES2.patch

Review of attachment 8905763 [details] [diff] [review]:
-----------------------------------------------------------------

looks good.

Comment 11

[Christina Fu]

Comment on attachment 8905764 [details] [diff] [review]
jss-ftweedal-0013-Improve-error-reporting.patch

Review of attachment 8905764 [details] [diff] [review]:
-----------------------------------------------------------------

looks good.

Comment 12

[Christina Fu]

Comment on attachment 8875117 [details] [diff] [review]
jss-ftweedal-0006-PBEKeyGenParams-allow-specifying-encryption-algorith.patch

Review of attachment 8875117 [details] [diff] [review]:
-----------------------------------------------------------------

looks good.

Comment 13

[Christina Fu]

Comment on attachment 8875119 [details] [diff] [review]
jss-ftweedal-0007-Support-the-CKK_GENERIC_SECRET-symmetric-key-type.patch

Review of attachment 8875119 [details] [diff] [review]:
-----------------------------------------------------------------

looks good

Comment 14

[Christina Fu]

Comment on attachment 8875120 [details] [diff] [review]
jss-ftweedal-0008-PK11Cipher-improve-error-reporting.patch

Review of attachment 8875120 [details] [diff] [review]:
-----------------------------------------------------------------

looks good.

Comment 15

[Christina Fu]

Comment on attachment 8875121 [details] [diff] [review]
jss-ftweedal-0009-Update-AES-CBC-PAD-cipher-definitions.patch

Review of attachment 8875121 [details] [diff] [review]:
-----------------------------------------------------------------

ack

Comment 16

[Christina Fu]

Comment on attachment 8875122 [details] [diff] [review]
jss-ftweedal-0010-PK11Cipher-use-pad-mechanism-for-algorithms-that-use.patch

Review of attachment 8875122 [details] [diff] [review]:
-----------------------------------------------------------------

ack

Comment 17

[Fraser Tweedale]

cfu checked-in the following changes provided by ftweedal:

changeset:   2204:87dca07f7529
tag:         tip
user:        Fraser Tweedale<[e-mail]>
date:        Fri Sep 08 11:56:04 2017 -0700
summary:     Bug 1370778 PBE and padded block cipher enhancements and fixes -

changeset:   2203:b3b653faef84
user:        Fraser Tweedale<[e-mail]>
date:        Fri Sep 08 11:53:36 2017 -0700
summary:     bug 1370778 PBE and padded block cipher enhancements and fixes -

changeset:   2202:0b8a6e84b6c7
user:        Fraser Tweedale<[e-mail]>
date:        Fri Sep 08 11:50:21 2017 -0700
summary:     Bug 1370778 PBE and padded block cipher enhancements and fixes -

changeset:   2201:d39e9b373798
user:        Fraser Tweedale<[e-mail]>
date:        Fri Sep 08 11:32:32 2017 -0700
summary:     Bug 1370778 PBE and padded block cipher enhancements and fixes -

changeset:   2200:890216599f21
user:        Fraser Tweedale<[e-mail]>
date:        Fri Sep 08 11:21:22 2017 -0700
summary:     Bug 1370778 PBE and padded block cipher enhancements and fixes -

changeset:   2199:bada1409d2bb
user:        Fraser Tweedale<[e-mail]>
date:        Fri Sep 08 11:15:29 2017 -0700
summary:     Bug 1370778 PBE and padded block cipher enhancements and fixes -

changeset:   2198:3629b598a9ce
user:        Fraser Tweedale<[e-mail]>
date:        Fri Sep 08 11:09:23 2017 -0700
summary:     Bug 1370778 PBE and padded block cipher enhancements and fixes -