Jarsigner Fails To Verify Signed By Alias If Alias Given In Wrong Case

[Philipp Kunz]

Hi Max and everyone,
With respect to the previous patch, parentheses moved from storeHash to
printCert, bug number added, and some comments added and clarified.
Regards,Philipp


On Tue, 2019-04-30 at 14:45 +0800, Weijun Wang wrote:
> > On Apr 30, 2019, at 1:56 PM, Philipp Kunz <philipp.kunz at paratix.ch>
> > wrote:
> > 
> > Hi Max,
> > 
> > I agree that the parentheses are superfluous, however, I would
> > slightly prefer going small steps.
> > 
> > Would the parentheses have to come from a resource bundle as well
> > like SPACE or COMMA when removed from storeHash values?
> 
> No. It's hardcoded and not localized in printCert.
> 
> > Now that with the patch, storeHash is not any longer useful to
> > check whether it contains ckaliases in inKeyStoreForOneSigner (but
> > currently still useful to save another call to
> > store.getCertificateAlias in printCert lateron), would it be a
> > worth a consideration to rearrange the code populating storeHash
> > closer to printCert in some way or even just completely remove
> > storeHash and replace it with store.getCertificateAlias in
> > printCert or anything similar?
> 
> Probably not. I believe storeHash was introduced to avoid too many
> calls to store.getCertificateAlias when there are hundreds of classes
> in a signed jar. 
> 
> --Max
> 
> > Regards,
> > Philipp
> > 
> > 
> > 
> > 
> > 
> > On Sat, 2019-03-30 at 23:01 +0800, Weijun Wang wrote:
> > > Hi Philipp,
> > > 
> > > Sorry I'm so late giving a response. I've filed
> > > 
> > >    
> > > https://bugs.openjdk.java.net/browse/JDK-8221719
> > > 
> > >    Jarsigner Fails To Verify Signed By Alias If Alias Given In
> > > Wrong Case
> > > 
> > > The fix is good. Since we don't support identitydb anymore I
> > > think we can remove the parenthesis around the alias in the
> > > storeHash now.
> > > 
> > > Thanks,
> > > Max
> > > 
> > > 
> > > > On Feb 20, 2019, at 8:41 PM, Philipp Kunz <
> > > > philipp.kunz at paratix.ch
> > > > > wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > When signing a jarfile the specified alias is converted by
> > > > JavaKeyStore (storetype JKS) to lower case.
> > > > When verifying the jarfile with the same alias, it is currently
> > > > not converted by JKS to lower case and jarsigner reports that
> > > > the jar is not signed by the alias even after having just
> > > > signed it with that same alias if not all characters are in
> > > > lower case.
> > > > 
> > > > I found no statement in the documentation that an alias would
> > > > support only lower case or which characters could be used. It
> > > > could, however, be worth noting in the documentation of
> > > > JavaKeyStore or the keytool that the alias can be changed by
> > > > the keystore in certain circumstances.
> > > > In my opinion, it feels like a bug that aliases are treated
> > > > differently when signing and verifying with respect to their
> > > > upper and lower case.
> > > > 
> > > > Find a patch attached. Some explanations, just in case not too
> > > > obvious anyway:
> > > > 
> > > > - The jarsigner never changed the upper and lower cases of
> > > > aliases itself and neither does with the patch. That is now
> > > > delegated to the keystore implementation not only for signing
> > > > but in addition also for verifying. The fix is therefore not
> > > > JSK-specific.
> > > > - I assume it is correct that the if (store != null) check and
> > > > the try catch KeyStoreException can both be moved outside the
> > > > for loop over the certificates because before as well as now
> > > > with the patch the result was alway 0 if store was null and
> > > > KeyStoreException can happen only when loading the keystore,
> > > > which is certainly true for JKS.
> > > > - storeHash is used only by printCert and
> > > > inKeyStoreForOneSigner and contains the aliases as values
> > > > enclosed in parentheses "(" and ")" which is how it is printed
> > > > by printCert. It would have probably been easier to add the
> > > > parentheses only later in printCert but then I tried to change
> > > > as little as possible.
> > > > - Two pairs of "result |= " statements were duplicate.
> > > > Therefore there are the two methods in the test which show that
> > > > both actually failed with "ckaliases.contains" being the
> > > > offending piece of code. In the patch I refactored to recombine
> > > > them.
> > > > - I really wonder how "result |= SIGNED_BY_ALIAS;" should be
> > > > set for any certificate c and not only the one an alias in
> > > > ckaliases points at but I guess that is another story if one at
> > > > all and might have come from filling storeHash with all
> > > > certificates and not only the ones in ckaliases or so.
> > > > - At first glance it might look like a mistake that "alias" is
> > > > not used inside the loop over ckaliases but instead of
> > > > comparing "alias" in lower case to ckaliases with unspecified
> > > > case, the certificate c is now compared to the one gotten from
> > > > the keystore handling the alias case in its own way.
> > > > 
> > > > Would someone sponsor this fix or can/should it be improved?
> > > > 
> > > > Regards,
> > > > Philipp
> > > > <JavaKeyStoreAliasCaseInsensitive.patch>
> > > > 
> > > 
> > > 
> > > 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20190501/0f54b18c/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20190501-JavaKeyStoreAliasCaseInsensitive.patch
Type: text/x-patch
Size: 10737 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20190501/0f54b18c/20190501-JavaKeyStoreAliasCaseInsensitive.patch>