[RFC][icedtea-web]: Added support for signed JNLP file
Jiri Vanek
jvanek at redhat.com
Mon Jun 27 07:21:44 PDT 2011
On 06/23/2011 10:47 PM, Saad Mohammad wrote:
> I have attached my patch that adds support for signed JNLP files to IcedTea-Web. There are two ways of signing a JNLP file;
> You can do it either way by having the following files in a signed jar:
> JNLP-INF/APPLICATION.JNLP
> JNLP-INF/APPLICATION_TEMPLATE.JNLP (Introduced in JNLP 7.0) [http://jcp.org/aboutJava/communityprocess/maintenance/jsr056/jnlp-7_0-changes.html]
>
> It will fail to launch an application if the signed JNLP file does not match the launching JNLP file.
Hi,
Nice work!
Several comments inline, but security impact of your code in classloader must check Deepak or Omair.
Do you think taht you can post your code in two patches? First will be around matching algorithm itself with proper unit tests. Second with usage in classlaoder ant stuff around. In besat case you can include any reproducers-tests.
>
> CHANGELOG:
>
> 2011-06-23 Saad Mohammad <smohammad at redhat.com>
>
> * netx/net/sourceforge/jnlp/JNLPVerify.java:
> Created this class to compares signed JNLP file with the launching JNLP file.
> It is able to compare both method of signing of a JNLP file: APPLICATION_TEMPLATE.JNLP
> and APPLICATION.JNLP
> * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
> Added a custom exception: JNLPVerifyException. Also added JNLPVerifyException to methods
> that throws the custom exception.
> (initializeResources): Checks if there is any signed JNLP file within the signed jars.
> If signed JNLP file fails to match or fails to be verified, the application throws
> a JNLPVerifyException.
> * netx/net/sourceforge/jnlp/Node.java:
> Created a method that retrieves the attribute names of the Node in a
> private string [] member. The method returns the attribute names.
> * netx/net/sourceforge/nanoxml/XMLElement.java:
> (sanitizeInput): Changed parameter type of isr from InputStreamReader to Reader.
>
> --
> Cheers,
> Saad Mohammad
>
>
> 06-23-357
>
>
> diff -r e0741a8c44b6 netx/net/sourceforge/jnlp/JNLPVerify.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/JNLPVerify.java Thu Jun 23 15:57:15 2011 -0400
> @@ -0,0 +1,247 @@
> +package net.sourceforge.jnlp;
> +
> +import java.io.IOException;
> +import java.io.InputStreamReader;
> +import java.io.PipedInputStream;
> +import java.io.PipedOutputStream;
> +import java.io.Reader;
> +import java.util.ArrayList;
> +import java.util.Arrays;
> +import java.util.LinkedList;
> +import net.sourceforge.nanoxml.XMLElement;
> +import net.sourceforge.nanoxml.XMLParseException;
> +
> +/**
> + * To compare launching JNLP file with signed APPLICATION.JNLP and
> + * APPLICATION_TEMPLATE.jnlp.
> + *
> + * Used by net.sourceforge.jnlp.runtime.JNLPCLassLoader
> + */
> +
> +public final class JNLPVerify {
> +
> + private Node appTemplateNode = null;
> + private Node launchJNLPNode = null;
> + boolean isTemplate = false;
> +
> + /**
> + * Used to create an instance of JNLPVerify
> + *
> + * @param appTemplateFile
> + * the reader stream of the signed APPLICATION.jnlp or
> + * APPLICATION_TEMPLATE.jnlp
> + * @param launchJNLPFile
> + * the reader stream of the launching JNLP file
> + * @param isTemplate
> + * a boolean that specifies if appTemplateFile is a template
> + *
> + * @return a new instance of JNLPVerify if the class was successfully
> + * constructed
> + *
> + * @throws Exception
> + * if constructing a new instance of JNLPVerify fails
> + */
> + public static JNLPVerify getInstanceOf(Reader appTemplateFile, Reader launchJNLPFile,
> + boolean isTemplate) throws Exception {
> + try {
> + JNLPVerify ret = new JNLPVerify(appTemplateFile, launchJNLPFile, isTemplate);
> + return ret;
> +
> + } catch (Exception e) {
> + throw new Exception(
> + "Failed to create an instance of JNLPVerify with specified Readers: "
> + + e.getMessage());
> + }
> + }
"catch (Exception e) { throw new Exception("
??? This looks really strage:) If you want to unify all possible exceptions under one (which i prefer), add Your own exception, and use new MyException((String message, )Throwable cause) instead of poor string.
> +
> + /**
> + * Private constructor
> + *
> + * @param appTemplate
> + * the reader stream of the signed APPLICATION.jnlp or
> + * APPLICATION_TEMPLATE.jnlp
> + * @param launchJNLP
> + * the reader stream of the launching JNLP file
> + * @param isTemplate
> + * a boolean that specifies if appTemplateFile is a template
> + *
> + * @throws XMLParseException
> + * if appTemplate and launchJNLP could not be parsed
> + * @throws IOException
> + * if PipedOutputStream or InputStreamReader failed to be
> + * created
> + */
> + private JNLPVerify(Reader appTemplate, Reader launchJNLP, boolean isTemplate)
> + throws XMLParseException, IOException {
> +
> + if (appTemplate != null&& launchJNLP != null) {
> +
> + XMLElement appTemplateXML = new XMLElement();
> + XMLElement launchJNLPXML = new XMLElement();
> +
> + // Remove the comments and CDATA from the JNLP file
> + final PipedInputStream pinTemplate = new PipedInputStream();
> + final PipedOutputStream poutTemplate = new PipedOutputStream(pinTemplate);
> + appTemplateXML.sanitizeInput(appTemplate, poutTemplate);
> +
> + final PipedInputStream pinJNLPFile = new PipedInputStream();
> + final PipedOutputStream poutJNLPFile = new PipedOutputStream(pinJNLPFile);
> + launchJNLPXML.sanitizeInput(launchJNLP, poutJNLPFile);
> +
> + //Parse both files
> + appTemplateXML.parseFromReader(new InputStreamReader(pinTemplate));
> + launchJNLPXML.parseFromReader(new InputStreamReader(pinJNLPFile));
> +
> + //Initialize parent nodes
> + this.appTemplateNode = new Node(appTemplateXML);
> + this.launchJNLPNode = new Node(launchJNLPXML);
> + this.isTemplate = isTemplate;
> +
> + } else {
> + if (appTemplate == null&& launchJNLP == null)
> + throw new NullPointerException(
> + "Template JNLP file and Launching JNLP file are both null.");
> + else if (appTemplate == null)
> + throw new NullPointerException("Template JNLP file is null.");
> + else
> + throw new NullPointerException("Launching JNLP file is null.");
> + }
> +
> + }
little bit clean-up can be healthy. Throw the exceptions at the start of constructor, instead of if/else bloks.
> +
> + /**
> + * Compares both JNLP files
> + *
> + * @return true if both JNLP files are 'matched', otherwise false
> + */
> + public boolean compare() {
> + return compareNodes(appTemplateNode, launchJNLPNode);
> + }
> +
> + /**
> + * Compares two Nodes regardless of order of their children/attributes
> + *
> + * @param appTemplate
> + * signed application or template's Node
> + * @param launchJNLP
> + * launching JNLP file's Node
> + *
> + * @return true if both Nodes are 'matched', otherwise false
> + */
> + private boolean compareNodes(Node appTemplate, Node launchJNLP) {
> +
> + if (appTemplate != null&& launchJNLP != null) {
> +
> + Node appTemplateNode = (Node) appTemplate;
> + Node launchJNLPNode = (Node) launchJNLP;
> +
> + //Store children of Node
> + LinkedList<Node> appTemplateChild = new LinkedList<Node>(Arrays.asList(appTemplateNode
> + .getChildNodes()));
> + LinkedList<Node> launchJNLPChild = new LinkedList<Node>(Arrays.asList(launchJNLPNode
> + .getChildNodes()));
> +
> + // Compare only if both Nodes have the same name, else return false
> + if (appTemplateNode.getNodeName().equals(launchJNLPNode.getNodeName())) {
> +
> + if (appTemplateChild.size() == launchJNLPChild.size()) { // Compare children
> +
> + int childLength = appTemplateChild.size();
> +
> + for (int i = 0; i< childLength;) {
> + for (int j = 0; j< childLength; j++) {
> + boolean isSame = compareNodes(appTemplateChild.get(i),
> + launchJNLPChild.get(j));
> +
> + if (!isSame&& j == childLength - 1)
> + return false;
> + else if (isSame) { // If both child matches, remove them from the list of children
> + appTemplateChild.remove(i);
> + launchJNLPChild.remove(j);
> + --childLength;
> + break;
> + }
> + }
> + }
> +
> +
> + if (!appTemplateNode.getNodeValue().equals(launchJNLPNode.getNodeValue())) {
> +
> + //If it's a template and the template's value is NOT '*'
> + if (isTemplate&& !appTemplateNode.getNodeValue().equals("*"))
> + return false;
> + //Else if it's not a template, then return false
> + else if (!isTemplate)
> + return false;
> + }
> + //Compare attributes of both Nodes
> + return compareAttributes(appTemplateNode, launchJNLPNode);
> + }
> +
> + }
> + }
> + return false;
> + }
> +
> + /**
> + * Compares attributes of two Nodes regardless of order
> + *
> + * @param appTemplateNode
> + * signed application or template's Node with attributes
> + * @param launchJNLPNode
> + * launching JNLP file's Node with attributes
> + *
> + * @return true if both Nodes have 'matched' attributes, otherwise false
> + */
> + private boolean compareAttributes(Node appTemplateNode, Node launchJNLPNode) {
> +
> + if (appTemplateNode != null&& launchJNLPNode != null) {
> +
> + ArrayList<String> appTemplateAttributes = new ArrayList<String>(Arrays.asList(appTemplateNode.getAttributeNames()));
> + ArrayList<String> launchJNLPAttributes = new ArrayList<String>(Arrays.asList(launchJNLPNode.getAttributeNames()));
> +
> + if (appTemplateAttributes.size() == launchJNLPAttributes.size()) {
> +
> + int size= appTemplateAttributes.size(); //Number of attributes
> +
> + for (int i = 0; i< size;) {
> + for (int j = 0; j< size; j++) {
> +
> + if (launchJNLPAttributes.get(i).equals(appTemplateAttributes.get(j))) { // If both Node's attribute name are the same then compare the values
> +
> + String attribute = launchJNLPAttributes.get(i);
> +
> + //If it is a template and it's value is '*', then consider the attributes to have matched value
> + if (isTemplate&& appTemplateNode.getAttribute(attribute).equals("*")) {
> +
> + launchJNLPAttributes.remove(i);
> + appTemplateAttributes.remove(j);
> + --size;
> + break; //Break for-loop
> + }
> +
> + boolean isSame = appTemplateNode.getAttribute(attribute).equals(// Check if the Attribute values match
> + launchJNLPNode.getAttribute(attribute));
> +
> + if (!isSame&& j == size- 1) //If the values did not match and there are no more attributes to compare to, return false
> + return false;
> +
> + else if (isSame) {
> + launchJNLPAttributes.remove(i);
> + appTemplateAttributes.remove(j);
> + --size;
> + break; //Break for-loop
> + }
> +
> + } else if (!launchJNLPAttributes.get(i).equals(appTemplateAttributes.get(j))
> +&& j == size- 1) //If there are no more attributes to compare to, return false
> + return false;
> + }
> +
> + }
> + return true;
> + }
> + }
> + return false;
> + }
> +}
This gave sense. But pelase, Can you deliver it in separate patch with unit tests?
> diff -r e0741a8c44b6 netx/net/sourceforge/jnlp/Node.java
> --- a/netx/net/sourceforge/jnlp/Node.java Tue Jun 14 13:30:55 2011 -0400
> +++ b/netx/net/sourceforge/jnlp/Node.java Thu Jun 23 15:57:15 2011 -0400
> @@ -19,6 +19,7 @@
> private XMLElement xml;
> private Node next;
> private Node children[];
> + private String attributeNames[];
>
> Node(XMLElement xml) {
> this.xml = xml;
> @@ -60,6 +61,19 @@
>
> return children;
> }
> +
> + String[] getAttributeNames() {
> + if (attributeNames == null) {
> + List<String> list = new ArrayList<String>();
> +
> + for (Enumeration e = xml.enumerateAttributeNames(); e.hasMoreElements();)
> + list.add(new String((String) e.nextElement()));
> +
> + attributeNames = list.toArray(new String[list.size()]);
> +
> + }
> + return attributeNames;
> + }
Still array? yu are creating array here from list and then list form array. This looks like nonsense:)
>
> String getAttribute(String name) {
> return (String) xml.getAttribute(name);
> diff -r e0741a8c44b6 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Tue Jun 14 13:30:55 2011 -0400
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Thu Jun 23 15:57:15 2011 -0400
> @@ -19,8 +19,10 @@
>
> 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.net.MalformedURLException;
> import java.net.URL;
> import java.net.URLClassLoader;
> @@ -51,6 +53,7 @@
> import net.sourceforge.jnlp.ExtensionDesc;
> import net.sourceforge.jnlp.JARDesc;
> import net.sourceforge.jnlp.JNLPFile;
> +import net.sourceforge.jnlp.JNLPVerify;
> import net.sourceforge.jnlp.LaunchException;
> import net.sourceforge.jnlp.ParseException;
> import net.sourceforge.jnlp.PluginBridge;
> @@ -158,8 +161,9 @@
> * Create a new JNLPClassLoader from the specified file.
> *
> * @param file the JNLP file
> + * @throws JNLPVerifyException if signed JNLP file failed to be verified or did not match
> */
> - protected JNLPClassLoader(JNLPFile file, UpdatePolicy policy) throws LaunchException {
> + protected JNLPClassLoader(JNLPFile file, UpdatePolicy policy) throws LaunchException, JNLPVerifyException {
> super(new URL[0], JNLPClassLoader.class.getClassLoader());
>
> if (JNLPRuntime.isDebug())
> @@ -273,8 +277,9 @@
> *
> * @param file the file to load classes for
> * @param policy the update policy to use when downloading resources
> + * @throws JNLPVerifyException if signed JNLP file failed to be verified or did not match
> */
> - public static JNLPClassLoader getInstance(JNLPFile file, UpdatePolicy policy) throws LaunchException {
> + public static JNLPClassLoader getInstance(JNLPFile file, UpdatePolicy policy) throws LaunchException, JNLPVerifyException {
> JNLPClassLoader baseLoader = null;
> JNLPClassLoader loader = null;
> String uniqueKey = file.getUniqueKey();
> @@ -341,9 +346,10 @@
> * @param location the file's location
> * @param version the file's version
> * @param policy the update policy to use when downloading resources
> + * @throws JNLPVerifyException if signed JNLP file failed to be verified or did not match
> */
> public static JNLPClassLoader getInstance(URL location, String uniqueKey, Version version, UpdatePolicy policy)
> - throws IOException, ParseException, LaunchException {
> + throws IOException, ParseException, LaunchException, JNLPVerifyException {
> JNLPClassLoader loader = urlToLoader.get(uniqueKey);
>
> if (loader == null || !location.equals(loader.getJNLPFile().getFileLocation()))
> @@ -403,7 +409,7 @@
> * Load all of the JARs used in this JNLP file into the
> * ResourceTracker for downloading.
> */
> - void initializeResources() throws LaunchException {
> + void initializeResources() throws LaunchException, JNLPVerifyException {
> JARDesc jars[] = resources.getJARs();
> if (jars == null || jars.length == 0)
> return;
> @@ -471,8 +477,129 @@
> //otherwise this jar is simply unsigned -- make sure to ask
> //for permission on certain actions
> }
> +
> + 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...");
> +
> + final String template = "JNLP-INF/APPLICATION_TEMPLATE.JNLP";
> + final String application = "JNLP-INF/APPLICATION.JNLP";
Those should be decalred outside method. Even as publc static final. They are sitll constans.
> +
> + 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
> +
> + tracker.addResource(jars[i].getLocation(), jars[i].getVersion(),
> + getDownloadOptionsForJar(jars[i]),
> + jars[i].isCacheable() ? JNLPRuntime.getDefaultUpdatePolicy()
> + : UpdatePolicy.FORCE);
> +
> + try {
> + signer.verifyJars(eachJar, tracker);
> +
> + if (signer.allJarsSigned()) { //If the jar is signed
> + URL location = jars[i].getLocation();
> + File localFile = tracker.getCacheFile(location);
> +
> + if (localFile != null) {
> + try {
> + JarFile jarFile = new JarFile(localFile);
> + 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.out.println("\tCreating Jar InputStream from Jar Entry");
> +
> + InputStream inStream = jarFile.getInputStream(je);
> + InputStreamReader inputReader = new InputStreamReader(
> + inStream);
> +
> + if (JNLPRuntime.isDebug())
> + System.out.println("\tCreating File InputStream from lauching JNLP file");
> +
> + JNLPFile jnlp = this.getJNLPFile();
> + URL url = jnlp.getFileLocation();
> + File jn = null;
> +
> + if (url.getProtocol().equals("file")) // If the file is on the local file system, use original path, otherwise find cache file
> + jn = new File(url.getPath());
> + else
> + jn = CacheUtil.getCacheFile(url, null);
> +
> + FileReader jnlpReader = new FileReader(jn);
> + JNLPVerify verify;
> +
> + if (jeName.equals(application)) {
> +
> + if (JNLPRuntime.isDebug())
> + System.out
> + .println("\tAPPLICATION.JNLP has been located within signed JAR. Starting verfication...");
> +
> + try {
> + verify = JNLPVerify.getInstanceOf(inputReader,
> + jnlpReader, false);
> + if (!verify.compare())
> + throw new JNLPVerifyException(
> + "Signed APPLICATION did not match launching JNLP File");
> + if (JNLPRuntime.isDebug())
> + System.out
> + .println("\t** Application Verification Successful **");
> + break; //break while loop
> + } catch (Exception e) {
> + throw new JNLPVerifyException(e.getMessage());
Instead new Exception(String), for sure use new Exception (Throwable cause) or new Exception (String message, Throwable cause).
> + }
> + }
> +
> + if (jeName.equals(template)) {
> +
> + if (JNLPRuntime.isDebug())
> + System.out
> + .println("\tAPPLICATION_TEMPLATE.JNLP has been located within signed JAR. Starting verfication...");
> +
> + try {
> + verify = JNLPVerify.getInstanceOf(inputReader,
> + jnlpReader, true);
> +
> + if (!verify.compare())
> + throw new JNLPVerifyException(
> + "Signed APPLICATION_TEMPLATE did not match launching JNLP File");
> + if (JNLPRuntime.isDebug())
> + System.out
> + .println("\t** Template Verification Successful **");
> + break; //break while loop
> + } catch (Exception e) {
> + throw new JNLPVerifyException(e.getMessage());
Instead new Exception(String), .... as above;) and al over code:)
> + }
> + }
> + }
> + }
> + } catch (IOException e) { //'new JarFile(localFile)' throws an IOException
> + e.printStackTrace();
Is there really some reason why to consume this exception?
> + }
> + }
> + }
> + } catch (JNLPVerifyException e) { //Throw e if signed JNLP file failed to be verified
> + throw e;
Why this?
> +
> + } catch (Exception e) {
> + e.printStackTrace();
> + }
Again -Is there really some reason why to consume this exception? e.print..() is not an reaction to exception. It is escape;)
> + }
> + if (JNLPRuntime.isDebug())
> + System.out.println("ENDING check for signed JNLP file...");
> + }
> }
>
> +
> for (JARDesc jarDesc : file.getResources().getJARs()) {
> try {
> File cachedFile = tracker.getCacheFile(jarDesc.getLocation());
> @@ -1460,3 +1587,13 @@
> }
> }
> }
> +
> +class JNLPVerifyException extends Exception
> +{
> + private static final long serialVersionUID = 1L;
> +
> + JNLPVerifyException(String message)
> + {
> + super(message);
> + }
> +}
I think it is worhy new file as this is public exception. And it should be in package with JNLP verifier implementation.
> diff -r e0741a8c44b6 netx/net/sourceforge/nanoxml/XMLElement.java
> --- a/netx/net/sourceforge/nanoxml/XMLElement.java Tue Jun 14 13:30:55 2011 -0400
> +++ b/netx/net/sourceforge/nanoxml/XMLElement.java Thu Jun 23 15:57:15 2011 -0400
> @@ -1166,7 +1166,7 @@
> * @param pout The PipedOutputStream that will be receiving the filtered
> * xml file.
> */
> - public void sanitizeInput(InputStreamReader isr, PipedOutputStream pout) {
> + public void sanitizeInput(Reader isr, PipedOutputStream pout) {
> try {
> PrintStream out = new PrintStream(pout);
>
Nice catch^
Ragrads J.
More information about the distro-pkg-dev
mailing list