[RFC][PATCH][icedtea-web]: Added support for signed JNLP file- Updated Patch

[Saad Mohammad]

On 07/28/2011 03:01 PM, Omair Majid wrote:
> On 07/28/2011 02:33 PM, Saad Mohammad wrote:
>> On 07/25/2011 12:30 PM, Omair Majid wrote:
>>> On 07/25/2011 11:32 AM, Saad Mohammad wrote:
>>>> On 07/21/2011 09:34 AM, Omair Majid wrote:
>>>>> On 07/15/2011 10:21 AM, Saad Mohammad wrote:
>>>>>> I have updated both the patches that includes the change that you 
>>>>>> have
>>>>>> recommended and requested.
>>>>>
>>>>> I haven't looked into this in too much detail, but I have a few
>>>>> questions (and concerns) after reading this patch. They are included
>>>>> in line, below.
>>>>>
>>>>>
>>>>>> +
>>>>>> + if (js.anyJarsSigned()) {
>>>>>> + // If there are any signed Jars, check if JNLP file is signed
>>>>>> +
>>>>>> + if (JNLPRuntime.isDebug())
>>>>>> + System.out.println("STARTING check for signed JNLP file...");
>>>>>> +
>>>>>> + for (int i = 0; i< jars.length; i++) {
>>>>>> + List<JARDesc> eachJar = new ArrayList<JARDesc>();
>>>>>> + JarSigner signer = new JarSigner();
>>>>>> + eachJar.add(jars[i]); // Adds only the single jar to check
>>>>>> + // if the jar has a valid signature
>>>>>> +
>>>>>
>>>>> There is already JarSigner object that JNLPClassLoader is using. Is
>>>>> there a reason for creating this new JarSigner object?
>>>>
>>>> I can use the JarSigner object that already exists.
>>>>
>>>
>>> Well, I am not sure if that's the right thing. Perhaps it maintains
>>> some state that you would not be fine with? I was really hoping you
>>> would explain why you are using a new JarSigner object :)
>>
>> I will not need the new JarSigner object because the only purpose of it
>> is verifying individual jars. I thought every time you wanted to verify
>> a jar with the same JarSigner object, it would append each jar file as
>> part of the arraylists of unverifiedJars or verifiedJars. Therefore if I
>> used the JarSigner that already exists, it would break my IF condition
>> that verifies the signature of each jar(allJarsSigned()) because it
>> would have previous cached jar files that were verified using the same
>> JarSigner object before.
>>
>> However, I was incorrect, JarSigner intializes a new arraylist every
>> time verifyJars() is called. Therefore, I can still use the method,
>> allJarsSigned(), for checking the signature of each jar file. That's why
>> I will not need a new JarSigner object. :)
>>
>
> Given that js is an instance variables of the class, things may go 
> wrong if js is used somewhere and that code assumes js contains all 
> jars. Even if it's quite safe, the internal state modification means I 
> am leaning towards a new JarSigner.
>
>>>
>>>>>
>>>>>> + try {
>>>>>> + signer.verifyJars(eachJar, tracker);
>>>>>> +
>>>>>
>>>>> The jarsigner js has already verified a subset of these earlier. Do
>>>>> you really want to verify everything again?
>>>>
>>>> This is the method that is checking each jar file individually. So
>>>> 'eachJar' stores only one jar file at a time and then checks if 
>>>> that jar
>>>> file is signed or not. It then uses allJarsSigned() to validate if the
>>>> jar file is signed. I added this because I did not find a solution 
>>>> that
>>>> can track whether an individual jar file is signed or not. The 
>>>> previous
>>>> check that JarSigner does uses all the jar resources (passed as
>>>> parameter) and does not keep track which jar files are signed and 
>>>> which
>>>> are not. Unless I am mistaken and there is a way of determining this.
>>>>
>>>
>>> Ah, that makes sense now. I suppose you only want to make sure that
>>> only a signed jar file contains the signed jnlp file.
>>
>> I did a bit more of research. The link you have sent me & some other
>> website mentions that the jar file with the main class must be the ONLY
>> one that is checked for a signed JNLP file.
>>
>
> What if there is no main class? Can you please verify that the 
> proprietary javaws behaves this way?

Sorry, I am not sure what you exactly mean by having 'no main class'. 
Wouldn't javaws have nothing to start and fail at initialization? 
Therefore a check for signed JNLP file will not be executed.
So definitely, a JNLP file would need a main class that will be called 
to run the application. So a main class is a requirement, right? Please 
correct me if I am mistaken.

>
> Anyway, do you intend to post an updated patch to address this? 
> Otherwise, as I mentioned before, your patch will download all jars 
> (which makes the 'lazy' download mechanism useless).

I will update the patch but only for the main jar file (I am guessing 
the 'main jar file' means the jar file that has the main class) If the 
main jar file is 'lazy', it still needs to be downloaded first and 
checked right away. In this case, I will just need to update the patch 
and force it to only download the main jar file regardless to whether 
it's 'lazy' or 'eagar'.

There is a bug for the 'lazy' download mechanism that I will need to 
address after this (PR765)
[http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=765].

Before I do this, I am going to confirm that it only checks the main jar 
file for a signed JNLP file and not the other signed jar files that may 
be used as a resource. If it does NOT ONLY check the main jar file, then 
I will update the patch that will download only 'eagar' type jar files 
and check them for signed JNLP file. I will then need to add the check 
for signed JNLP file for 'lazy' jar files with the PR765 fix.

>
>>>
>>>>> So here's what I am really confused about: what does matching do? I
>>>>> see that if there is no signed jnlp file, the code runs normally. If
>>>>> there is a correctly signed jnlp file the code also continues
>>>>> normally. So why are signed jnlp files needed at all? What problem 
>>>>> are
>>>>> they solving?
>>>>>
>>>>> I guess I am missing something obvious; an explanation of what this
>>>>> code is trying to do would be appreciated.
>>>>
>>>> If there is no signed JNLP file, the code runs normally. (as you
>>>> mentioned above)
>>>> If there is a matching signed JNLP file, the code runs normally. 
>>>> (as you
>>>> mentioned above)
>>>> If there is an unmatched signed JNLP file, the application is not
>>>> initialized and fails to start. The reason why I found this beneficial
>>>> is because the signer may not want their signed jars to be used if the
>>>> launching JNLP file is not the same as the one the signer provides.
>>>> Anyone can just create their own JNLP file to launch the 
>>>> application as
>>>> long as they know the location of the resource(s). I think this way 
>>>> the
>>>> signer has the option to put restrictions onto their signed jar 
>>>> file; so
>>>> under certain conditions the jar files can be used only if the 
>>>> launching
>>>> JNLP file is "approved" by the signer. I think another reason why a
>>>> signer might do this is so the signer does not have another person
>>>> running their code and using their signed jar files as resource (using
>>>> the API? If they know it). I am not sure about this one, but it makes
>>>> sense.
>>>>
>>>
>>> Personally, I don't see that as a problem that Sun/Oracle would
>>> attempt to solve (what's so special about this "approved" file?). I
>>> suspect it has more to do with granting additional security privileges.
>>>
>>> I found this thread [1] after a quick search and it mentions a number
>>> of things you may find interesting (including secure system
>>> properties). There is probably a lot more information on this. Could
>>> you please look around some more and tell us what you find?
>>
>> I been reading couple of forum posts and it seems that you may be right.
>> Certain properties need JNLP files to be signed. In the same forum, it
>> also mentions:
>> "AFAIU, the only reason this JNLP needs to be signed is because
>> java-vm-args were used."
>> (http://forums.oracle.com/forums/thread.jspa?threadID=1359245&tstart=0)
>>
>> I am unable to find detailed documentation on these features but I have
>> to look more into this. After my patch2 and the fixes for patch1 are
>> both committed, I will take a look into this more and find other
>> advantages of signing a JNLP file. As of now, I want to push patch2
>> first because I think that is also one of the key features of signing a
>> JNLP file.
>>
>
> Ah, nice find! That thread mentions that if there is a signed jnlp 
> file and it does not match the original jnlp file then javaws should 
> not run the jnlp application. This is exactly what your patch does :)

Thank You. :)

>
> Thanks,
> Omair


-- 
Cheers,
Saad Mohammad