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

[Philipp Kunz]

Hi Max,

Do I understand correctly that your question refers to
        if (e == null && MF_MAIN_ATTRS.equals(name)) {
            e = getMainAttsEntry();
        }
?

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.

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"?

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

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
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20190703/4cd315c5/attachment.htm>