[RFC][icedtea-web] Use global JarCertVerifier in JNLPClassLoader - tests

[Danesh Dadachanji]

Hi Jiri,

On 17/05/12 05:45 AM, Jiri Vanek wrote:
> On 05/17/2012 12:24 AM, Danesh Dadachanji wrote:
>> Hi,
>
> I do not feel skilled enough to review this. Although as far as i walked through it looks very ok!
>
>> The attached patches make JNLPClassLoader use a global JCV to ensure we can actually check the app
>> is signed entirely by at least one common signer. Currently, the classloader does not maintain
>> verification of jars loaded upon main initialization vs jars loaded at runtime. Therefore, we are
>> not actually able to enforce an application to be signed entirely by one signer.
>>
>> The application may initially be signed entirely (i.e. all resources/archives specified have a
>> common signer) but then as classes are loaded at runtime (e.g. via manifest classpath), these are
>> not verified along with the original jars. A new JarCertVerifier is used to verify each set.
>>
>> I used an instance var per JNLPClassLoader to keep track of all the jars verified thus far. Each
>> JNLPClassLoader keeps track of its own app's jars so when JNLP extensions are specified, a new
>> JNLPClassLoader is created. This ensures there can be separate signers between different JNLPs.
>>
>> The current method to determine if a jar is signed completely was to check against
>> JarCertVerifier#anyJarSigned. This method is somewhat flawed. If any single entry is signed then
>> this method returns true. However, if one entry of a jar is signed and another unsigned, then the
>> jar is considered unsigned, as is the app as a whole. Therefore, I have included
>> JarCertVerifier#isFullySigned in the conditional as well to ensure the app is in fact fully signed.
>>
>> Another change is the removal of permission checking of nested jars. If the entry of the nested jar
>> is signed, then we should assume that the person signing the jar trusts it to do whatever it must.
>> The nested jar is given the same security context as its parent jar.
>>
>
>
>> I've extensively tested this against a combination of singed/unsigned entries in
>
> Please share those reproducers. They have to be tracked for regression.
>

Unfortunately all of these are hardcoded jars that I made manually. They're too complicated to add to the test engine. Things like 
having multiple signers for different jars, having only one of two entries in a jar signed are not possible yet.

I want to avoid pushing jars to the repo. However, I think out of the 50+ scenarios I ran through, there is maybe 10% similarity 
between all of them, if that even. Each test will need its own mechanism to create proper jars with each test run. We will have to add 
a new feature to the test engine to do these customized tests. As we discussed on IRC, the best looking approach is to use additional 
Makefiles in each of the tests' directories. Then, if these are present, the test is built according to this Makefile's specifications. 
I will have to think about this some more but I'll just leave this here for now. =)

>> resource/archive-specified/nested/manifest-classpath/extension jars using JNLPs, the applet tag,
>> javaws and jnlp_href, as well as many duplicate jars. I have also run through all the regression
>
> I think this changset do not belongs to 1.1. What is motivation for do so?

This is a very security oriented fix. Right now in 1.1 we do not ensure that the entire app is signed by 1 common signer. If jars are 
brought in on the fly (e.g. classpath), they currently can have different signers. Technically we are okay for JNLPs because of other 
restrictions (such as no manifest classpaths accepted) but IMHO that's too close of a call that it's scary. Having this global JCV will 
ensure that everything absolutely must be signed by at least 1 signer, not just the jars passed in when doing the verification.

>
>> tests for HEAD, 1.2 and 1.1, everything ran fine.
>>
>
>
>
> ^ This is true reason of my reply. This changeset will need bunch of reproducers. They will be better even before the push of this
> changeset so I will be able to track they fix.
>
> Sorry for adding more work but it is really necessary.

np =)

>
> Also when I walked through code of JarCertVerifier, it looks like easily to be tempted by unnit tests. What do you think?

Unit tests are hard for JCV. =( For my major update to JCV (that is still in the works), I focused on making the class more unit 
testable. Currently, it's very dependent on passing JARDesc lists into the methods. IOW we would have to create these jars inside of 
the unit tests. That's a whole other task in itself. TBH I think it will be wasted effort to do this if my larger JCV updates will make 
things easier.

>
>
> Thanx again, J.

Thanks for reviewing!

Cheers,
Danesh