New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK 8u102 changed JceSecurity#isRestricted to final #4101

Closed
kares opened this Issue Aug 22, 2016 · 19 comments

Comments

Projects
None yet
6 participants
@kares
Member

kares commented Aug 22, 2016

Environment

originally reported by @jkutner on the mailing list, the OpenJDK
javax.crypto.JceSecurity internals changed: http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/rev/666ddde3fecf

Expected Behavior

unlimited strength Java cryptography support out of the box (on OpenJDK derrivates)

Actual Behavior

limited cryptography strength - need to install Java Cryptography Extension (JCE) Unlimited Strength Jurisdiction Policy Files

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 22, 2016

Member

... and the naive patch would be :

diff --git a/core/src/main/java/org/jruby/Ruby.java b/core/src/main/java/org/jruby/Ruby.java
index 9f56537..1c06db5 100644
--- a/core/src/main/java/org/jruby/Ruby.java
+++ b/core/src/main/java/org/jruby/Ruby.java
@@ -170,6 +170,7 @@ import java.lang.invoke.MethodHandle;
 import java.lang.ref.WeakReference;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
 import java.net.BindException;
 import java.net.MalformedURLException;
 import java.net.URL;
@@ -1288,10 +1289,23 @@ public final class Ruby implements Constantizable {
         try {
             Class jceSecurity = Class.forName("javax.crypto.JceSecurity");
             Field isRestricted = jceSecurity.getDeclaredField("isRestricted");
-            isRestricted.setAccessible(true);
-            isRestricted.set(null, false);
-            isRestricted.setAccessible(false);
-        } catch (Exception e) {
+            if ( Boolean.TRUE.equals(isRestricted.get(null)) ) {
+                if ( Modifier.isFinal(isRestricted.getModifiers()) ) {
+                    Field modifiers = Field.class.getDeclaredField("modifiers");
+                    modifiers.setAccessible(true);
+                    modifiers.setInt(isRestricted, isRestricted.getModifiers() & ~Modifier.FINAL);
+                }
+                isRestricted.setAccessible(true);
+                isRestricted.setBoolean(null, false); // isRestricted = false;
+                isRestricted.setAccessible(false);
+            }
+        }
+        catch (ClassNotFoundException e) { // not an OpenJDK ~ JVM
+            if (isDebug() || LOG.isDebugEnabled()) {
+                LOG.debug("unable to enable unlimited-strength crypto " + e);
+            }
+        }
+        catch (Exception e) {
             if (isDebug() || LOG.isDebugEnabled()) {
                 LOG.debug("unable to enable unlimited-strength crypto", e);
             }
-- 
Member

kares commented Aug 22, 2016

... and the naive patch would be :

diff --git a/core/src/main/java/org/jruby/Ruby.java b/core/src/main/java/org/jruby/Ruby.java
index 9f56537..1c06db5 100644
--- a/core/src/main/java/org/jruby/Ruby.java
+++ b/core/src/main/java/org/jruby/Ruby.java
@@ -170,6 +170,7 @@ import java.lang.invoke.MethodHandle;
 import java.lang.ref.WeakReference;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
 import java.net.BindException;
 import java.net.MalformedURLException;
 import java.net.URL;
@@ -1288,10 +1289,23 @@ public final class Ruby implements Constantizable {
         try {
             Class jceSecurity = Class.forName("javax.crypto.JceSecurity");
             Field isRestricted = jceSecurity.getDeclaredField("isRestricted");
-            isRestricted.setAccessible(true);
-            isRestricted.set(null, false);
-            isRestricted.setAccessible(false);
-        } catch (Exception e) {
+            if ( Boolean.TRUE.equals(isRestricted.get(null)) ) {
+                if ( Modifier.isFinal(isRestricted.getModifiers()) ) {
+                    Field modifiers = Field.class.getDeclaredField("modifiers");
+                    modifiers.setAccessible(true);
+                    modifiers.setInt(isRestricted, isRestricted.getModifiers() & ~Modifier.FINAL);
+                }
+                isRestricted.setAccessible(true);
+                isRestricted.setBoolean(null, false); // isRestricted = false;
+                isRestricted.setAccessible(false);
+            }
+        }
+        catch (ClassNotFoundException e) { // not an OpenJDK ~ JVM
+            if (isDebug() || LOG.isDebugEnabled()) {
+                LOG.debug("unable to enable unlimited-strength crypto " + e);
+            }
+        }
+        catch (Exception e) {
             if (isDebug() || LOG.isDebugEnabled()) {
                 LOG.debug("unable to enable unlimited-strength crypto", e);
             }
-- 
@jkutner

This comment has been minimized.

Show comment
Hide comment
@jkutner

jkutner Aug 22, 2016

Member

I'm not sure if it's ok for JRuby to ship this. I mean, I'm not a lawyer.

But JRuby users can easily adjust their Rail boot scripts to include the modifiers.setInt(isRestricted, isRestricted.getModifiers() & ~Modifier.FINAL);, which keeps the status quo working.

@kares thanks for figuring this out!

Member

jkutner commented Aug 22, 2016

I'm not sure if it's ok for JRuby to ship this. I mean, I'm not a lawyer.

But JRuby users can easily adjust their Rail boot scripts to include the modifiers.setInt(isRestricted, isRestricted.getModifiers() & ~Modifier.FINAL);, which keeps the status quo working.

@kares thanks for figuring this out!

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 22, 2016

Member

@jkutner I do not know either, but its been in for some time already. // cc @headius might know

can you explicitly confirm it actually works/helps your case please :) ?

Member

kares commented Aug 22, 2016

@jkutner I do not know either, but its been in for some time already. // cc @headius might know

can you explicitly confirm it actually works/helps your case please :) ?

@jkutner

This comment has been minimized.

Show comment
Hide comment
@jkutner

jkutner Aug 22, 2016

Member

@kares ah, i just realized the setAccessible stuff has been in there. TIL. But it seems that the custom code to set isRestricted is still needed, so I'm confused. Does this not get executed all the time?

Member

jkutner commented Aug 22, 2016

@kares ah, i just realized the setAccessible stuff has been in there. TIL. But it seems that the custom code to set isRestricted is still needed, so I'm confused. Does this not get executed all the time?

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 22, 2016

Member

yes should get executed on runtime initialization, thus the "Rails initializer hack" (or whatever you're using) shouldn't be necessary. on 8u102 you should see a trace printed with JRuby in debug mode: jruby -d (due the final modifier change)

Member

kares commented Aug 22, 2016

yes should get executed on runtime initialization, thus the "Rails initializer hack" (or whatever you're using) shouldn't be necessary. on 8u102 you should see a trace printed with JRuby in debug mode: jruby -d (due the final modifier change)

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 23, 2016

Member

It would be interesting to see what they do in the unlimited-strength crypto stuff now. They have to have a way to turn this off too. The code that existed before basically just emulated the same steps that they do.

Member

headius commented Aug 23, 2016

It would be interesting to see what they do in the unlimited-strength crypto stuff now. They have to have a way to turn this off too. The code that existed before basically just emulated the same steps that they do.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 23, 2016

Member

Here's the bug in OpenJDK: https://bugs.openjdk.java.net/browse/JDK-8149417

No other code changed.

# HG changeset patch
# User igerasim
# Date 1460476747 -10800
#      Tue Apr 12 18:59:07 2016 +0300
# Node ID 666ddde3fecf014cb3c2949542f317231b72db46
# Parent  7d49df0ab4f69bdb0c0751e37e37549e054b794d
8149417: Use final restricted flag
Reviewed-by: mullan, weijun, coffeys

diff -r 7d49df0ab4f6 -r 666ddde3fecf src/share/classes/javax/crypto/JceSecurity.java
--- a/src/share/classes/javax/crypto/JceSecurity.java   Mon Apr 11 14:42:45 2016 +0300
+++ b/src/share/classes/javax/crypto/JceSecurity.java   Tue Apr 12 18:59:07 2016 +0300
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -64,8 +64,7 @@
     private final static Map<Provider, Object> verifyingProviders =
             new IdentityHashMap<>();

-    // Set the default value. May be changed in the static initializer.
-    private static boolean isRestricted = true;
+    private static final boolean isRestricted;

     /*
      * Don't let anyone instantiate this.
Member

headius commented Aug 23, 2016

Here's the bug in OpenJDK: https://bugs.openjdk.java.net/browse/JDK-8149417

No other code changed.

# HG changeset patch
# User igerasim
# Date 1460476747 -10800
#      Tue Apr 12 18:59:07 2016 +0300
# Node ID 666ddde3fecf014cb3c2949542f317231b72db46
# Parent  7d49df0ab4f69bdb0c0751e37e37549e054b794d
8149417: Use final restricted flag
Reviewed-by: mullan, weijun, coffeys

diff -r 7d49df0ab4f6 -r 666ddde3fecf src/share/classes/javax/crypto/JceSecurity.java
--- a/src/share/classes/javax/crypto/JceSecurity.java   Mon Apr 11 14:42:45 2016 +0300
+++ b/src/share/classes/javax/crypto/JceSecurity.java   Tue Apr 12 18:59:07 2016 +0300
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2016, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -64,8 +64,7 @@
     private final static Map<Provider, Object> verifyingProviders =
             new IdentityHashMap<>();

-    // Set the default value. May be changed in the static initializer.
-    private static boolean isRestricted = true;
+    private static final boolean isRestricted;

     /*
      * Don't let anyone instantiate this.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 23, 2016

Member

So @kares's trick seems fine to me. It would be better moved out to a separate utility at this point, think.

The alternative, for those who question how kosher it is for us to do this, would be to improve our handling of the situation when these policy files do not exist. The logic in JDK basically just loads up two jar files from the crypto extension...jar files which contain little more than two small security policy files. It would not be difficult for us to present a warning when crypto gets booted with helpful links and/or utilities to install the policy files.

Here's the code that actually does the policy file logic in JDK: https://gist.github.com/headius/84a4b41706523579f9829db0a66904f8

Member

headius commented Aug 23, 2016

So @kares's trick seems fine to me. It would be better moved out to a separate utility at this point, think.

The alternative, for those who question how kosher it is for us to do this, would be to improve our handling of the situation when these policy files do not exist. The logic in JDK basically just loads up two jar files from the crypto extension...jar files which contain little more than two small security policy files. It would not be difficult for us to present a warning when crypto gets booted with helpful links and/or utilities to install the policy files.

Here's the code that actually does the policy file logic in JDK: https://gist.github.com/headius/84a4b41706523579f9829db0a66904f8

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 23, 2016

Member

A tip from a SO user indicates that our trick doesn't entirely disable the restrictions. I'm leaning toward being noisy until the user downloads policy files or runs our utility.

The JceSecurity.isRestricted = false part is all that is needed to use 256-bit ciphers directly; however, without the two other operations, Cipher.getMaxAllowedKeyLength() will still keep reporting 128, and 256-bit TLS cipher suites won't work.

This user's much larger hack, with the additional tweaks, is provided here: http://stackoverflow.com/questions/1179672/how-to-avoid-installing-unlimited-strength-jce-policy-files-when-deploying-an

Member

headius commented Aug 23, 2016

A tip from a SO user indicates that our trick doesn't entirely disable the restrictions. I'm leaning toward being noisy until the user downloads policy files or runs our utility.

The JceSecurity.isRestricted = false part is all that is needed to use 256-bit ciphers directly; however, without the two other operations, Cipher.getMaxAllowedKeyLength() will still keep reporting 128, and 256-bit TLS cipher suites won't work.

This user's much larger hack, with the additional tweaks, is provided here: http://stackoverflow.com/questions/1179672/how-to-avoid-installing-unlimited-strength-jce-policy-files-when-deploying-an

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 23, 2016

Member

@kares We've never had anyone complain about the restricted key stuff outside of jruby-openssl use, have we? I'm thinking we could do the following if we wanted to remove the reflection hacks:

  • In jruby-openssl, check these policies and whatnot. If they're not set to the unlimited strength stuff, make noise on install recommending the user run our utility.
  • The utility would just be another gem with a script to help you download the policy files and install it into the JDK in the right place.
  • Alternatively the warning could link to a page describing how to do it yourself.

The key point is that we'd be proactively warning the user that they might run into crypto problems, rather than letting them fall into a cryptic (pun intended) JCE error message.

Member

headius commented Aug 23, 2016

@kares We've never had anyone complain about the restricted key stuff outside of jruby-openssl use, have we? I'm thinking we could do the following if we wanted to remove the reflection hacks:

  • In jruby-openssl, check these policies and whatnot. If they're not set to the unlimited strength stuff, make noise on install recommending the user run our utility.
  • The utility would just be another gem with a script to help you download the policy files and install it into the JDK in the right place.
  • Alternatively the warning could link to a page describing how to do it yourself.

The key point is that we'd be proactively warning the user that they might run into crypto problems, rather than letting them fall into a cryptic (pun intended) JCE error message.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 23, 2016

Member

@headius nope - no explicit complains for restricted. but most users might not know they can install a JCE pack for unlimited crypto (I wonder if packaged OpenJDK builds e.g. in Debian do setup unlimited policy - probably not and they might not provide explicit support to do so neither). recall there's been issues around max-allowed-key-length but I believe its worked around (BC ciphers handle longer keys).

I do like your idea however - having jruby-openssl gem packed with a script to help installing JCE pack.

Member

kares commented Aug 23, 2016

@headius nope - no explicit complains for restricted. but most users might not know they can install a JCE pack for unlimited crypto (I wonder if packaged OpenJDK builds e.g. in Debian do setup unlimited policy - probably not and they might not provide explicit support to do so neither). recall there's been issues around max-allowed-key-length but I believe its worked around (BC ciphers handle longer keys).

I do like your idea however - having jruby-openssl gem packed with a script to help installing JCE pack.

@mkristian

This comment has been minimized.

Show comment
Hide comment
@mkristian

mkristian Aug 23, 2016

Member

+1 for adding a script to jruby-openssl to help the installing of missing bits.

Member

mkristian commented Aug 23, 2016

+1 for adding a script to jruby-openssl to help the installing of missing bits.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 24, 2016

Member

@kares For now, commit your patch. It's no worse than what we had. If you could, move it into a utility class somewhere, like Helpers. At least this will be working as well as it used to for 9.1.3.0 + 8u102.

We'll deal with the better solution later.

Member

headius commented Aug 24, 2016

@kares For now, commit your patch. It's no worse than what we had. If you could, move it into a utility class somewhere, like Helpers. At least this will be working as well as it used to for 9.1.3.0 + 8u102.

We'll deal with the better solution later.

@headius headius added this to the JRuby 9.1.3.0 milestone Aug 24, 2016

@kares kares closed this in d0dedf9 Aug 24, 2016

headius added a commit that referenced this issue Aug 29, 2016

Don't try to read inaccessible field before setting it accessible.
This also attempts to make this hackery happen only once, in case
this logic is called from many places (e.g. many JRuby instances
in the same JVM).

See #4101.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 29, 2016

Member

@kares FYI, the pre-check of "isAccessible" was causing this code to never run, since it was never accessible to be read. I removed that check and instead used a static boolean to "guarantee" we only attempt this hack once.

I am also going to remove the check for Oracle JDK vs OpenJDK.

Member

headius commented Aug 29, 2016

@kares FYI, the pre-check of "isAccessible" was causing this code to never run, since it was never accessible to be read. I removed that check and instead used a static boolean to "guarantee" we only attempt this hack once.

I am also going to remove the check for Oracle JDK vs OpenJDK.

headius added a commit that referenced this issue Aug 29, 2016

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Aug 31, 2016

Member

my bad - did not realize that - thought the hack will only be needed on OracleJDK (not on OpenJDK).

@headius with your change under different JDKs there will be an info being logged ...
the reasoning was we want to know early if its being changed/inaccessible but it does not make sense elsewhere e.g. under IBM Java :

kares@sputnik:~/workspace/oss/jruby$ export JAVA_HOME=/opt/java/ibm-java-i386-71
kares@sputnik:~/workspace/oss/jruby$ jruby -v -S irb
jruby 9.1.3.0 (2.3.1) 2016-08-29 a2a3b29 IBM J9 VM 2.7 on pxi3270_27-20131115_04 +jit [linux-x86]
2016-08-31T13:35:08.899+02:00 [main] INFO SecurityHelper : unable un-restrict jce security: java.lang.ClassNotFoundException: javax.crypto.JceSecurity

... maybe going back to debug shall do as its not that relevant? - whatever you feel is best.

Member

kares commented Aug 31, 2016

my bad - did not realize that - thought the hack will only be needed on OracleJDK (not on OpenJDK).

@headius with your change under different JDKs there will be an info being logged ...
the reasoning was we want to know early if its being changed/inaccessible but it does not make sense elsewhere e.g. under IBM Java :

kares@sputnik:~/workspace/oss/jruby$ export JAVA_HOME=/opt/java/ibm-java-i386-71
kares@sputnik:~/workspace/oss/jruby$ jruby -v -S irb
jruby 9.1.3.0 (2.3.1) 2016-08-29 a2a3b29 IBM J9 VM 2.7 on pxi3270_27-20131115_04 +jit [linux-x86]
2016-08-31T13:35:08.899+02:00 [main] INFO SecurityHelper : unable un-restrict jce security: java.lang.ClassNotFoundException: javax.crypto.JceSecurity

... maybe going back to debug shall do as its not that relevant? - whatever you feel is best.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 6, 2016

Member

@kares Thanks for patching that warning 👍

Member

headius commented Sep 6, 2016

@kares Thanks for patching that warning 👍

@vijaykramesh

This comment has been minimized.

Show comment
Hide comment
@vijaykramesh

vijaykramesh Dec 2, 2016

will there be a 1.7 release with this fix?

vijaykramesh commented Dec 2, 2016

will there be a 1.7 release with this fix?

jurajsomorovsky added a commit to RUB-NDS/JOSEPH that referenced this issue Dec 7, 2016

Recent Java version changed the Java crypto restriction modifier to f…
…inal. This code attempts to solve this issue. See also

https://github.com/jruby/jruby/blob/0c345e1b186bd457ebd96143c0816abe93b18fdf/core/src/main/java/org/jruby/util/SecurityHelper.java
and the relevant discussion at
jruby/jruby#4101

In addition, code reformatting is executed during process-sources maven phase automatically.
@xuanzhaopeng

This comment has been minimized.

Show comment
Hide comment
@xuanzhaopeng

xuanzhaopeng Jan 10, 2017

Hi, When I change to Java 8u112, isRestricted.get(null) will also directly raise the IllegalAccessException,

So following will works without the condition

if ( Modifier.isFinal(isRestricted.getModifiers()) ) {
                    Field modifiers = Field.class.getDeclaredField("modifiers");
                    modifiers.setAccessible(true);
                    modifiers.setInt(isRestricted, isRestricted.getModifiers() & ~Modifier.FINAL);
                }
                isRestricted.setAccessible(true);
                isRestricted.setBoolean(null, false); // isRestricted = false;
                isRestricted.setAccessible(false);

xuanzhaopeng commented Jan 10, 2017

Hi, When I change to Java 8u112, isRestricted.get(null) will also directly raise the IllegalAccessException,

So following will works without the condition

if ( Modifier.isFinal(isRestricted.getModifiers()) ) {
                    Field modifiers = Field.class.getDeclaredField("modifiers");
                    modifiers.setAccessible(true);
                    modifiers.setInt(isRestricted, isRestricted.getModifiers() & ~Modifier.FINAL);
                }
                isRestricted.setAccessible(true);
                isRestricted.setBoolean(null, false); // isRestricted = false;
                isRestricted.setAccessible(false);
@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jan 19, 2017

Member

@xuanzhaopeng exactly the code we have in place already, I am not sure what you're suggesting here

Member

kares commented Jan 19, 2017

@xuanzhaopeng exactly the code we have in place already, I am not sure what you're suggesting here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment