CWE ID 327: AbstractNamedCurve.java:94

[Brent Putman]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://shibboleth.net/pipermail/users/attachments/20220914/bf66243d/attachment.htm>