CWE ID 327: AbstractNamedCurve.java:94

[Brent Putman]

Oh, never mind on the 27 vs 327.  I see that the actual subject line 
has 327, so just the ID in the message body is typod.


On 9/14/22 11:55 PM, Brent Putman wrote:
>
> Hi Jeremy,
>
>
> On 9/14/22 8:55 PM, Jeremy Karlson via users wrote:
>> Hi everyone. I’m new here, so I apologize if this has been discussed ad nauseam before. (I did try searching for answers, but didn’t fine clarity for this specific file.)
>
>
> Nope, this is the first time it's been brought up AFAIK.
>
>
>> Veracode (a code scanning tool my employer has recently started using) is reporting CWE ID 27 (Use of a Broken or Risky Cryptographic Algorithm) specifically in opensaml-security-api,
>
>
> I have no familiarity with Veracode. I found this table with CWE IDs:
>
> https://docs.veracode.com/r/c_review_cwe
>
> but 27 is a different thing, about path traversal.  The one with that 
> name/label is actually 327 - can you confirm that that is a typo etc, 
> and 327 is the one in question?
>
>
>> org.opensaml.security.crypto.ec.curves.AbstractNamedCurve.java, line 94. This method is:
>>
>>     @Nullable protected ECParameterSpec buildParameterSpec()
>>
>> And the line in question is:
>>
>>     jcaSpec = ECPublicKey.class.cast(
>>        KeySupport.generateKeyPair(JCAConstants.KEY_ALGO_EC, new ECGenParameterSpec(getName()), null)
>>           .getPublic()).getParams();
>>
>> I looked at the code but I am certainly out of my depth here, as I know very little about how this operates internally.
>
>
> That's sort of weird. The only cryptographic algorithm mentioned 
> there is (general) use of Elliptic Curve (EC) keys, and that 
> certainly isn't broken or risky. I mean, it's considered more secure 
> and future-proof than RSA.  I was expecting this to possibly point at 
> a weak signing or encryption algorithm, but all that code is doing is 
> generating an EC keypair so if that is truly the code in question, 
> this really doesn't make any sense.
>
> Unless... it's possibly actually flagging on a *particular* named 
> curve.  But in that case, we'd need to know which named curve. The 
> code above is just code in an abstract base class that resolves an 
> ECParameterSpec based on a curve name.  So we'd need to know the 
> actual curve name from the surrounding context.
>
>
>>   I am certain this has been brought up and considered before, so if someone wants to let me know this isn’t a problem, I can put a checkmark in this box and we can all get on with out days. :-)
>
>
> It actually never has, to my knowledge.  It's certainly possible that 
> we are by default including support for a named curve that is 
> considered weak at this point.
>
> But the code reference above doesn't tell us anything useful. We'd 
> need to know the value of the getName() there - the named curve ID - 
> which is going to be in the concrete class impl. (Every named curve 
> will have a concrete NamedCurve impl, which has AbstractNamedCurve as 
> a super class.)
>
> --Brent
>