[fyi][icedtea-web] backend and itw-settings for extended applets security
Adam Domurad
adomurad at redhat.com
Thu Feb 14 10:19:20 PST 2013
Thanks for the update! -- and sorry that you had to endure a
double-review on an [fyi] :-(
Tried to keep it as brief as possible this time.
> Adam recently wrote this sentence: IMO Extra High Security is better
> here as simply 'Disable Applets'. It is a bit odd to say you're
> securing Java by turning it off.
>
> I'm not sure if he mean the same or oposite:)
I believe Omair and me here are in agreement :-) Extra high security is
a simple checkbox in Oracle's plugin that says 'Enable Java in the
Browser'. I think people looking for this setting will not readily
equate it to 'Extra High security'.
>
> My vote is for Extra High Security - fact that it disables runtime of
> all applets (not JVM at all) is in easily accessible description of
> the item.
>
>>
>> For matching, I think it might be good to follow the same rule that is
>> used to decide whether to share classloaders between applets.
This is the default when user hits accept + remember option.
>
>>> 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?
Does seem like it it should be moved from netx, but I think there are
some issues with accessing plugin classes? OK for now, it's not JNLP
related anyhow.
Maybe though netx/net/sourceforge/applets/security.
>>
>>> +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)
>
> Adam's name :P
> Now it is your name :)
icedtea-web belongs to all of us :-)
>>
>>> 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?
>
> Adam's aname again :P
> Now it will be yours name :P
>>
>>> + 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.
>
> I would rather stay with the one char convention here
Still think yes/no is not needed after all. It complicates things a lot,
since its user-visible. But OK.
>>
>>> + 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 :(
>
> Why? I was living in universe when archives will bne delivered as
> relative paths - jar1.jar;jar2.jar;some/hidden.jar
>
> But if there is supposed to be whole url of jar, than this wil need
> rethinking and do need Adams opinions also here
>
>
> To bad discover that my universe was the wrong one :)
It is relative paths only for sure, but note -- archive tag is comma
separated.
Technically, you *can* name a jar 'a.jar;b.jar' to try and fake a match
to an existing entry, and maybe eg host two applets, one legit that user
accepts, another that dupes the archive tag and is also accepted. The
defensive option in this case is to swap semi-colons for the
impossible-to-have comma character.
>>
>>> 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.
>
> :(( ooook
+1 on Custom->User.
> > 2) I have attached my patch, although take note of the following:
> > - I had to fix an issue where applets without any jars said they
> were 'signing', not showing the
> > dialogue. Note though that they were still not given any
> permissions, so there was no security hole.
> > - Your LockingFile as-was did not recursively lock correctly. I had
> to fix it so I could recursively
> > lock to make sure that updating the dialogue is an atomic operation.
> However I still prefer my
> > refactoring of Locking File.
>
> Your version was not handling some existence/permissions variants, I
> have kept my original version
> But during reading of your refactored one I have not not strumbled
> across issue you are mentioning.
+ if (file.exists()){
+ this.randomAccessFile = new RandomAccessFile(this.file, rw);
+ this.fileChannel = randomAccessFile.getChannel();
+ }
+
+ if (!isReadOnly()) this.processLock = this.fileChannel.lock();
These actions are strongly related and should not have been separated,
this caused problems.
>
> Can you please in your nextr iterationpoint only to the corrupted part
> and do not do any more refactoring?
>
> > -Even still it seems like multiple applet entries can sneak their
> way in, making it impossible to
> > properly update the stored action (because simple 'yes/no's are
> stored...). Might need to put the
> > most recent decision at the top of the list, or match starting from
> the bottom, just in case
> > duplicates do sneak in. I am in favour though of dropping simple
> yes/no's being stored.
>
> Well we have agreed ot add it, and we can agree to remove it. But I
> still am in favour to keep it And actually it can help with the issue
> you are solving.
>
> When first applet will show dialogues, others should not react shown
> it if they match definition this appelt have added/updated
Except this is permanent. More appropriate in this case would be a
simple eg hashmap.
>
> [...]
>
> I thought we are already doing this. Jsut with exception, when first
> shown dialogue creates an rule some other applet is matching, then it
> do not show the rule.
>
> This is vlaid especially for appelts with shared classlaoder, because
> they ALWAYS match rule saved by dialogue of first of them.
>
I do this once for every shared classloader. The other option (probably
OK for backporting) is to force all unsigned applet checks to be
synchronized (and thus have the behaviour you said) Oracle just hits you
with like 20 pop-ups :-)
>
> > The table is looking quite nice.
> >
>
> Thanx! first nice words to this patch :) ..?..:(
Sorry that patch review is so focused on pulling things apart :-/ It's
all great work, and thanks for putting up with so much disagreement.
>
> > I have attached a preferred iteration of my test.
>
> Unable to thing anymore today, I have just added this test to patch as
> "...Test2"
> But they seems to me nearly completely same... Just with moreover
> cosmetich changes. Why this?
Cosmetic changes that I prefer :-) Clearer names etc.
>
> >> import net.sourceforge.jnlp.runtime.JNLPProxySelector;
> >> @@ -384,6 +385,41 @@
> >> DeploymentConfiguration.KEY_PLUGIN_JVM_ARGUMENTS,
> >> null,
> >> null
> >> + },
> >> + //unsigned applet security level
> >> + {
> >
> > Why are you not just using a set of strings with a string validator
> ? I do not like the idea of
> > having an anonymous class here.
> >
> >> + DeploymentConfiguration.KEY_SECURITY_LEVEL,
> >> + new ValueVa
>
>
> Disagree - the anonymous class is good enough. If you really insists I
> will make regular validator class, but I'm not going to change the way
> how it is done.
+1 on regular class.
> > This silent failure is not acceptable IMO! It ignores the intent of
> the method completely, and it
> > was causing me a lot of confusion when I was playing with the code.
> We ought to error-out here.
> >
> > Same here, we shouldn't be backed by an invalid file in the first
> place.
> > Note that I believe this will cause problems with how you are
> currently handling non-existant global
> > configurations -- but that needs a more robust solution than this
> anyway.
>
>
> yah, this is serious But I think that we *should* handle not existing
> file. But I'mnotsure how to do it properly == where to catch this.
> For now I woul dlike to keep this as it is, because it is quite
> important functionality, and you need to work on top of it. Anyway I
> will think about it and try to fix it (because I really agree that
> this silent ignoration is the worst evil)
I think this should error-out, and not be handled. The highest levels of
the algorithm (eg the interface display) should do a check for file
existence, and handle it accordingly.
>
>
>
> >> + }
> >> +
> >> +}
> >
> > In conclusion, nits are nits, but I really want to see this in HEAD
> :-)) Great work on the
> > icedtea-web-settings parts.
> >
> > Happy hacking,
> > -Adam
>
>
> I needed some positive sentence at the end.
>
>
> Guys, this was really hard to zigzag and fullfill both yours reviews
> but it hsould be done now.
> Tis was actually the longest reply to review I have ever done,
> espcially because there were three fully-scramming voices. I hope I
> have not forget to reply/fix/explain some nit, but it was really long
> reading
I am not surprised, and sorry :-(
We should have taken it to the public mailing list immediately, and
maybe kept a separate repo that we pushed immediately to (easier than us
constantly applying the patches).
>
> ps: Adam, you really MUST to snip your replies. To seek few chars of
> nit between hundreds of lines (of unrelated lines!) is not possible.
> Especially after longer time, or during such a mixed review... it is
> also protection to get some importatn stuff lost.
I apologise!
>
>
>
Only comments otherwise:
> + javax.swing.GroupLayout layout = new
> javax.swing.GroupLayout(this);
> + this.setLayout(layout);
> + layout.setHorizontalGroup(
> +
> layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING).addGroup(javax.swing.GroupLayout.Alignment.TRAILING,
> layout.createSequentialGroup().addContainerGap().addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.TRAILING).addComponent(jTabPane1,
> javax.swing.GroupLayout.Alignment.LEADING,
> javax.swing.GroupLayout.DEFAULT_SIZE, 583,
> Short.MAX_VALUE).addComponent(globalBehaviourLabel,
> javax.swing.GroupLayout.Alignment.LEADING).addGroup(javax.swing.GroupLayout.Alignment.LEADING,
> layout.createSequentialGroup().addComponent(securityLevelLabel).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED).addComponent(mainPolicyComboBox,
> 0, 474,
> Short.MAX_VALUE)).addGroup(javax.swing.GroupLayout.Alignment.LEADING,
> layout.createSequentialGroup().addComponent(addRowButton).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.UNRELATED).addComponent(validateTableButton).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.UNRELATED).addComponent(testUrlButton).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED,
> 94,
> Short.MAX_VALUE).addComponent(moveRowDownButton).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED).addComponent(moveRowUpButton)).addGroup(layout.createSequentialGroup().addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING).addGroup(layout.createSequentialGroup().addComponent(deleteButton).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED).addComponent(deleteTypeComboBox,
> javax.swing.GroupLayout.PREFERRED_SIZE,
> javax.swing.GroupLayout.DEFAULT_SIZE,
> javax.swing.GroupLayout.PREFERRED_SIZE).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED).addComponent(invertSelectionButton)).addGroup(layout.createSequentialGroup().addComponent(askBeforeActionCheckBox).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED).addComponent(filterRegexesCheckBox))).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED,
> 93, Short.MAX_VALUE).addComponent(helpButton,
> javax.swing.GroupLayout.PREFERRED_SIZE, 108,
> javax.swing.GroupLayout.PREFERRED_SIZE))).addContainerGap()));
> + layout.setVerticalGroup(
> +
> layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING).addGroup(layout.createSequentialGroup().addContainerGap().addComponent(globalBehaviourLabel).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED).addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING).addComponent(securityLevelLabel).addComponent(mainPolicyComboBox,
> javax.swing.GroupLayout.PREFERRED_SIZE,
> javax.swing.GroupLayout.DEFAULT_SIZE,
> javax.swing.GroupLayout.PREFERRED_SIZE)).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED).addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.TRAILING).addGroup(layout.createSequentialGroup().addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING,
> false).addComponent(deleteButton).addComponent(deleteTypeComboBox).addComponent(invertSelectionButton,
> javax.swing.GroupLayout.DEFAULT_SIZE,
> javax.swing.GroupLayout.DEFAULT_SIZE,
> Short.MAX_VALUE)).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED).addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.BASELINE).addComponent(askBeforeActionCheckBox).addComponent(filterRegexesCheckBox))).addComponent(helpButton,
> javax.swing.GroupLayout.PREFERRED_SIZE, 53,
> javax.swing.GroupLayout.PREFERRED_SIZE)).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.UNRELATED).addComponent(jTabPane1,
> javax.swing.GroupLayout.DEFAULT_SIZE, 161,
> Short.MAX_VALUE).addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED).addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.BASELINE).addComponent(addRowButton).addComponent(validateTableButton).addComponent(testUrlButton).addComponent(moveRowUpButton).addComponent(moveRowDownButton)).addContainerGap()));
> +
Please place this on multiple lines.
> + JPanel pane1 = new JPanel(new BorderLayout());
> + JPanel pane2 = new JPanel(new BorderLayout());
> + pane1.add(userTableScrollPane);
> + pane2.add(globalTableScrollPane);
I am in favour of getting this into HEAD after 1. fixing lock file as I
mentioned, 2. the spacing fix. You do not need to post again if you
don't feel you made any other substantial changes.
I need *somewhere* to get the latest version of this reliably, even if
its a separate repo.
Thanks for sticking with it!
-Adam
More information about the distro-pkg-dev
mailing list