add some more tests by giantpune · Pull Request #14 · frida/frida-java-bridge

giantpune · 2017-02-03T22:46:24Z

No description provided.

oleavr

💣

oleavr · 2017-02-03T23:25:30Z

+                "  var tm = X509TrustManager.$new();" +
+                "}catch(e){" + 
+                "  var MethodTest = Java.use('re.frida.MethodTest');" +
+                "  MethodTest.Fail('couldnt create trustmanager: ' + e);" +


Let's use script.getNextMessage() instead (like above), that way we send('some string'); and assert that script.getNextMessage() gives us the same string.

oleavr · 2017-02-03T23:29:02Z

+    }
+
+    @Test
+    public void TestNewInterface() {


Should follow the camelCase() style here and below, and also avoid test as a prefix since it's already implicit from the class. I tried to follow the examples in the JUnit docs for naming, so in this case perhaps something like interfaceCannotBeInstantiated() or something like that.

oleavr · 2017-02-03T23:32:53Z

+        loadScript("var X509TrustManager = Java.use('javax.net.ssl.X509TrustManager');" +
+                "try{" +
+                "  var tm = X509TrustManager.$new();" +
+                "}catch(e){" + 


For the JS we should stick to semistandard, the style we're using in the implementation code. This is almost there, just missing spaces on each side of the curly braces and after catch.

oleavr · 2017-02-03T23:39:08Z

+        loadScript("var c = Java.use('java.lang.Class');" +
+                "try{" +
+                "  var orig = c.forName.overload('java.lang.String');" +
+                "  c.forName.overload('java.lang.String').implementation = function(s){ orig(s); };" +


For this test-suite we should avoid hooking things in the framework and only do that as a last resort. It's fine to start there though and refine the test so it's reproducing the same thing. The method in question here seems to be this one:

static Class<?> forName(String className)

And that made me think "uh oh, generics, we have some TODOs in that area", so maybe we can reproduce the same issue by using a locally defined method with the same kind of return type? I'd just throw it into the Badger class at the bottom.

I've tried adding a static Class<?> function to Badger and hooking it. Frida does not crash in the same way as when hooking java.lang.Class. I also tried hooking some different built-ins with similar returns like dalvik.system.VMStack.getStackClass2 and even Made a function in Badger that calls through directly to java.lang.Class.forName() and hooked that. So far, I'm unable to reproduce the same crash without directly hooking java.lang.Class. Maybe we can comment it out for now so it doesn't block the rest of the PR?

…erface_test

…om js to java

…s, and returning string

oleavr

Curious if things are looking better on the latest master – released in Frida 9.1.4 this evening – hopefully there will be less weirdness now 🤞

oleavr · 2017-02-07T00:28:16Z

+                "} catch (e) {" + 
+                "  send('couldnt create trustmanager');" + 
+                "}" + 
+                "send('ok');"


Shouldn't this be right after var tm = ..., and the assert below should check against the error-case? (As the test is checking that it is invalid to instantiate an interface.) We should however have another test that illustrates how to register a new class implementing an interface in pure JS – or perhaps this test is meant to evolve into that?

oleavr · 2017-02-07T00:29:12Z

+    }
+
+    @Test
+    public void TestClassForNameOrig() {


Malformed camel. :)

Also, this test should be named like the other tests, without a test prefix, and communicating the expected outcome.

oleavr · 2017-02-07T00:30:17Z

+    }
+
+    @Test
+    public void TestClassForName() {


Same two remarks as above.

oleavr · 2017-02-07T00:30:22Z

+    // this one was just hanging indefinitely during the test, but in an actual app, it was crashing
+    //! either one of those is bad.
+    @Test
+    public void TestMethodInvoke() {


Same two remarks as above.

oleavr · 2017-02-07T00:30:28Z

+    }*/
+
+    /*@Test
+    public void TestNativeLibraryLoading() {


Same two remarks as above.

oleavr · 2017-02-07T00:35:26Z

+                "} catch (e) {" + 
+                "  send('SecretKeySpec: ' + e);" + 
+                "}" + 
+                "send('ok');"


The ok feels like it belongs inside the try, at the very end (that is only reached if everything went well).

oleavr · 2017-02-07T00:35:46Z

+                "  c.$init.overload('[B', 'java.lang.String').implementation = function(a, b){ orig.call(this, a, b); };" +
+
+                // now look up the function again and call it
+                "  var testConstructor = c.$new( [1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1], 'AES' );" +


Semistandard isn't followed here.

oleavr · 2017-02-07T00:36:00Z

    }
+
+    static Class<?>forName() {
+      return Badger.class;


Indentation is off.

oleavr · 2017-02-07T00:36:18Z

        throw new IllegalStateException("Already dead");
    }
+
+    static Class<?>forName() {


Missing space between type and method name.

oleavr · 2017-02-07T00:36:49Z

+        loadScript("var c = Java.use('java.lang.Runtime');" +
+                "try{" +
+                "  var orig = c.loadLibrary.overload('java.lang.String');" +
+                "  c.loadLibrary.overload('java.lang.String').implementation = function(s){ orig(s); };" +


Semistandard isn't followed here.

…erface_test

oleavr

Let's push the style through the last mile :)

oleavr · 2017-02-07T17:43:23Z


 import javax.crypto.Cipher;
 import java.io.IOException;
+import javax.crypto.Cipher;


This import looks like a duplicate.

oleavr · 2017-02-07T17:44:52Z

+    public void interfaceCannotBeInstantiated() {
+        loadScript("var X509TrustManager = Java.use('javax.net.ssl.X509TrustManager');" +
+                "try {" +
+                "  var tm = X509TrustManager.$new();" +


Just noticed that indentation is inside the string, whereas the existing tests puts the indentation outside. I prefer your approach though, so I'll fix the other tests so they match it.

oleavr · 2017-02-07T17:45:43Z

+                "} catch (e) {" + 
+                "  send('couldnt create trustmanager');" + 
+                "}"
+                );


The ); should go on the previous line for consistency.

oleavr · 2017-02-07T17:45:49Z

+                "} catch (e) {" + 
+                "  send('class.forName failed. ' + e);" + 
+                "}"
+                );


The ); should go on the previous line for consistency.

oleavr · 2017-02-07T17:45:55Z

+                "} catch (e) {" + 
+                "  send('forName failed. ' + e);" + 
+                "}"
+                );


The ); should go on the previous line for consistency.

oleavr · 2017-02-07T18:01:05Z

+        loadScript("var c = Java.use('re.frida.Badger');" +
+                "try {" +
+                "  var orig = c.forName;" +
+                "  c.forName.implementation = function () { " +
