Escape referenced names in BCELFactory Java output#501
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the BCELifier Java-source output against code injection by escaping constant-pool-derived names before embedding them into generated Java string literals.
Changes:
- Escape
ObjectTypeclass names when emittingPUSHconstants inBCELFactory. - Escape referenced class/field/method names when emitting
createNew,createFieldAccess, andcreateInvokestatements inBCELFactory. - Add a regression test that constructs a class with a hostile constant-pool method name and verifies the generated BCELifier output contains the escaped form (and not the raw injected form).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/java/org/apache/bcel/util/BCELFactory.java | Escapes class/field/method names before embedding them into generated Java string literals. |
| src/test/java/org/apache/bcel/util/BCELifierTest.java | Adds a regression test to ensure hostile constant-pool method names are escaped in BCELifier output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assertTrue(source.contains(Utility.convertString(evilName)), source); | ||
| assertFalse(source.contains('"' + evilName + '"'), source); |
There was a problem hiding this comment.
makes sense. tightened it to assert the escaped name appears as the quoted createInvoke argument instead of just anywhere in the output.
| void testCreateInvokeEscapesConstantPoolName() throws Exception { | ||
| // A hostile constant pool can hold any UTF-8 as a referenced method name. | ||
| final String evilName = "evil\"); System.exit(1); il.append(\""; | ||
| final ClassGen cg = new ClassGen("Example", "java.lang.Object", "Example.java", Const.ACC_PUBLIC | Const.ACC_SUPER, new String[] {}); | ||
| final ConstantPoolGen cp = cg.getConstantPool(); | ||
| final InstructionFactory factory = new InstructionFactory(cg, cp); | ||
| final InstructionList il = new InstructionList(); | ||
| il.append(InstructionConst.ALOAD_0); | ||
| il.append(factory.createInvoke("java.lang.Object", evilName, Type.VOID, Type.NO_ARGS, Const.INVOKEVIRTUAL)); | ||
| il.append(InstructionConst.RETURN); | ||
| final MethodGen mg = new MethodGen(Const.ACC_PUBLIC, Type.VOID, Type.NO_ARGS, new String[] {}, "m", "Example", il, cp); | ||
| mg.setMaxStack(); | ||
| mg.setMaxLocals(); | ||
| cg.addMethod(mg.getMethod()); | ||
|
|
There was a problem hiding this comment.
yep, real gap. same root cause as the createInvoke case: BCELifier writes constant-pool-derived names straight into the generated string literals. pushed a follow-up that routes the class/superclass/source-file/interface names, the MethodGen method and class names, exception names, argument names and the stackmap names through Utility.convertString too. added tests for the method declaration name and the class/source-file cases.
garydgregory
changed the title
garydgregory
changed the title
| @Test | ||
| void testCreateInvokeEscapesConstantPoolName() throws Exception { | ||
| // A hostile constant pool can hold any UTF-8 as a referenced method name. | ||
| final String evilName = "evil\"); System.exit(1); il.append(\""; |
There was a problem hiding this comment.
@rootvector2
In the future, please avoid the FUD and use meaningful named like "content" or "escapeExpected".
comparing
createConstant(which escapes stringLDCoperands viaUtility.convertString) against the siblingcreateNew/createFieldAccess/createInvokeemitters shows those write referenced class, field and method names raw, so a crafted constant-pool name such asfoo"); exec(...); il.append("breaks out of the generated string literal and injects code into theBCELifieroutput, which the same helper now prevents.