Col-E / Recaf

The modern Java bytecode editor
https://recaf.coley.software
MIT License
6.06k stars 467 forks source link

Recaf accepts invalid argument for 'new' insn #837

Closed C0D3-M4513R closed 3 months ago

C0D3-M4513R commented 3 months ago

Still using recaf v4 btw.

I made a custom class using bytecode directly.

The bytecode is this:

.method public static main ([Ljava/lang/String;)V {
    parameters: { args },
    code: {
    A: 
        line 6
        iconst_3 
        anewarray [[Ljava/lang/Object;
        pop 
        iconst_3 
        anewarray [Ljava/lang/Object;
        pop
        iconst_3 
        anewarray [J
        pop
        iconst_3 
        anewarray [[Ljava/lang/Object;
        iconst_0
        aaload
        checkcast [Ljava/lang/String;
        pop
        aconst_null
        instanceof [Ljava/lang/Object;
        iconst_0
        if_icmpeq Z
        return 
   B: 
   Z:
        new Ljava/lang/String;
        dup
        ldc "test"
        invokespecial java/lang/String.<init> (Ljava/lang/String;)V
        pop
        return
    }
}

Which decompiles to this java code (yes, it basically does nothing and will crash because of a ClassCastException):

package pkg;

public class Test {
    public static void main(String[] args) {
        Object[][][] cfr_ignored_0 = new Object[3][][];
        Object[][] cfr_ignored_1 = new Object[3][];
        long[][] cfr_ignored_2 = new long[3][];
        String[] cfr_ignored_3 = (String[])(new Object[3][][])[0];
        if (null instanceof Object[]) {
            return;
        }
        new String("test");
    }
}

Here is the class attached: (curse you github arbitrary file-name/file-type limitations) test.zip

I had issues parsing the class in cafebabe (a rust jvm bytecode parser) (see https://github.com/staktrace/cafebabe/issues/41).

The tests in cafebabe failed with the following error, when adding my class:

[FAIL]: "/home/user/Documents/cafebabe/tests/parse/clazz/Test.class"
Invalid binary name for constant pool entry 23
thread 'parse_success' panicked at tests/parse.rs:21:23:
[FAIL]: "/home/user/Documents/cafebabe/tests/parse/clazz/Test.class"
Invalid binary name for constant pool entry 23
stack backtrace:
   0: rust_begin_unwind
             at /rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/std/src/panicking.rs:647:5
   1: core::panicking::panic_fmt
             at /rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/panicking.rs:72:14
   2: parse::parse_success
             at ./tests/parse.rs:21:23
   3: parse::parse_success::{{closure}}
             at ./tests/parse.rs:9:19
   4: core::ops::function::FnOnce::call_once
             at /rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/ops/function.rs:250:5
   5: core::ops::function::FnOnce::call_once
             at /rustc/7cf61ebde7b22796c69757901dd346d0fe70bd97/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Removing the new String("test") bit (in bytecode ofc though) fixed the error.

`javap -v` Output

``` Classfile /home/user/Downloads/Test.class Last modified Jul 31, 2024; size 584 bytes SHA-256 checksum 9ccf2f55d7847806a5529db871ab1a5e0fd17a808030abfd7a765c1a6becc125 Compiled from "Test.java" public class pkg.Test minor version: 0 major version: 60 flags: (0x0021) ACC_PUBLIC, ACC_SUPER this_class: #2 // pkg/Test super_class: #4 // java/lang/Object interfaces: 0, fields: 0, methods: 2, attributes: 1 Constant pool: #1 = Utf8 pkg/Test #2 = Class #1 // pkg/Test #3 = Utf8 java/lang/Object #4 = Class #3 // java/lang/Object #5 = Utf8 Test.java #6 = Utf8 #7 = Utf8 ()V #8 = NameAndType #6:#7 // "":()V #9 = Methodref #4.#8 // java/lang/Object."":()V #10 = Utf8 this #11 = Utf8 Lpkg/Test; #12 = Utf8 main #13 = Utf8 ([Ljava/lang/String;)V #14 = Utf8 [[Ljava/lang/Object; #15 = Class #14 // "[[Ljava/lang/Object;" #16 = Utf8 [Ljava/lang/Object; #17 = Class #16 // "[Ljava/lang/Object;" #18 = Utf8 [J #19 = Class #18 // "[J" #20 = Utf8 [Ljava/lang/String; #21 = Class #20 // "[Ljava/lang/String;" #22 = Utf8 Ljava/lang/String; #23 = Class #22 // "Ljava/lang/String;" #24 = Utf8 test #25 = String #24 // test #26 = Utf8 java/lang/String #27 = Class #26 // java/lang/String #28 = Utf8 (Ljava/lang/String;)V #29 = NameAndType #6:#28 // "":(Ljava/lang/String;)V #30 = Methodref #27.#29 // java/lang/String."":(Ljava/lang/String;)V #31 = Utf8 args #32 = Utf8 Code #33 = Utf8 LineNumberTable #34 = Utf8 LocalVariableTable #35 = Utf8 StackMapTable #36 = Utf8 SourceFile { public pkg.Test(); descriptor: ()V flags: (0x0001) ACC_PUBLIC Code: stack=1, locals=1, args_size=1 0: aload_0 1: invokespecial #9 // Method java/lang/Object."":()V 4: return LineNumberTable: line 3: 0 LocalVariableTable: Start Length Slot Name Signature 0 5 0 this Lpkg/Test; public static void main(java.lang.String[]); descriptor: ([Ljava/lang/String;)V flags: (0x0009) ACC_PUBLIC, ACC_STATIC Code: stack=3, locals=1, args_size=1 0: iconst_3 1: anewarray #15 // class "[[Ljava/lang/Object;" 4: pop 5: iconst_3 6: anewarray #17 // class "[Ljava/lang/Object;" 9: pop 10: iconst_3 11: anewarray #19 // class "[J" 14: pop 15: iconst_3 16: anewarray #15 // class "[[Ljava/lang/Object;" 19: iconst_0 20: aaload 21: checkcast #21 // class "[Ljava/lang/String;" 24: pop 25: aconst_null 26: instanceof #17 // class "[Ljava/lang/Object;" 29: iconst_0 30: if_icmpeq 34 33: return 34: new #23 // class "Ljava/lang/String;" 37: dup 38: ldc #25 // String test 40: invokespecial #30 // Method java/lang/String."":(Ljava/lang/String;)V 43: pop 44: return StackMapTable: number_of_entries = 1 frame_type = 34 /* same */ LineNumberTable: line 6: 0 LocalVariableTable: Start Length Slot Name Signature 0 45 0 args [Ljava/lang/String; } SourceFile: **"Test.java"** ```

The author of the cafebabe crate argues, that the class shouldn't parse, because:

I think the code in cafebabe is correct. Section 4.4.1 says:

The constant_pool entry at that index must be a CONSTANT_Utf8_info structure representing a valid binary class or interface name encoded in internal form.

The "binary class or interface name encoded in internal form" links to section 4.2.1 (when then references 4.2.2) which says:

An unqualified name must contain at least one Unicode code point and must not contain any of the ASCII characters . ; [ / (that is, period or semicolon or left square bracket or forward slash).

The entry is supposed to be java/lang/String and not Ljava/lang/String; here. If I take your test file and run java -cp . Test on it the JVM (openjdk version "21.0.2" 2024-01-16 on macOS) fails with this:

Error: LinkageError occurred while loading main class Test
  java.lang.ClassFormatError: Illegal class name "Ljava/lang/String;" in class file Test

which agrees with what cafebabe is doing.

SooStrator1136 commented 3 months ago

This isn't an issue related to recaf, in your bytecode you have new Ljava/lang/String; instead it should be new java/lang/String, the type should be referenced by internal name and not in descriptor format

C0D3-M4513R commented 3 months ago

Okay, but then recaf shouldn't accept that bytecode.

Col-E commented 3 months ago

Updated the title so it'll be for tracking more assembler strictness

Col-E commented 3 months ago

upstream: 2c61fd3e2ab7f9057d093213e3abce64c823ea1f