On Tue, 17 Jan 2023 16:46:28 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> I started there, but ran into some problems: >> >> SignatureFileVerifier.isSigningRelated calls isBlockOrSF, but it removes the >> "META-INF/" prefix from the path first. So we can't assume that input to >> isBlockOrSF is the full path. >> >> I could update SignatureFileVerifier.isSigningRelated to send the full path, >> but we still have another problem: >> >> JarSigner.sign0 puts META-INF/ files in a vector, such that it can output >> them first. We want to update this method such that it outputs only files >> which live directly in META-INF/ first. So we still need to check for >> "directness" outside isBlockOrSF. >> >> isBlockOrSF has 8 call sites, most of them in security sensitive and tricky >> code. In the end, I felt safer leaving isBlockOrSF alone and just fix the >> bug-relevant call sites instead. Also, being a new contributor to >> security-dev, I wanted to keep the PR relatively simple and easy to review. >> >> WDYT? > > In almost every call to `isBlockOrSF`, there is already an `isInMetaInf` (or > equivalent) call right before it so it's always safe. The only exception is > in `jarsigner`'s `verifyJar` method: > > hasSignature = hasSignature > || SignatureFileVerifier.isBlockOrSF(name); > > and it makes a subtle difference. The > `unrelated-signature-files-modified.jar` your test created has SF/block files > inside a sub-directory. Try verifying it with jarsigner. Here `hasSignature` > is true but the SF/block is not considered signature-related, an error > message will show `Signature is either not parsable or not verifiable`. If > `isBlockOrSF` is modified to return false, it will be simply `jar is > unsigned`. Good catch! I've added a guarding call to SignatureFileVerifier.isSigningRelated here, similar to the earlier call site of isBlockOrSf in this methid. I've also added a new check to the test to verify that there is no WARNING when calling jarsigner with the modified jar. ------------- PR: https://git.openjdk.org/jdk/pull/11976
