PBE and padded block cipher enhancements and fixes

RESOLVED FIXED

Status

JSS
Library
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: Fraser Tweedale, Assigned: Fraser Tweedale)

Tracking

4.4.2

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Assignee)

Description

4 months ago
Created attachment 8875117 [details] [diff] [review]
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.
(Assignee)

Comment 1

4 months ago
Created attachment 8875119 [details] [diff] [review]
jss-ftweedal-0007-Support-the-CKK_GENERIC_SECRET-symmetric-key-type.patch
(Assignee)

Comment 2

4 months ago
Created attachment 8875120 [details] [diff] [review]
jss-ftweedal-0008-PK11Cipher-improve-error-reporting.patch
(Assignee)

Comment 3

4 months ago
Created attachment 8875121 [details] [diff] [review]
jss-ftweedal-0009-Update-AES-CBC-PAD-cipher-definitions.patch
(Assignee)

Comment 4

4 months ago
Created attachment 8875122 [details] [diff] [review]
jss-ftweedal-0010-PK11Cipher-use-pad-mechanism-for-algorithms-that-use.patch

Comment 5

a month ago
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.
(Assignee)

Comment 6

a month ago
Created attachment 8905453 [details] [diff] [review]
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

a month ago
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.
(Assignee)

Comment 8

a month ago
Created attachment 8905763 [details] [diff] [review]
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.
Attachment #8905453 - Attachment is obsolete: true
(Assignee)

Comment 9

a month ago
Created attachment 8905764 [details] [diff] [review]
jss-ftweedal-0013-Improve-error-reporting.patch

Comment 10

a month ago
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

a month ago
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

a month ago
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

a month ago
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

a month ago
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

a month ago
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

a month ago
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
(Assignee)

Comment 17

a month ago
cfu checked-in the following changes provided by ftweedal:

changeset:   2204:87dca07f7529
tag:         tip
user:        Fraser Tweedale<ftweedale@redhat.com>
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<ftweedale@redhat.com>
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<ftweedale@redhat.com>
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<ftweedale@redhat.com>
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<ftweedale@redhat.com>
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<ftweedale@redhat.com>
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<ftweedale@redhat.com>
date:        Fri Sep 08 11:09:23 2017 -0700
summary:     Bug 1370778 PBE and padded block cipher enhancements and fixes -

Updated

a month ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED

Updated

a month ago
Assignee: glenbeasley → ftweedal
You need to log in before you can comment on or make changes to this bug.