[RFC[PATCH]: Updated Patch for validating signedJNLP file at launch
Omair Majid
omajid at redhat.com
Mon Aug 15 06:31:06 PDT 2011
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 ;)
> * 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?
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.
> /** 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.
> /**
> * 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.
> //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.
> + 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.
> + 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?
> + }
> + }
> + } 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.
> +
> + 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.
>
> 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);
> + }
>
> 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.
> /**
> * 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.
> 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?
> JPanel buttonsPanel = new JPanel(new BorderLayout());
> JButton certDetails = new JButton(R("SCertificateDetails"));
>
Cheers,
Omair
More information about the distro-pkg-dev
mailing list