RFR 8217375: jarsigner breaks old signature with long lines in manifest

Weijun Wang weijun.wang at oracle.com
Wed Jul 3 10:27:18 UTC 2019



> On Jul 3, 2019, at 4:06 PM, Philipp Kunz <philipp.kunz at paratix.ch> wrote:
> 
> Hi Max,
> 
> Do I understand correctly that your question refers to
>         if (e == null && MF_MAIN_ATTRS.equals(name)) {
>             e = getMainAttsEntry();
>         }
> ?

Yes.

> 
> If so, because this sticks closest to the previous behavior. According to src/java.base/share/classes/module-info.java, ManifestDigester is not publicly exported but how can it then be used then in JarVerifier in java.util.jar? If you or someone can confirm that we can identify all callers, I'd prefer to remove above three lines of code.

It's not publicly exported therefore cannot be used by an application, but it's accessible by java.util.jar which is also in the java.base module.

It can also be accessed by JDK modules listed in the "exports sun.security.util to" block of java.base/share/classes/module-info.java.

Anyway any caller must be inside JDK. IntelliJ IDEA tells me its only SignatureFileVerifier and JarSigner. Hopefully whatever IDE you are using it gives the same result.

> 
> Otherwise, with above three lines, main attributes and an entry with name MF_MAIN_ATTRS will still be confused for all those code continuing to call get as before the patch if an entry with such a name exists just like before. However, with the patch, all (known) places that call ManifestDigester.get* are lines 539 and 606 in SignatureFileVerifier with the patch, 539 calling getMainAttsEntry without any ambiguity and 606 calling get(String, b) always expecting the entry for a section that is present and would otherwise not be known to pass as an argument anyway, provided the signature file does not contain more entries than the manifest or entries have not been removed from the manifest after signing, should work correctly, I guess. If we cannot get rid of those three lines, another test for those edge cases should probably be added, shouldn't it, if not to support the previous confusion at least to show it now works as expected?
> 
> What exactly did you mean with "coding error"?

Not sure. I don't know if any bad thing would happen if a file named "Manifest-Main-Attributes" is out inside a jar file.

> 
> Along with removing above three lines of code, two tests would have to be adjusted. FindHeaderEndVsManifestDigesterFindFirstSection on lne 235 and DigestInput on line 270.

That's OK. No real app should call methods in this class directly.

Thanks,
Max

> 
> Regards,
> Philipp
> 
> 
> 
> 
> 
> 
> On Mon, 2019-07-01 at 15:41 +0800, Weijun Wang wrote:
>> https://cr.openjdk.java.net/~weijun/8217375/webrev.02
>>  uploaded. There are still several trailing spaces and I've removed them.
>> 
>> I just ran a test job: SignTwice.java failed on Windows with `failed to clean up files after test`. Most likely a file is not closed.
>> 
>> The src change looks good. In fact, I am wondering if we need to support ManifestDigester.get(MF_MAIN_ATTRS, b) at all. Is it possible we miss a coding error because of it?
>> 
>> I'll take a look at the tests.
>> 
>> Thanks,
>> Max
>> 
>> 



More information about the security-dev mailing list