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

[Omair Majid]

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?

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).

>>
>>>> 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 :)

Thanks,
Omair