[fyi][icedtea-web] backend and itw-settings for extended applets security

Omair Majid omajid at redhat.com
Tue Feb 5 15:22:14 PST 2013


Hi Jiri,

On 02/03/2013 07:08 AM, Jiri Vanek wrote:
> Whole troubles are described here
> http://icedtea.classpath.org/wiki/Extended_Applets_Security rather
> then in plain email.

Thanks for adding this to the wiki. I have a few thoughts and some
questions.

Would it be clearer if "Extra High Security" was renamed to "Disable All
Applets"?

For matching, I think it might be good to follow the same rule that is
used to decide whether to share classloaders between applets.

What's the motivation behind keeping codebase and document-bases as regexes?

> There is what me and adam have already achieved on this topic, what we
> have o achieve and where we are stil not so sure....
> 
> This patch is containing itw-settings part and backend for manipulating
> stored entries.and basic matching.
> I noted this just [fyi] because adam ave already walked through the
> patch, and so have I across his part. So the patch should be ready for
> push unless someone find something malicious on
> implementation or even on design.

Some comments/questions and suggestions below.

> diff -r e631770d76ba netx/net/sourceforge/appletextendedsecurity/AppletExtendedSecurity.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/appletextendedsecurity/AppletExtendedSecurity.java	Fri Feb 01 20:55:48 2013 +0100

Hm... I would have expected this to be for the plugin. Why netx?

If it's in netx, why not the existing net/sourceforge/jnlp/security package?

> @@ -0,0 +1,84 @@
> +/*
> +Copyright (C) 2013 Red Hat
> +
> +This program is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 2 of the License, or
> +(at your option) any later version.
> +
> +This program is distributed in the hope that it will be useful, but
> +WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with this program; if not, write to the Free Software
> +Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +*/

We generally use GPL+Classpath exception license (see [1] for an
example). Is there a specific reason for using the GPL? This same issue
is with other classes too.

