[icedtea-web] RFC: Patch to fix PR794 (Class-Path element processing)
Deepak Bhole
dbhole at redhat.com
Wed Sep 28 11:57:19 PDT 2011
* Omair Majid <omajid at redhat.com> [2011-09-27 21:07]:
> On 09/27/2011 06:07 PM, Deepak Bhole wrote:
> >Hi,
> >
> >Attached patch fixes PR794:
> >http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=794
> >
> >ChangeLog:
> >2011-09-27 Deepak Bhole<dbhole at redhat.com>
> >
> > PR794: IcedTea-Web does not work if a Web Start app jar has a Class-Path
> > element in the manifest.
> > * netx/net/sourceforge/jnlp/runtime/CachedJarFileCallback.java
> > (retrieve): Blank out the Class-Path elements in manifest.
> > * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> > (activateJars): Only load Class-Path elements if this is an applet. Add a
> > security mapping for jars from the Class-Path.
> >
> >Okay for HEAD and 1.1?
> >
>
> While the overall idea looks fine to me, I have a few concerns noted
> inline below.
>
> >diff -r 0a1733685325 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> >--- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Fri Sep 23 12:14:39 2011 -0400
> >+++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Tue Sep 27 18:02:12 2011 -0400
> >@@ -779,7 +778,15 @@
> >
> > JarFile jarFile = new JarFile(localFile.getAbsolutePath());
> > Manifest mf = jarFile.getManifest();
> >- classpaths.addAll(getClassPathsFromManifest(mf, jar.getLocation().getPath()));
> >+
> >+ if (file instanceof PluginBridge) {
> >+ for (String classpath: getClassPathsFromManifest(mf, jar.getLocation().getPath())) {
> >+ URL codebaseURL = file.getCodeBase();
> >+ jarLocationSecurityMap.put(new URL(codebaseURL.getProtocol() + "://" + codebaseURL.getHost() + classpath), jarSecurity);
>
> I have a concern about this line here ^
>
> Is this jar being verified before we grant it permissions? It's not
> obvious from the patch that we are.
>
Ah, I thought addNewJar was already doing it.
New patch attached. It adds the right permissions and does verification.
> Also, a nitpick: we are ignoring any possible port numbers in
> codebaseURL. Other code in this class does something along the lines
> of:
>
> new URL(codebaseURL, classpath)
>
Fixed.
New ChangeLog:
2011-09-27 Deepak Bhole <dbhole at redhat.com>
PR794: IcedTea-Web does not work if a Web Start app jar has a Class-Path
element in the manifest.
* netx/net/sourceforge/jnlp/runtime/CachedJarFileCallback.java
(retrieve): Blank out the Class-Path elements in manifest.
* netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
(activateJars): Only load Class-Path elements if this is an applet.
(addNewJar): Add the right permissions for the cached jar file and verify
signatures.
Thanks,
Deepak
-------------- next part --------------
diff -r 0a1733685325 NEWS
--- a/NEWS Fri Sep 23 12:14:39 2011 -0400
+++ b/NEWS Wed Sep 28 14:53:24 2011 -0400
@@ -11,6 +11,8 @@
New in release 1.1.3 (2011-XX-XX):
* Plugin
- PR782: Support building against npapi-sdk as well
+* Common
+ - PR794: IcedTea-Web does not work if a Web Start app jar has a Class-Path element in the manifest
New in release 1.1.2 (2011-08-31):
* Plugin
diff -r 0a1733685325 netx/net/sourceforge/jnlp/runtime/CachedJarFileCallback.java
--- a/netx/net/sourceforge/jnlp/runtime/CachedJarFileCallback.java Fri Sep 23 12:14:39 2011 -0400
+++ b/netx/net/sourceforge/jnlp/runtime/CachedJarFileCallback.java Wed Sep 28 14:53:24 2011 -0400
@@ -94,7 +94,24 @@
if (UrlUtils.isLocalFile(localUrl)) {
// if it is known to us, just return the cached file
- return new JarFile(localUrl.getPath());
+ JarFile returnFile = new JarFile(localUrl.getPath());
+
+ try {
+
+ // Blank out the class-path because:
+ // 1) Web Start does not support it
+ // 2) For the plug-in, we want to cache files from class-path so we do it manually
+ returnFile.getManifest().getMainAttributes().putValue("Class-Path", "");
+
+ if (JNLPRuntime.isDebug()) {
+ System.err.println("Class-Path attribute cleared for " + returnFile.getName());
+ }
+
+ } catch (NullPointerException npe) {
+ // Discard NPE here. Maybe there was no manifest, maybe there were no attributes, etc.
+ }
+
+ return returnFile;
} else {
// throw new IllegalStateException("a non-local file in cache");
return null;
diff -r 0a1733685325 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
--- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Fri Sep 23 12:14:39 2011 -0400
+++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Wed Sep 28 14:53:24 2011 -0400
@@ -779,7 +779,11 @@
JarFile jarFile = new JarFile(localFile.getAbsolutePath());
Manifest mf = jarFile.getManifest();
- classpaths.addAll(getClassPathsFromManifest(mf, jar.getLocation().getPath()));
+
+ if (file instanceof PluginBridge) {
+ classpaths.addAll(getClassPathsFromManifest(mf, jar.getLocation().getPath()));
+ }
+
JarIndex index = JarIndex.getJarIndex(jarFile, null);
if (index != null)
jarIndexes.add(index);
@@ -1094,7 +1098,7 @@
* is downloaded.
* @param desc the JARDesc for the new jar
*/
- private void addNewJar(JARDesc desc) {
+ private void addNewJar(final JARDesc desc) {
available.add(desc);
@@ -1104,10 +1108,80 @@
JNLPRuntime.getDefaultUpdatePolicy()
);
- URL remoteURL = desc.getLocation();
- URL cachedUrl = tracker.getCacheURL(remoteURL);
- addURL(remoteURL);
- CachedJarFileCallback.getInstance().addMapping(remoteURL, cachedUrl);
+ // Give read permissions to the cached jar file
+ AccessController.doPrivileged(new PrivilegedAction<Void>() {
+ public Void run() {
+ Permission p = CacheUtil.getReadPermission(desc.getLocation(),
+ desc.getVersion());
+
+ resourcePermissions.add(p);
+
+ return null;
+ }
+ });
+
+ final URL remoteURL = desc.getLocation();
+ final URL cachedUrl = tracker.getCacheURL(remoteURL); // blocks till download
+
+ available.remove(desc); // Resource downloaded. Remove from available list.
+
+ try {
+
+ // Verify if needed
+
+ final JarSigner signer = new JarSigner();
+ final List<JARDesc> jars = new ArrayList<JARDesc>();
+ jars.add(desc);
+
+ // Decide what level of security this jar should have
+ // The verification and security setting functions rely on
+ // having AllPermissions as those actions normally happen
+ // during initialization. We therefore need to do those
+ // actions as privileged.
+
+ Exception verificationResult = AccessController.doPrivileged(new PrivilegedAction<Exception>() {
+ public Exception run() {
+ try {
+ signer.verifyJars(jars, tracker);
+
+ if (signer.anyJarsSigned() && !signer.getAlreadyTrustPublisher()) {
+ checkTrustWithUser(signer);
+ }
+
+ final SecurityDesc security;
+ if (signer.anyJarsSigned()) {
+ security = new SecurityDesc(file,
+ SecurityDesc.ALL_PERMISSIONS,
+ file.getCodeBase().getHost());
+ } else {
+ security = new SecurityDesc(file,
+ SecurityDesc.SANDBOX_PERMISSIONS,
+ file.getCodeBase().getHost());
+ }
+
+ jarLocationSecurityMap.put(remoteURL, security);
+
+ } catch (Exception e) {
+ return e;
+ }
+
+ return null;
+ }
+ });
+
+ if (verificationResult != null)
+ throw verificationResult;
+
+ addURL(remoteURL);
+ CachedJarFileCallback.getInstance().addMapping(remoteURL, cachedUrl);
+
+ } catch (Exception e) {
+ // Do nothing. This code is called by loadClass which cannot
+ // throw additional exceptions. So instead, just ignore it.
+ // Exception => jar will not get added to classpath, which will
+ // result in CNFE from loadClass.
+ e.printStackTrace();
+ }
}
/**
More information about the distro-pkg-dev
mailing list