RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Seán Coffey]

Hi Philipp,

thanks for the reply .. comments inline

On 19/01/20 16:09, Philipp Kunz wrote:
> Hi Sean,
>
> I figure that distinguishing zips and jars is ambiguous in a certain
> way. After having signed a zip, it contains a manifest and is also a
> jar file according to the specs. That would mean we would end up with
> two kinds of jar files, those with permissions and those without,
> depending on the file name extension of the files. That a tool named
> jarsigner is successfully applied to a file is for me convincing enough
> to consider that file a jar file or jarsigned would reject it however
> it name ends.
Yes, both zip and jar files enjoy the same format. jar files being created
predominantly with the java based API/tools.
>
> Now when signing jar files (namely those with zip file name extension
> but depending on the further discussion not necessary limited to those)
> with permissions, I wonder if those permissions should also be subject
> of the signature. I would consider a change of an executable flag for
> example a change of the signed file just the same as a changed byte of
> contents. The way jarsigner works now, loosing the permissions, we know
> at least that the files contained in signed jars have certain
> permission flags (none or defaults) immediately after having signed
> them even though later manipulation would not be detected and, hence,
> cannot be trusted even with a valid signature.
historically, jar tool never recorded these permissions. As such, it 
should make
no difference to how jarsigner operates on such files. The original report
highlights the loss of data on files created with the zip utility. It 
think it's reasonable
for jarsigner to preserve attributes in such a case.
>
> Far as I understand the current situation, jars and zips could be told
> from one another only before signing depending on the presence or
> absence of a manifest. After signing, however, a manifest is always
> there. Looking at it that way, the "much more consideration" you
> mention would have to take place now, wouldn't it?
I'm not sure if there's much change in this area post fix. The classic
jar tool doesn't add or read the file attributes at hand. The name check
was useful to prevent any unintended behavioural changes in this area.
>
> I also wonder why the "if (filename.endsWith(".zip"))" piece of code
> does not occur only in JarSigner. Why is it in ZipFile, too? One such
> kind of if statement should have been enough, at first glance. There
> may be a good and convincing explanation but I don't see it just like
> that. On the other hand, having this if condition duplicated into
> ZipFile looks like it could have an undesired side-effect elsewhere. I
> also kind of miss another test case to make sure permission flags
> continue to be disregarded for "non-zip" jar files.
fair point. I should be able to remove this code. I can also improve the
test case.
>
> And the comment of Michael Osipov is really frightening. Did you know
> that i and I are two different characters in turkish both of which have
> yet other capital/small counterparts? At that point maybe discussing
> the file name extension distinction could easily become lengthier than
> a discussion about consequences of supporting permissions for all jar
> files.
frightening ??  Yes, I've worked on such issues before. Thanks to Michael
for pointing that out. I should be able to do a comparison using Locale.ROOT
>
> You mention "the case, at hand is unique here" and that made to occur
> to me that if there is maybe only one person affected that he or she
> could work around it more easily than changing the jdk. Is it really
> important for most others? And is it really a bug at all?
>
> Another idea to work around compatibility issues would be to introduce
> a flag for jarsigner, for example -p like with tar. Yet another idea
> could be to refire the jar file specs and jar and jarsigner tools docs
> accordingly.
Let me have a think about this. A new flag in jarsigner may help.

regards,
Sean.
>
> Regards,
> Philipp
>
>
> On Fri, 2020-01-17 at 13:07 +0000, Seán Coffey wrote:
>> Hi Philipp,
>>
>> On 17/01/2020 12:40, Philipp Kunz wrote:
>>> Hi Sean,
>>>
>>> Nice patch. I wonder why permissions should be preserved only in
>>> zip
>>> files. Jar files also are zip files, according to the jar file
>>> specs,
>>> and hence, shouldn't jar files benefit of preserving permissions,
>>> too?
>> Thanks for your comments. The jar tool has never been interested in
>> the
>> posix
>> permissions fields for the individual entries. Such a change could
>> yield
>> more
>> interoperability issues. Such a change would also need much more
>> consideration
>>
>> The zip tool on the other hand has always populated this field and I
>> think the case
>> at hand is unique here (preserving attributes already created by
>> non-java tools)
>>> The file name extension is most often zip for zip files and jar for
>>> jar
>>> files but is that really a safe assumption? I would not expect it
>>> always. Removing
>> Yes, I didn't see any easy way to distinguish a zip file from a jar
>> file
>> without being
>> more invasive and scanning file attributes for that file. I could
>> take
>> that approach
>> if it's deemed necessary.
>>
>> regards,
>> Sean.
>>>> if (zf.getName().toLowerCase().endsWith(".zip")) {
>>> along with similar code in ZipFile would avoid discussing that
>>> question
>>> and the test would not have to check that files with another name
>>> extension than zip don't preserve permissions.
>>>
>>> Philipp
>>>
>>>
>>> On Fri, 2020-01-17 at 10:59 +0000, Seán Coffey wrote:
>>>> Hi,
>>>>
>>>> Looking to introduce some JDK private functionality which will
>>>> help
>>>> preserve internal zip file attribute permissions when jarsigner
>>>> is
>>>> run
>>>> on a zip file. Some of the logic is taken from the recent work
>>>> carried
>>>> out in this area for zipfs API.
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8218021
>>>> http://cr.openjdk.java.net/~coffeys/webrev.8218021/webrev/
>>>>
>>>> regards,
>>>> Sean.
>>>>
>>>>