> +public class AppletExtendedSecurity {

To me this name sounds like it extends another class (the existing
AppletSecurity class, even). Maybe call it something else, like
AppletStartupSecuritySettings? (not a great name either, but slightly
more descriptive)

Any reason for making this class mostly static? I would prefer it if was
instantiated. Maybe JNLPRuntime can be used to obtain an instance of this?

> +    public static AppletSecurityLevel getDefaultSecurityLevel(){
> +        return AppletSecurityLevel.getDefault();
> +    }

I am trying to see what is the advantage of this method, but I am having
trouble.

> +    /**
> +     *
> +     * @return user-set seurity level or default one if user-set do not exists
> +     */
> +    public static AppletSecurityLevel getCustomSecurityLevel(){
> +        DeploymentConfiguration conf = JNLPRuntime.getConfiguration();
> +        if (conf==null){
> +            conf = new DeploymentConfiguration();
> +            try {
> +            conf.load();
> +            } catch (ConfigurationException ex){
> +                ex.printStackTrace();
> +                return getDefaultSecurityLevel();
> +            }
> +        }

I think it would be much more robust if this method threw an exception
instead of instantiating a new DeploymentConfiguration object. I would
so far as to require a DeploymentConfiguration parameter in the constructor.

> +        String s = conf.getProperty(DeploymentConfiguration.KEY_SECURITY_LEVEL);
> +        if (s==null) {
> +            return getDefaultSecurityLevel();
> +        }
> +        return AppletSecurityLevel.fromString(s);
> +    }

There are two methods that use the word 'custom' in the name. But moth
mean different things.

I would prefer it if both methods were renamed (custom is not a very
descriptive term):

getUnsignedAppletActionCustomStorage -> getUnsignedAppletActionUserStorage

getCustomSecurityLevel -> getCurrentSecurityLevel

> diff -r e631770d76ba netx/net/sourceforge/appletextendedsecurity/AppletSecurityLevel.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/appletextendedsecurity/AppletSecurityLevel.java	Fri Feb 01 20:55:48 2013 +0100

> +public enum AppletSecurityLevel {
> +
> +    DENY_ALL, DENY_UNSIGNED, ASK_UNSIGNED, ALLOW_UNSIGNED;
> +
> +    public static String allToString() {
> +        return DENY_ALL.toChars()+" "+DENY_UNSIGNED.toChars()+" "+ASK_UNSIGNED.toChars()+" "+ALLOW_UNSIGNED.toChars();
> +    }

If you just need this for debugging, try:
Arrays.toString(AppletSecurityLevel.values())

And you wont have to change this code if you add another enum value.

> +    public String toChars() {
> +        switch (this) {
> +            case DENY_ALL:
> +                return " DENY_ALL";
> +            case DENY_UNSIGNED:
> +                return "DENY_UNSIGNED";
> +            case ASK_UNSIGNED:
> +                return "ASK_UNSIGNED";
> +            case ALLOW_UNSIGNED:
> +                return "ALLOW_UNSIGNED";
> +        }
> +        throw new RuntimeException("Unknown AppletSecurityLevel");
> +    }

I think you can replace this with the name() method of the enum (unless
you really want the space in the beginning of DENY_ALL):

DENY_ALL.name().

> +    public static AppletSecurityLevel fromString(String s) {
> +        if (s.trim().equalsIgnoreCase("DENY_ALL")) {
> +            return AppletSecurityLevel.DENY_ALL;
> +        } else if (s.trim().equalsIgnoreCase("DENY_UNSIGNED")) {
> +            return AppletSecurityLevel.DENY_UNSIGNED;
> +        } else if (s.trim().equalsIgnoreCase("ASK_UNSIGNED")) {
> +            return AppletSecurityLevel.ASK_UNSIGNED;
> +        } else if (s.trim().equalsIgnoreCase("ALLOW_UNSIGNED")) {
> +            return AppletSecurityLevel.ALLOW_UNSIGNED;
> +        } else {
> +            throw new RuntimeException("Unknown AppletSecurityLevel for " + s);
> +        }
> +    }

AppletSecurityLevel.valueOf(String.toUpperCase()) does what fromString
does, I think.

> diff -r e631770d76ba netx/net/sourceforge/appletextendedsecurity/UnsignedAppletAction.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/appletextendedsecurity/UnsignedAppletAction.java	Fri Feb 01 20:55:48 2013 +0100

> +public enum UnsignedAppletAction {

Maybe call it ExecuteUnsignedApplet?

> +    ALWAYS, NEVER, YES, NO;
> +
> +    public String toChar() {
> +        switch (this) {
> +            case ALWAYS:
> +                return "A";
> +            case NEVER:
> +                return "N";
> +            case YES:
> +                return "y";
> +            case NO:
> +                return "n";
> +        }
> +        throw new RuntimeException("Unknown UnsignedAppletAction");
> +    }
> +

I think name() already does this.

> +    public String toExplanation() {
> +        switch (this) {
> +            case ALWAYS:
> +                return Translator.R("APPEXTSECunsignedAppletActionAlways");
> +            case NEVER:
> +                return Translator.R("APPEXTSECunsignedAppletActionNever");
> +            case YES:
> +                return Translator.R("APPEXTSECunsignedAppletActionYes");
> +            case NO:
> +                return Translator.R("APPEXTSECunsignedAppletActionNo");
> +        }
> +        throw new RuntimeException("Unknown UnsignedAppletAction");
> +    }

Do you expect the explanation to be visible to the user? Maybe it
belongs with the rest of the UI, not in here with the logic. Just
something to think about.

> +    public static UnsignedAppletAction fromString(String s) {
> +        if (s.startsWith("A")) {
> +            return UnsignedAppletAction.ALWAYS;
> +        } else if (s.startsWith("N")) {
> +            return UnsignedAppletAction.NEVER;
> +        } else if (s.startsWith("y")) {
> +            return UnsignedAppletAction.YES;
> +        } else if (s.startsWith("n")) {
> +            return UnsignedAppletAction.NO;
> +        } else {
> +            throw new RuntimeException("Unknown UnsignedAppletAction for " + s);
> +        }
> +    }

Maybe use Enum.valueOf() can replace this fromString method.

> diff -r e631770d76ba netx/net/sourceforge/appletextendedsecurity/UnsignedAppletActionEntry.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/appletextendedsecurity/UnsignedAppletActionEntry.java	Fri Feb 01 20:55:48 2013 +0100

> +    @Override
> +    public String toString() {
> +        return unsignedAppletAction.toChar() +
> +                " " + ((timeStamp == null)?"1":timeStamp.getTime()) +
> +                " " + ((documentBase == null)?"":documentBase.getRegEx()) +
> +                " " + ((codeBase == null)?"":codeBase.getRegEx()) +
> +                " " + ((mainClass == null)?"":mainClass) +
> +                " " + createArchivesString(archives);
> +
> +    }
> +
> +    public void write(Writer bw) throws IOException {
> +        bw.write(this.toString());
> +    }

This may not be a good idea. toString() is generally meant for humans to
read, not for machines:

     * Returns a string representation of the object. In general, the
     * {@code toString} method returns a string that
     * "textually represents" this object. The result should
     * be a concise but informative representation that is easy for a
     * person to read.

I would encourage you to serialize the data for writing to a file in
write(), but only serialize it for debugging/display in toString().

> +    public  static String createArchivesString(List<String> listOfArchives) {
> +        if (listOfArchives == null){
> +            return "";
> +        }
> +        StringBuilder sb = new StringBuilder();
> +        for (int i = 0; i < listOfArchives.size(); i++) {
> +            String string = listOfArchives.get(i);

As a sanity check, maybe you can check that the input string does not
contain any semicolons? I ran into a URL last week that had a semicolon :(

> +            if (string.trim().isEmpty()){
> +                continue;
> +            }
> +            sb.append(string).append(";");
> +        }
> +        return sb.toString();


> diff -r e631770d76ba netx/net/sourceforge/appletextendedsecurity/UnsignedAppletActionStorage.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/appletextendedsecurity/UnsignedAppletActionStorage.java	Fri Feb 01 20:55:48 2013 +0100
> @@ -0,0 +1,106 @@
> +/*
> +Copyright (C) 2013 Red Hat
> +
> +This program is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 2 of the License, or
> +(at your option) any later version.
> +
> +This program is distributed in the hope that it will be useful, but
> +WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with this program; if not, write to the Free Software
> +Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +*//*
> +Copyright (C) 2013 Red Hat
> +
> +This program is free software; you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 2 of the License, or
> +(at your option) any later version.
> +
> +This program is distributed in the hope that it will be useful, but
> +WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with this program; if not, write to the Free Software
> +Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +*/

This file has 2 licenses.

> +/**
> + *
> + * @author jvanek
> + */

Javadocs on what this class does would be nice to have.

> diff -r e631770d76ba netx/net/sourceforge/appletextendedsecurity/UrlRegEx.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/appletextendedsecurity/UrlRegEx.java	Fri Feb 01 20:55:48 2013 +0100

> +public class UrlRegEx {

I don't follow the motivation for having this class.

> +    public String getFilteredRegEx() {
> +        return regEx.replaceAll("\\\\Q", "").replaceAll("\\\\E", "");
> +    }

Uh.. do you expect someone to use quote things in the strings? IMHO, if
you are exposing a "regex" to the users, don't try and
sanitize/modify/"fix" it.

> diff -r e631770d76ba netx/net/sourceforge/appletextendedsecurity/impl/UnsignedAppletActionStorageOperator.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/appletextendedsecurity/impl/UnsignedAppletActionStorageOperator.java	Fri Feb 01 20:55:48 2013 +0100

> +public class UnsignedAppletActionStorageOperator extends UnsignedAppletActionStorageImpl {

The name "operator" at the end of the name makes it sound like it should
be using UnsignedAppletActionStorage, not extending it.

> diff -r e631770d76ba netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java
> --- a/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java	Thu Jan 31 11:12:35 2013 +0100
> +++ b/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java	Fri Feb 01 20:55:48 2013 +0100


> +    public static File getAppletTrustCustomSettingsPath() {
> +        return new File(System.getProperty("user.home") + File.separator + DEPLOYMENT_DIR
> +                + File.separator + APPLET_TRUST_SETTINGS);
> +    }

Please avoid 'custom'. It's not very indicative of the purpose. I think
"User" might be more apporpriate than Custom.

> +     public static File getAppletTrustGlobalSettingsPath() {
> +       return new File(File.separator + "etc" + File.separator + ".java" + File.separator
> +                + "deployment" + File.separator + APPLET_TRUST_SETTINGS);
> +        
> +    }
> +
>      /**
>       * Initialize this deployment configuration by reading configuration files.
>       * Generally, it will try to continue and ignore errors it finds (such as file not found).

> diff -r e631770d76ba netx/net/sourceforge/jnlp/controlpanel/UnsignedAppletsTrustingListPanel.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/controlpanel/UnsignedAppletsTrustingListPanel.java	Fri Feb 01 20:55:48 2013 +0100


> +        jButton1.setText(Translator.R("APPEXTSECguiPanelHelpButton"));
> +        jButton1.addActionListener(new java.awt.event.ActionListener() {
> +            @Override
> +            public void actionPerformed(java.awt.event.ActionEvent evt) {
> +                jButton1ActionPerformed(evt);
> +            }
> +        });

Uh.. can you better names than "jButton#" ?

> diff -r e631770d76ba netx/net/sourceforge/jnlp/util/lockingfile/LockingFile.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/util/lockingfile/LockingFile.java	Fri Feb 01 20:55:48 2013 +0100

> +public class LockingFile {

Maybe call it LockedFile?

> +    private LockingFile(File file) {
> +        this.file = file;
> +        try{
> +            //just try to ctreate
> +            this.file.createNewFile();
> +        }catch(Exception ex){
> +            //intentionaly silent

Are you sure about the semantics here? Maybe we can chek if the file
exists and is writiable before doing this?

> +    // Provide shared access to LockedFile's via weak map
> +    static private final Map<File, LockingFile> instanceCache = new WeakHashMap<File, LockingFile>();
> +
> +    /**
> +     * Get a LockingFile for a given File.
> +     * Ensures that we share the same instance for all threads
> +     * @param file the file to lock
> +     * @return a LockingFile instance
> +     */
> +    synchronized public static LockingFile getInstance(File file) {
> +        if (!instanceCache.containsKey(file)) {
> +            instanceCache.put(file, new LockingFile(file));
> +        }
> +
> +        return instanceCache.get(file);
> +    }

Nice!

> diff -r e631770d76ba netx/net/sourceforge/jnlp/util/lockingfile/LockingReaderWriter.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/util/lockingfile/LockingReaderWriter.java	Fri Feb 01 20:55:48 2013 +0100

> +public abstract class LockingReaderWriter {

> +    private LockingFile lockedFile;

I am going to point to as support for my argument that LockingFile
should be renamed to LockedFile :)

> diff -r e631770d76ba netx/net/sourceforge/jnlp/util/lockingfile/StorageIoException.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/netx/net/sourceforge/jnlp/util/lockingfile/StorageIoException.java	Fri Feb 01 20:55:48 2013 +0100
> @@ -0,0 +1,22 @@

Please add a license header.

> +package net.sourceforge.jnlp.util.lockingfile;
> +
> +/**
> + * Thrown when an exception occurs using the storage (namely IOException)
> + */
> +public class StorageIoException extends RuntimeException {


> diff -r e631770d76ba tests/netx/unit/net/sourceforge/jnlp/util/lockingfile/LockingStringListStorageTest.java
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/netx/unit/net/sourceforge/jnlp/util/lockingfile/LockingStringListStorageTest.java	Fri Feb 01 20:55:48 2013 +0100

> +public  class LockingStringListStorageTest {

Copy-paste mistake, I hope.

> +public static class LockingStringListStorage extends LockingReaderWriter {
> +
> +    private List<String> cachedContents = new ArrayList<String>();
> +
> +    //To sutisfy testengine, void constructor and dummy testmethod
> +    public LockingStringListStorage() throws IOException {
> +        this(createTmpBackend());
> +    }

Please dont do this! Someone will end up using this constructor. I
assume this is for unit tests? Just have the unit test itself create a
temp file and pass it along to the real constructor.

> +    @Test
> +    public void lockingStringListStorageCanBeInstantiated(){
> +        Assert.assertNotNull(this);
> +    }

Please move tests to separate files.

> +
> +    private static File createTmpBackend() throws IOException{
> +        File f = File.createTempFile("forTests","emptyConstructor");
> +        f.deleteOnExit();
> +        return f;
> +    }

Please dont do this!

> +    /**
> +     * Create locking file-backed storage.
> +     * @param file the storage file
> +     */
> +    public LockingStringListStorage(File file) {
> +        super(file);
> +    }
> +
> +    /**
> +     *  Get the underlying string list cache. Should lock
> +     *  before using.
> +     *  @return the cache
> +     */
> +    final protected List<String> getCachedContents() {
> +        return cachedContents;
> +    }
> +
> +    @Override
> +    public void writeContent(BufferedWriter writer) throws IOException {
> +        for (String string : cachedContents) {
> +            writer.write(string);
> +            writer.newLine();
> +        }
> +    }
> +
> +    @Override
> +    protected void readLine(String line) {
> +        this.cachedContents.add(line);
> +    }
> +
> +    @Override
> +    protected void readContents() throws IOException {
> +        cachedContents.clear();
> +        super.readContents();
> +    }

I am not a huge fan of this. The javadoc for the parent says "read
contents from file", but nothing bout refresing. This class overrides it
and clears the old contents. If an instance of this class is returned as
a LockingReaderWriter, all sorts of glitches can happen because of the
undexpected clear() call.

This method should probably be called reloadContents() or something.

My suggestions is to remove the inheritence completely and make
LockingStringListStorage use a LockingReaderWriter instead.

> +    /**
> +     * Removes the first occurrence of the specified element from this list,
> +     * if it is present (optional operation).  If this list does not contain
> +     * the element, it is unchanged.
> +     *
> +     * @param line string to be removed from this list, if present
> +     * @return <tt>true</tt> if the storage contained the line
> +     */
> +    synchronized public boolean remove(String line) {
> +        boolean didRemove;
> +
> +        lock();
> +        try {
> +            readContents();
> +            didRemove = cachedContents.remove(line);
> +            writeContents();
> +        } catch (IOException e) {
> +            throw new StorageIoException(e);
> +        } finally {
> +            unlock();
> +        }
> +
> +        return didRemove;
> +    }
> +}


I hope there was a mistake in the patch and the test code below goes in
separate classes.

> +    private static File storagefile;
> +
> +    private static LockingStringListStorage newInstance() {
> +        return new LockingStringListStorage(storagefile);
> +    }
> +
> +    @Before
> +    public void setUp() throws IOException {
> +        storagefile = File.createTempFile("foo", "bar");
> +    }



[1]
http://icedtea.classpath.org/hg/icedtea-web/file/fd01cd1c2bbc/netx/net/sourceforge/jnlp/browser/BrowserAwareProxySelector.java#l1

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681



More information about the distro-pkg-dev mailing list