[RFC[PATCH]: Updated Patch for validating signedJNLP file at launch
Saad Mohammad
smohammad at redhat.com
Mon Aug 15 09:04:08 PDT 2011
Thanks for reviewing my patch.
On 08/15/2011 09:31 AM, Omair Majid wrote:
> On 08/12/2011 04:18 PM, Saad Mohammad wrote:
>> This is the updated patch that validates a signed JNLP file when an
>> application is launched. If the signed JNLP file is invalid, it stop
>> the launch of the application.
>>
>>
>> ChangeLog:
>>
>> 2011-07-06 Saad Mohammad<smohammad at redhat.com>
>
> That date seems off by a bit ;)
Hehe, good catch!
>
>> * netx/net/sourceforge/jnlp/resources/Messages.properties:
>> Added LSignedJNLPFileDidNotMatch and SJNLPFileIsNotSigned.
>> * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
>> (initializeResources): Locates the jar file that contains the
>> main class
>> and verifies if a signed JNLP file is also located in that jar.
>> This also
>> checks 'lazy' jars if the the main class was not found in 'eager'
>> jars.
>> If the main jar was not found, a LaunchException is thrown which
>> terminates
>> the launch of the application.
>> (checkForMain): A method that goes through each jar and checks to
>> see
>> if it has the main class. If the main class was found, it calls
>> verifySignedJNLP() to verify if a valid signed JNLP file is also
>> found in
>> the jar.
>> (verifySignedJNLP): A method that checks if the jar file contains
>> a valid
>> signed JNLP file.
>> (closeInputStream): Closes an InputStream.
>> (closeInputReader): Closes an InputStreamReader
>> (showSignedJNLPWarning): Returns true if a signed JNLP warning
>> should be
>> shown in the security dialog. A signed JNLP warning should be
>> displayed
>> only if the main jar is signed but does not contain a signed JNLP
>> file.
>> (loadClassExt): Added a try/catch block when addNextResource() is
>> called.
>> (addNextResource): If the main jar has not been found,
>> checkForMain() is
>> called to check if the jar contains the main class, and verifies
>> if a signed
>> JNLP file is also located.
>> * netx/net/sourceforge/jnlp/security/MoreInfoPane.java:
>> (addComponents): Displays the signed JNLP warning message if
>> necessary.
>>
>>
>> 08-12-406
>>
>>
>> diff -r 68756a4f8cc0
>> netx/net/sourceforge/jnlp/resources/Messages.properties
>> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties Thu
>> Aug 11 14:11:41 2011 -0400
>> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties Fri
>> Aug 12 16:06:56 2011 -0400
>> @@ -80,6 +80,7 @@
>> LUnsignedJarWithSecurityInfo=Application requested security
>> permissions, but jars are not signed.
>> LSignedAppJarUsingUnsignedJar=Signed application using unsigned jars.
>> LSignedAppJarUsingUnsignedJarInfo=The main application jar is
>> signed, but some of the jars it is using aren't.
>> +LSignedJNLPFileDidNotMatch=The signed JNLP file did not match the
>> launching JNLP file.
>>
>> JNotApplet=File is not an applet.
>> JNotApplication=File is not an application.
>> @@ -210,6 +211,7 @@
>> SNotAllSignedDetail=This application contains both signed and
>> unsigned code. While signed code is safe if you trust the provider,
>> unsigned code may imply code outside of the trusted provider's control.
>> SNotAllSignedQuestion=Do you wish to proceed and run this
>> application anyway?
>> SAuthenticationPrompt=The {0} server at {1} is requesting
>> authentication. It says "{2}"
>> +SJNLPFileIsNotSigned=This application contains a digital signature
>> in which the launching JNLP file is not signed.
>>
>> # Security - used for the More Information dialog
>> SBadKeyUsage=Resources contain entries whose signer certificate's
>> KeyUsage extension doesn't allow code signing.
>> diff -r 68756a4f8cc0
>> netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Thu
>> Aug 11 14:11:41 2011 -0400
>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Fri
>> Aug 12 16:06:56 2011 -0400
>> @@ -19,8 +19,11 @@
>>
>> import java.io.File;
>> import java.io.FileOutputStream;
>> +import java.io.FileReader;
>> import java.io.IOException;
>> import java.io.InputStream;
>> +import java.io.InputStreamReader;
>> +import java.io.OutputStream;
>> import java.net.MalformedURLException;
>> import java.net.URL;
>> import java.net.URLClassLoader;
>> @@ -47,10 +50,13 @@
>> import java.util.jar.JarFile;
>> import java.util.jar.Manifest;
>>
>> +import net.sourceforge.jnlp.ApplicationDesc;
>> import net.sourceforge.jnlp.DownloadOptions;
>> import net.sourceforge.jnlp.ExtensionDesc;
>> import net.sourceforge.jnlp.JARDesc;
>> import net.sourceforge.jnlp.JNLPFile;
>> +import net.sourceforge.jnlp.JNLPMatcher;
>> +import net.sourceforge.jnlp.JNLPMatcherException;
>> import net.sourceforge.jnlp.LaunchException;
>> import net.sourceforge.jnlp.ParseException;
>> import net.sourceforge.jnlp.PluginBridge;
>> @@ -81,6 +87,16 @@
>> // extension classes too so that main file classes can load
>> // resources in an extension.
>>
>> + /** Signed JNLP File and Template */
>> + final public static String template =
>> "JNLP-INF/APPLICATION_TEMPLATE.JNLP";
>> + final public static String application =
>> "JNLP-INF/APPLICATION.JNLP";
>> +
>
> A stylistic nit: the variables should be named TEMPLATE and APPLICATION.
>
>> + /** True if the application has a signed JNLP File */
>> + private boolean isSignedJNLP = false;
>> +
>> + /** True if the application is signed but is missing a signed
>> JNLP File (warning should be shown); otherwise false */
>> + private static boolean signedJNLPWarning= false;
>> +
>
> Why is it static?
This is a static variable because showSignedJNLPWarning() is a static
method that returns this variable. This is used to determine if a signed
JNLP warning should be shown in the 'More Information' panel.
>
> Also, is this behaviour (if the jar is signed but does not have a
> sigend jnlp file, then a warning is shown) documented somewhere? I ask
> because I find it rather strange. AFAIK, a lot of applications fit
> this pattern. I am not sure if showing warnings for all of these makes
> sense.
Unfortunately, I did not find any documentations that state anything
about displaying a warning, but it's the way Oracle behave. I assume
IcedTea-web should behave similar?
I have also noticed that Oracle only display the warning if the jar file
is not verified. If it's a verified jar, it does not show any warning
regarding a signed JNLP. I have not made that change with IcedTea-Web, I
was debating whether it should show this warning every time or not. I
think I will implement that on my updated patch.
>
>> /** map from JNLPFile url to shared classloader */
>> private static Map<String, JNLPClassLoader> urlToLoader =
>> new HashMap<String, JNLPClassLoader>(); // never
>> garbage collected!
>> @@ -153,6 +169,9 @@
>>
>> /** Loader for codebase (which is a path, rather than a file) */
>> private CodeBaseClassLoader codeBaseLoader;
>> +
>> + /** True if a Jar file has been located that contains the main
>> class; otherwise false */
>> + boolean foundMainJar= false;
>>
>
> Private, please.
Will do.
>
>> /**
>> * Create a new JNLPClassLoader from the specified file.
>> @@ -460,6 +479,29 @@
>>
>> !SecurityDialogs.showNotAllSignedWarningDialog(file))
>> throw new LaunchException(file, null,
>> R("LSFatal"), R("LCClient"), R("LSignedAppJarUsingUnsignedJar"),
>> R("LSignedAppJarUsingUnsignedJarInfo"));
>>
>> +
>> + //Check for main class in the downloaded jars, and
>> check/verify signed JNLP file
>> + checkForMain(initialJars);
>> +
>> + //If jar with main class was not found, check
>> available resources
>> + while (!foundMainJar&& available != null&&
>> available.size() != 0) {
>> + addNextResource();
>> + }
>> +
>> + //If jar with main class was not found and there are
>> no more available jars, throw a LaunchException
>> + if(!foundMainJar&& (available == null ||
>> available.size() == 0))
>> + {
>> + throw new LaunchException(file, null,
>> + R("LSFatal"), R("LCClient"),
>> R("LCantDetermineMainClass"),
>> + R("LCantDetermineMainClassInfo"));
>> + }
>> +
>> + //If main jar was found, but a signed JNLP file was
>> not located
>> + if(!isSignedJNLP&& foundMainJar)
>> + {
>> + signedJNLPWarning= true;
>> + }
>> +
>
> Some of the indentation/spacing/newlines here look a bit odd. Please
> see http://icedtea.classpath.org/wiki/IcedTea-Web#Code_style.
I am not sure why the indentations are off, but I will make the changes.
>
>> //user does not trust this publisher
>> if (!js.getAlreadyTrustPublisher()) {
>> checkTrustWithUser(js);
>> @@ -518,8 +560,214 @@
>> System.err.println(mfe.getMessage());
>> }
>> }
>> + activateJars(initialJars);
>> + }
>> +
>> + /***
>> + * Checks for the jar that contains the main class. If the main
>> class was
>> + * found, it checks to see if the jar is signed and whether it
>> contains a
>> + * signed JNLP file
>> + *
>> + * @param jars
>> + * Jars that are checked to see if they contain the
>> main class
>> + * @throws LaunchException
>> + * Thrown if the signed JNLP file, within the main
>> jar, fails to
>> + * be verified or does not match
>> + */
>
> Please see http://icedtea.classpath.org/wiki/IcedTea-Web#Code_style
>
>> + private void checkForMain(List<JARDesc> jars) throws
>> LaunchException {
>>
>> - activateJars(initialJars);
>> + ApplicationDesc ad = (ApplicationDesc) file.getLaunchInfo();
>> + String mainClass = ad.getMainClass();
>> + for (int i = 0; i< jars.size(); i++) {
>> +
>> + try {
>> + File localFile =
>> tracker.getCacheFile(jars.get(i).getLocation());
>> +
>> + if (localFile == null)
>> + throw new NullPointerException(
>> + "Could not locate jar file, returned
>> null");
>> +
>> + JarFile jarFile = new JarFile(localFile);
>> + Enumeration<JarEntry> entries = jarFile.entries();
>> + JarEntry je;
>> +
>> + while (entries.hasMoreElements()) {
>> + je = entries.nextElement();
>> + String jeName = je.getName().replaceAll("/", ".");
>> +
>> + if (jeName.startsWith(mainClass)&&
>> jeName.endsWith(".class")) {
>
> I think this might match extra classes too.. I am thinking of inner
> classes here. If the main class is named my.main.Class then this might
> match my.main.Class$Inner.class too. Now, it's not likely that
> my.main.Class will be in a separate jar from my.main.Class$Inner, but
> you might want to fix this anyway.
Okay, I will implement this change.
>
>> + foundMainJar = true;
>> + verifySignedJNLP(jars.get(i), jarFile);
>> + break;
>> + }
>> + }
>> + }
>> + catch(IOException e)
>> + {
>> + // After this exception is caught, it is
>> + // escaped. This will skip the jarFile that may
>> + // have thrown this exception and move on to the
>> + // next jarFile (if there are any)
>> + }
>
> Please see the code style.
>
>> + }
>> + }
>> +
>> + /**
>> + * Is called by checkForMain() to check if the jar file is
>> signed and if it
>> + * contains a signed JNLP file.
>> + *
>> + * @param jarDesc
>> + * JARDesc of jar
>> + * @param jarFile
>> + * The jar file
>> + * @throws LaunchException
>> + * Thrown if the signed JNLP file, within the main
>> jar, fails to
>> + * be verified or does not match
>> + */
>> + private void verifySignedJNLP(JARDesc jarDesc, JarFile jarFile)
>> + throws LaunchException {
>> +
>> + JarSigner signer = new JarSigner();
>> + List<JARDesc> desc = new ArrayList<JARDesc>();
>> + desc.add(jarDesc);
>> +
>> + // Initialize streams
>> + InputStream inStream = null;
>> + InputStreamReader inputReader = null;
>> + FileReader fr = null;
>> + InputStreamReader jnlpReader = null;
>> +
>> + try {
>> + signer.verifyJars(desc, tracker);
>> +
>> + if (signer.allJarsSigned()) { // If the jar is signed
>> +
>> + Enumeration<JarEntry> entries = jarFile.entries();
>> + JarEntry je;
>> +
>> + while (entries.hasMoreElements()) {
>> + je = entries.nextElement();
>> + String jeName = je.getName().toUpperCase();
>> +
>> + if (jeName.equals(template) ||
>> jeName.equals(application)) {
>> +
>> + if (JNLPRuntime.isDebug())
>> + System.err.println("\tCreating Jar
>> InputStream from JarEntry");
>> +
>> + inStream = jarFile.getInputStream(je);
>> + inputReader = new InputStreamReader(inStream);
>> +
>
> I am not sure if performance here is a concern, but normally, you
> buffer Streams, but wrapping it in a BufferedReader/BufferedInputStream.
I will have to look into this one but I will wrap it in
BufferedReader/BufferedInputStream like you suggest and see how it works
out.
>
>> + if (JNLPRuntime.isDebug())
>> + System.err.println("\tCreating File
>> InputStream from lauching JNLP file");
>> +
>> + JNLPFile jnlp = this.getJNLPFile();
>> + URL url = jnlp.getFileLocation();
>> + File jn = null;
>> +
>> + // If the file is on the local file system,
>> use original path, otherwise find cached file
>> + if
>> (url.getProtocol().toLowerCase().equals("file"))
>> + jn = new File(url.getPath());
>> + else
>> + jn = CacheUtil.getCacheFile(url, null);
>> +
>> + fr = new FileReader(jn);
>> + jnlpReader = fr;
>> +
>> + //Initialize JNLPMatcher class
>> + JNLPMatcher matcher;
>> +
>> + if (jeName.equals(application)) { // If
>> signed application was found
>> + if (JNLPRuntime.isDebug())
>> +
>> System.err.println("\tAPPLICATION.JNLP has been located within signed
>> JAR. Starting verfication...");
>> +
>> + matcher = new JNLPMatcher(inputReader,
>> jnlpReader, false);
>> + } else { // Otherwise template was found
>> + if (JNLPRuntime.isDebug())
>> +
>> System.err.println("\tAPPLICATION_TEMPLATE.JNLP has been located
>> within signed JAR. Starting verfication...");
>> +
>> + matcher = new JNLPMatcher(inputReader,
>> jnlpReader, true);
>> + }
>> +
>> + // If signed JNLP file does not matches
>> launching JNLP file, throw JNLPMatcherException
>> + if (!matcher.isMatch())
>> + throw new JNLPMatcherException("Signed
>> Application did not match launching JNLP File");
>> +
>> + this.isSignedJNLP = true;
>> + if (JNLPRuntime.isDebug())
>> + System.err.println("\t** Signed
>> Application Verification Successful **");
>> +
>> + break;
>> + }
>
> We dont normally output debug info with tabs, or stars. I am not quite
> objecting to it, but is there a reason you have made the output that way?
The only reason why I was outputting it with tabs so it was much easier
to read. I will remove the tabs to have a consistent output pattern.
>
>> + }
>> + }
>> + } catch (JNLPMatcherException e) {
>> +
>> + // Throw LaunchException if signed JNLP file fails to be
>> verified or
>> + // fails to match launching JNLP file
>> + throw new LaunchException(file, null, R("LSFatal"),
>> R("LCClient"),
>> + R("LSignedJNLPFileDidNotMatch"),
>> R(e.getMessage()));
>> +
>> + // Throwing this exception will fail to initialize the
>> + // application resulting in the termination of the
>> + // application
>> +
>> + } catch (Exception e) {
>> +
>> + if (JNLPRuntime.isDebug())
>> + e.printStackTrace(System.err);
>> +
>> + // After this exception is caught, it is escaped.
>> + // If an exception is thrown while handling the jar file,
>> + // (mainly for JarSigner.verifyJars)
>> + // it assumes the jar file is unsigned and skip
>> + // the check for a signed JNLP file
>> + } finally {
>> +
>> + //Close all streams
>> + closeInputStream(inStream);
>> + closeInputReader(inputReader);
>> + closeInputReader(fr);
>> + closeInputReader(jnlpReader);
>> + }
>> +
>> + if (JNLPRuntime.isDebug())
>> + System.err.println("ENDING check for signed JNLP file...");
>> + }
>> +
>> + /***
>> + * Closes an input stream
>> + *
>> + * @param stream
>> + * The input stream that will be closed
>> + */
>> + private void closeInputStream(InputStream stream) {
>> + if (stream != null)
>> + try {
>> + stream.close();
>> + } catch (Exception e) {
>> + e.printStackTrace(System.err);
>> + }
>> + }
>> +
>> + /***
>> + * Closes an input stream reader
>> + *
>> + * @param stream
>> + * The input stream reader that will be closed
>> + */
>> + private void closeInputReader(InputStreamReader stream) {
>> + if (stream != null)
>> + try {
>> + stream.close();
>> + } catch (Exception e) {
>> + e.printStackTrace(System.err);
>> + }
>> + }
>> +
>
> How about replacing both these methods with a close(Closeable
> closeable) method? Since both InputStream and Reader derive from this,
> you wont need separate methods. Also, close() only throws IOException,
> so you might want to just catch that.
I was for a way to close both type of stream. Thanks, I will make this
change too :)
>
>> +
>> + public static boolean showSignedJNLPWarning()
>> + {
>> + return signedJNLPWarning;
>> }
>
> Why is this static? Also, the name can be a bit misleading. Invoking a
> method named <verb>Foo will normally take the action implied. This
> method does not show the warning.
This is a static method because MoreInfroPane calls this method without
having a JNLPClassLoader object to determine if a signed jnlp warning
should be displayed.
>
>>
>> private void checkTrustWithUser(JarSigner js) throws
>> LaunchException {
>> @@ -1154,7 +1402,18 @@
>>
>> // add resources until found
>> while (true) {
>> - JNLPClassLoader addedTo = addNextResource();
>> + JNLPClassLoader addedTo = null;
>> +
>> + try {
>> + addedTo = addNextResource();
>> + } catch (LaunchException e) {
>> +
>> + // This method will never handle any search for the
>> main
>> + // class [It is handled in initializeResources()].
>> + // Therefore, this exception will never be thrown
>> here and
>> + // is escaped
>> +
>
> If this code should never get executed, then I would put extra checks
> in here instead of swallowing the exception. How about something like:
>
> throw new IllegalStateException(e);
Yes, I will do that.
>
>> + }
>>
>> if (addedTo == null)
>> throw new ClassNotFoundException(name);
>> @@ -1245,8 +1504,9 @@
>> * no more resources to add, the method returns immediately.
>> *
>> * @return the classloader that resources were added to, or null
>> + * @throws LaunchException Thrown if the signed JNLP file,
>> within the main jar, fails to be verified or does not match
>> */
>> - protected JNLPClassLoader addNextResource() {
>> + protected JNLPClassLoader addNextResource() throws
>> LaunchException {
>> if (available.size() == 0) {
>> for (int i = 1; i< loaders.length; i++) {
>> JNLPClassLoader result = loaders[i].addNextResource();
>> @@ -1262,6 +1522,7 @@
>> jars.add(available.get(0));
>>
>> fillInPartJars(jars);
>> + checkForMain(jars);
>> activateJars(jars);
>>
>> return this;
>> diff -r 68756a4f8cc0
>> netx/net/sourceforge/jnlp/security/MoreInfoPane.java
>> --- a/netx/net/sourceforge/jnlp/security/MoreInfoPane.java Thu Aug
>> 11 14:11:41 2011 -0400
>> +++ b/netx/net/sourceforge/jnlp/security/MoreInfoPane.java Fri Aug
>> 12 16:06:56 2011 -0400
>> @@ -53,6 +53,8 @@
>> import javax.swing.JPanel;
>> import javax.swing.SwingConstants;
>>
>> +import net.sourceforge.jnlp.runtime.JNLPClassLoader;
>> +
>
> Is there a way you can avoid this dependency? This really should not
> be tied to the classloader.
Perhaps, I can create another class that holds the static variable that
can be used by MoreInfoPane?
I will have to look into this one. I will get back to you.
>
>> /**
>> * Provides the panel for the More Info dialog. This dialog shows
>> details about an
>> * application's signing status.
>> @@ -72,6 +74,10 @@
>> private void addComponents() {
>> ArrayList<String> details = certVerifier.getDetails();
>>
>> + //Show signed JNLP warning if the signed main jar does not
>> have a signed JNLP file
>> + if(JNLPClassLoader.showSignedJNLPWarning())
>> + details.add(R("SJNLPFileIsNotSigned"));
>> +
>
> This doesn't look right at all. This dialog is only shown if there is
> a security problem in the first place. I am not sure if the user
> should be shown this warning, but this will make it so that the user
> _only_ sees it if there is a bigger security concern.
>
The way Oracle is showing their warning message is only if the jar does
not have a verified signature. If the jar is verified and is part of the
trusted certificates, the warning is not shown (even if a signed JNLP
file is not found). I have not implemented this part, but I will send it
in the next patch.
I don't think it should show this warning for any other dialog. I think
it should only show it when it's with an unverified jar.
What do you think?
>> int numLabels = details.size();
>> JPanel errorPanel = new JPanel(new GridLayout(numLabels, 1));
>> errorPanel.setBorder(BorderFactory.createEmptyBorder(10,
>> 10, 10, 10));
>> @@ -88,6 +94,10 @@
>>
>> errorPanel.add(new JLabel(htmlWrap(details.get(i)),
>> icon, SwingConstants.LEFT));
>> }
>> +
>> + //Remove signed JNLP warning
>> + if(JNLPClassLoader.showSignedJNLPWarning())
>> + details.remove(details.size()-1);
>>
>
> Are you removing the warning unconditionally?
I am removing this warning after it has been added to the 'More
Information' panel. When I add to details, it also adds it to certVerifier.
That's why I am removing it because I don't want it to be part of
certVerifier permanently.
>
>> JPanel buttonsPanel = new JPanel(new BorderLayout());
>> JButton certDetails = new JButton(R("SCertificateDetails"));
>>
>
> Cheers,
> Omair
--
Cheers,
Saad Mohammad
More information about the distro-pkg-dev
mailing list