[fyi][icedtea-web] backend and itw-settings for extended applets security
Jiri Vanek
jvanek at redhat.com
Wed Feb 20 04:47:12 PST 2013
On 02/14/2013 07:19 PM, Adam Domurad wrote:
> 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'.
>
>>
Ok, fixed - Disable running of all java applets
As java *will* launch, but no appelt will be launched.
To forbid java utterly we should touch the plugin c code, what I'm against, and I would rather let
this to browsers' configuration (like chrome's outdated plugins)
>> 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.
ok. I made the list coma separated now in internal form. Thanx for catch!
>
> 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.
*Should* be fixed
>
>
>>
>> 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.
Ok. let only your impl in.
>
>
>>
>> >> 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.
ok, extracted, although I doubt advantage.
>
>> > 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.
done, sorry!
>
>> + 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.
Omair still need to say his opinions, but I would like to encourage you to DO NOT wait. Work on tof
of this, I hope there will be no more dramatic changes (and you can find some more flaws in
LockedFile impl)
J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: doubleReviewedIntegrationOfExtendedSecurityToITW-round5.diff
Type: text/x-patch
Size: 124131 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130220/0a060dff/doubleReviewedIntegrationOfExtendedSecurityToITW-round5.diff
More information about the distro-pkg-dev
mailing list