JDK 9 Review Request for 8038349: Signing XML with DSA throws Exception when key is larger than 1024 bits

[Sean Mullan]

On 05/01/2014 10:12 AM, Xuelei Fan wrote:
>> security/utils/JavaUtils.java
>> =============================
> The updated code is a straightforward move from other classes in order
> to remove the duplicated code.  Looks fine but we might want to make a
> few little enhancements within this fix.
>
> 162  public static byte[] convertASN1toXMLDSIG(byte asn1Bytes[], ..
> 201  public static byte[] convertXMLDSIGtoASN1(byte xmldsigBytes[], ..
>
> Minor comment about code conversions.  I may prefer to use consistent
> style to declare the array parameters.
>      byte asn1Bytes[] -> byte[] asn1Bytes
>      byte xmldsigBytes[] -> byte[] xmldsigBytes

Yes, I agree and will change that.

> convertASN1toXMLDSIG(byte asn1Bytes[], int size):
> -------------------------------------------------
> I may check the asn1Bytes.length against/and the "size" parameter, and
> check first 3 bytes before getting the 4rd byte.  Similarly, I may check
> the first byte and the rest length of the array before try to parse the
> s (the 2nd fragment of the array) value of the signature.

Ok, I moved those checks to the start of the method.

> convertXMLDSIGtoASN1(byte xmldsigBytes[], int size):
> ----------------------------------------------------
> I did not get the idea behind lines 212-215 and lines 220-223.  Why try
> to check and ignore the negative value?

I do not know but I am reluctant to remove it in case I break something. 
The code is not commented very well.

> 225         byte asn1Bytes[] = new byte[6 + j + l];
> By the line, the length of the target byte array is known. I might
> prefer to allocate the exactly size for byte array here.  Otherwise, the
> 2nd System.arraycopy() would need re-allocate the array for more space.

I'm not quite following you here. System.arrayCopy doesn't resize the 
array, it will throw an exception if the dest array is not big enough. 
Can you clarify what you mean or show me the corrected code?

Thanks,
Sean

>
> Xuelei
>
>> On 4/30/2014 4:48 AM, Sean Mullan wrote:
>>> Please review the following change which adds support for 2048-bit DSA
>>> keys and the DSA-SHA256 algorithm to the XML Signature implementation:
>>>
>>> webrev: http://cr.openjdk.java.net/~mullan/webrevs/8038349/webrev.00/
>>>
>>> JDK 8 already includes the underlying support for both of these in the
>>> Sun provider. For 2048 bit keys, the ASN.1 parsing code in the XML
>>> Signature layer needed to be adapted to handle 2048 bit keys, and for
>>> SHA-256 it was just a matter of registering the algorithm URI and
>>> instantiating a Signature object with the SHA256WithDSA algorithm.
>>>
>>> Thanks,
>>> Sean
>>
>