code review request: 7012160: read SF file in signed jar in streaming mode

[Weijun Wang]

On 02/02/2011 04:42 AM, Sean Mullan wrote:
> Hi Max,
>
> This is looking good. I'm about halfway through. Here are some code
> comments to start with. I think we'll need a couple more rounds of code
> reviews before we finish so I wanted to get these to you now.
>
> * JarFile
>
> [329-330]: How about adding a SignatureFileVerifier.isBlock method
> instead? That would be cleaner.

Good.

>
> [338]: I believe you can replace the argument to getBytes with the local
> variable e :
>
> s/getBytes(getJarEntry(names[i])),/getBytes(e),

Correct.

>
> * PKCS7
>
> Can we avoid duplicating code across SignerInfo and PKCS7Verifier? Can
> SignerInfo.verify invoke methods of PKCS7Verifier?

I thought SignerInfo.verify() could be removed at all, and only keep it 
there for your code review. Yes. It can be rewritten to --

     SignerInfo verify(PKCS7 block, byte[] data)
             throws NoSuchAlgorithmException, SignatureException {

         PKCS7.PKCS7Verifier p7v = PKCS7.PKCS7Verifier.from(block, this);
         if (p7v == null) return null;
         if (data == null) {
             try {
                 data = block.getContentInfo().getContentBytes();
             } catch (IOException e) {
                 throw new SignatureException(
                         "IO error verifying signature:\n" +
                         e.getMessage());
             }
         }
         p7v.update(data, 0, data.length);
         return p7v.verify();
     }

>
> [538-540]: fields should be marked final

OK.

>
> [542]: Why is a static method necessary here? Since the method always
> creates a PKCS7Verifier object, it doesn't seem like it is that useful
> or necessary.

The original verify() method in SignerInfo sometimes return null and 
sometimes throws an Exception. I don't exactly understand why there is 
such a difference and use this static method to precisely mimic the 
behavior.

>
> [661-665]: replace this code with MessageDigest.isEqual.

Yes.

All changes are trivial except for the new SignerInfo.verify() method. I 
guess a webrev is not needed.

Thanks
Max

>
> --Sean
>
>
> On 1/14/11 3:31 AM, Weijun Wang wrote:
>> Hi Sean
>>
>> http://cr.openjdk.java.net/~weijun/7012160/webrev.00/
>>
>> I've made changes to the following classes to enable streaming mode SF
>> file
>> reading:
>>
>> - java/util/jar/JarVerifier.java:
>>
>> 1. New verifyBlock method.
>>
>> 2. Change the constructor from JarVerifier(byte[]) to JarVerifier(byte[],
>> Manifest). In SignatureFileVerifier.processImpl(), if we already
>> confirm the
>> *-Digest-Manifest header in the SF file matches the whole MANIFEST.MF,
>> there'se
>> no need to parse the rest of the SF file, since we can be sure that
>> entries in
>> the SF file are identical to those in MANIFEST.MF. Of course, the
>> content of the
>> SF file still needs to be fed into PKCS7Verifier to verify the signature.
>>
>> - java/util/jar/JarFile.java:
>>
>> Read DSA file in byte[] and SF file in InputStream, and call
>> JarVerifier.verifyBlock() to verify.
>>
>> - java/util/jar/Manifest.java:
>>
>> Adding update(byte[]) to read manifest in streaming mode. This is a
>> new public API.
>>
>> - sun/security/pkcs/PKCS7.java:
>>
>> New PKCS7Verifier class to verify SignedData in streaming mode. I
>> basically
>> divide the SignerInfo.verify(PKCS7 block, byte[] data) method into 3
>> parts and
>> make them the 3 methods of this class.
>>
>> - sun/security/util/SignatureFileVerifier.java:
>>
>> Rewrite the processImpl(*) method to make use of new methods in PKCS7
>> and Manifest.
>>
>> No new regression tests, use existing ones.
>>
>> I've tried NetBeans profiler to look at the memory. The program simply
>> calls
>> JarSigner.main(new String[]{"-verify", "x.jar"}) and the signed jar
>> x.jar has
>> 10000 files inside.
>>
>> Before After
>> byte[] 3.6MB 2.8MB
>> char[] 2.0MB 1.3MB
>> String 1.1MB 650KB
>>
>> So it does have some difference.
>>
>> Thanks
>> Max
>>
>>
>> -------- Original Message --------
>> *Change Request ID*: 7012160
>> *Synopsis*: read SF file in signed jar in streaming mode
>>
>>
>> === *Description*
>> ============================================================
>> When a signed jar is verified, its SF file is read into a byte array and
>> verified against the signature. When there are many files in the jar,
>> the SF
>> file can be very big. It will be better if the file can be read in
>> streaming mode.
>>
>> *** (#1 of 1): 2011-01-13 12:23:25 GMT+00:00 weijun.wang at oracle.com
>>