Bouncy Castle FIPS Issue

Thu Jul 15 00:55:21 UTC 2021

Hi,

Either I don't think I understand, or maybe you misread our code.  
AFAIK the KeyAgreement service interface requires the 2 Keys over which 
the key agreement is being performed. For example:

keyAgreement = KeyAgreement.getInstance(agreementAlgo);
keyAgreement.init(privateKey);
keyAgreement.doPhase(publicKey, true);

Our KeyDerivation impls like ConcatKDF do not have access to any of the 
key agreement key material.  They are about key derivation from an 
input byte[] only (i.e. the output byte[] of the KeyAgreement op).  The 
do not and can not have access to any key agreement key material, 
because it's out-of-scope for what they do.  So I don't see how we can 
re-implement our ConcatKDF "SecretKey derive(...)" method using the 
KeyAgreement service interface as you suggest.

Please clarify if I am missing something here.

Thanks,
Brent

On 7/14/21 8:21 PM, David Hook wrote:
>
> Hi Brent,
>
> It will work. The naming convention is also to allow mixing and 
> matching, you just need to add:
>
> org.bouncycastle.jcajce.spec.UserKeyingMaterialSpec
>
> as a parameter to the KeyAgreement. It's unfortunate that the JCE 
> doesn't define this parameter, so I am not surprised you are not 
> aware of it, but it's impossible to use key agreement properly 
> without it, and it exists in both BC and BCFIPS specifically for 
> situations like this.
>
> You use the UserKeyingMaterialSpec to pass in otherInfo, which I can 
> see is being constructed in accordance with SP 800-56A which is the 
> FIPS standard. There is some BC support for otherInfo but we are 
> using the ASN.1 construction, SP 800-56A allows for several others, 
> and I'm not sure what the XML standard settled on. You are probably 
> better off sticking to what you are doing now.
>
> For our part I think we'd just need to add some additional KDF 
> support for the non-NIST digests. In the case of the SAML code it 
> would just be a matter of replacing SecretKey derive() I think and 
> allowing for the UserKeyingMaterialSpec construction.
>
> Concerning  your comments about BC and BCFIPS. Yes the low level 
> libraries are different, they have to be, but we've been able to keep 
> the JCE layer in sync since 1.58. The upside of this is it means if 
> people swap in BCFIPS rather than BC they can be confident the code 
> is then FIPS compliant. If you interested in finding out why the 
> difference exists, there is actually a document
>
> https://www.bouncycastle.org/fips/BCFipsDescription.pdf
>
> which provides a summary. I would recommend that over reading the 
> FIPS IG and the 30 or so associated documents that go with it unless 
> you have a specific interest. The same applies for Common Criteria.
>
> I don't think there will be any end-user problems as the end result 
> will be a massive broadening of what the OpenSAML project can be used 
> for.
>
> Any further questions or issues, please let me know.
>
> Thanks,
>
> David
>
> On 15/7/21 8:59 am, Brent Putman wrote:
>>
>> Hi David,
>>
>> I took a look and unfortunately what you propose won't work, for at 
>> least 2 reasons.
>>
>> First (the main and show-stopping technical issue), those 
>> provider-based algorithms you mention are apparently for the 
>> KeyAgreement service of the Java security framework. It seems those 
>> combine the key agreement op with the KDF op all in one go?
>>
>> That won't work for our design because we're modeling the 
>> requirements of XML Encryption 1.1 where the KA and KDF ops are 
>> conceptually completely separate. They are defined separately, have 
>> distinct XML representations, etc, so one can mix-and-match KA algo 
>> and KDF algo.
>>
>> So in OpenSAML we have distinct KA and KDF interfaces and then 
>> various impls for each. For example we have interface 
>> org.opensaml.xmlsec.derivation.KeyDerivation.  The ConcatKDF impl of 
>> that in question that threw a NoClassDefFoundError with FIPS was our 
>> org.opensaml.xmlsec.derivation.impl.ConcatKDF, which imports these 
>> KDF-related classes (along with some needed BC digest classes):
>>
>> import org.bouncycastle.crypto.agreement.kdf.ConcatenationKDFGenerator;
>> import org.bouncycastle.crypto.params.KDFParameters;
>>
>> It was specifically throwing on not finding 
>> org.bouncycastle.crypto.DerivationParameters, which is the interface 
>> implemented by the above BC KDFParameters.  For reference that 
>> source is here:
>>
>> http://git.shibboleth.net/view/?p=java-opensaml.git;a=blob;f=opensaml-xmlsec-impl/src/main/java/org/opensaml/xmlsec/derivation/impl/ConcatKDF.java;hb=refs/heads/main
>>
>> By contrast, for PBKDF2 we are using the SecretKeyFactory service 
>> with the JDK-provided algorithm support for "PBKDF2With*" (completed 
>> with the appropriate PRF algo ID). So if the BC provider had a 
>> SecretKeyFactory impl for ConcatKDF by itself, that would be closer 
>> to our needs. But....
>>
>> Second, it's unlikely we could introduce a deployment requirement 
>> for people to add the BC provider declaratively in java.security and 
>> also questionable for us to add the BC provider programmatically by 
>> default.  As a library OpenSAML probably shouldn't muck with 
>> people's environments like that.
>>
>> And in XML Encryption 1.1, ConcatKDF is the mandatory-to-implement 
>> KDF (PBKDF2 is optional).  For that reason it is our default KDF for 
>> ECDH, and it needs to work out-of-the-box.  So hopefully you can see 
>> that adding a requirement on configuring a third-party provider 
>> would be an issue for us.  Our team can discuss further the idea of 
>> automagically/programmatically adding in BC.  I personally am not a 
>> fan of the idea.  But unless/until BC has a provider-based impl of 
>> the ConcatKDF by itself, it's essentially a moot question.
>>
>> I guess what I would ask about the FIPS version is:  If the provider 
>> there fundamentally supports ConcatKDF via the KeyAgreement 
>> algorithms you mention, then why aren't the KDF interfaces/impls 
>> there as well?  Probably they are there but just with different 
>> package and/or class names, etc?  If they are, then you know, that 
>> means the FIPS version really isn't a drop-in replacement for the 
>> regular library, so it's going to be challenging for us as a 
>> downstream consumer to overcome that.
>>
>> Thanks,
>> Brent
>>
>>
>>
>> On 7/13/21 10:30 PM, David Hook wrote:
>>> Hi Brent,
>>>
>>> I'd like to suggest that it gets changed to use the BC provider 
>>> rather than the BC low-level API.
>>>
>>> For some reason I can't connect to any of the nabble references, so 
>>> I can't easily find the source file concerned, but from what I 
>>> understand V4.1 added support for the ConcatKDF key derivation 
>>> function with ECDH. When I checked the code this looked like 
>>> something the BCFIPS provider supports already (and the BC 
>>> provider) for the JCE. The agreement algorithms ending in CKDF use 
>>> the concat KDF function.
>>>
>>> The following ones are currently available in both providers:
>>>
>>> ECCDHWITHSHA1CKDF
>>> ECCDHWITHSHA256CKDF
>>> ECCDHWITHSHA384CKDF
>>> ECCDHWITHSHA512CKDF
>>>
>>> I have subscribed to the dev list if you would prefer to continue 
>>> the discussion there. If you would include the link to the source 
>>> file either way we should be able to work something out that will 
>>> work for both providers (as I have commit access to both, you'd 
>>> hope so!).
>>>
>>> Let me know,
>>>
>>> Thanks,
>>>
>>> David
>>>
>>> On 14/7/21 7:17 am, Brent Putman wrote:
>>>>
>>>> Hello,
>>>>
>>>> Sorry David (Hook) that I didn't get back to you when I returned 
>>>> from vacation a couple of weeks ago, I unfortunately overlooked 
>>>> that email todo.
>>>>
>>>> I'm interested to hear what you had in mind.  If possible it would 
>>>> be great to have this discussion over on the Shibboleth/OpenSAML 
>>>> developers list, so that other members of the team and community 
>>>> can be aware and participate:
>>>>
>>>> https://shibboleth.net/mailman/listinfo/dev
>>>>
>>>> Thanks,
>>>> Brent
>>>>
>>>>
>>>> On 7/9/21 11:54 AM, David Castro wrote:
>>>>> Brent,
>>>>>
>>>>> Just wanted to follow up Re: the following thread on the FIPS 
>>>>> issue we were having.
>>>>>
>>>>> https://shibboleth.1660669.n2.nabble.com/Bouncy-Castle-FIPS-Issue-td7649455.html#a7649653 
>>>>> <https://shibboleth.1660669.n2.nabble.com/Bouncy-Castle-FIPS-Issue-td7649455.html#a7649653>
>>>>>
>>>>> We spoke with David Hook from Bouncy Castle (included in this 
>>>>> thread) and he had some ideas on resolving the issue. Scott 
>>>>> Cantor recommended you as the point of contact, but let us know 
>>>>> if there is someone else we should reach out to.
>>>>>
>>>>> We are also available to assist as needed.
>>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>> -- 
>>>>> CONFIDENTIALITY NOTICE: This transmission, and any attachments, 
>>>>> may contain CONFIDENTIAL, PRIVILEGED or PROPRIETARY information 
>>>>> of HRworx, LLC (dba Intelliworx) that is protected from 
>>>>> disclosure under applicable laws. If you are not the intended 
>>>>> recipient, any disclosure, copying, distribution, or use of any 
>>>>> of the information contained in or attached to this transmission 
>>>>> is STRICTLY PROHIBITED. If you have received this communication 
>>>>> in error, please notify the sender, by reply e-mail, and delete 
>>>>> the original message with any attachments. Thank you for your 
>>>>> cooperation.
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://shibboleth.net/pipermail/dev/attachments/20210714/67b7d4de/attachment.htm>