Added validation and exception handling in readAttribute #405
Conversation
|
Hello @Shivam7-1 You'll need to add a unit test that fails without the changes to 'main'. It looks like the indentation is messed up. Ty. |
|
Hii @garydgregory Thanks for response done this |
There was a problem hiding this comment.
You did not follow the instructions in the PR template: Run mvn by itself and fix all errors. The PR title is confusing: The PR doesn't handle any exceptions, it throws exceptions for validation failures.
See my scattered comments as well.
Overall, I'm not sure if this PR is needed: If I apply the test side of the patch, I get ClassFormatExceptions already for the new tests.
Looking at the method, you could see where an NPE could happen but that's not what the tests cause.
| @@ -18,6 +18,7 @@ | |||
| */ | |||
| package org.apache.bcel.classfile; | |||
|
|
|||
| import java.security.InvalidParameterException; | |||
There was a problem hiding this comment.
This change has nothing to do with JCA or JCE, so java.security.InvalidParameterException is the wrong type. We define ClassFormatException for this very purpose. Examine the reset of the code base for examples.
| @@ -113,10 +114,16 @@ public static Attribute readAttribute(final DataInput dataInput, final ConstantP | |||
| // Get class name from constant pool via 'name_index' indirection | |||
| final int nameIndex = dataInput.readUnsignedShort(); | |||
| final String name = constantPool.getConstantUtf8(nameIndex).getBytes(); | |||
|
|
|||
| // Validate name | |||
| if (name == null || name.isEmpty()) { | |||
There was a problem hiding this comment.
Reuse StringUtils.isEmpty().
| @@ -125,8 +132,8 @@ public static Attribute readAttribute(final DataInput dataInput, final ConstantP | |||
| } | |||
| } | |||
|
|
|||
| // Call proper constructor, depending on 'tag' | |||
| switch (tag) { | |||
| // Call proper constructor, depending on 'tag' | |||
There was a problem hiding this comment.
The indentation is still messed up, no need to change it.
| import org.apache.bcel.classfile.ConstantPool; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class readAttributeTest { |
There was a problem hiding this comment.
Class name does not follow Java conventions.
Issue link: https://issues.oss-fuzz.com/issues/375660994
Added validation for attribute name and length in readAttribute method to prevent security exceptions. This change ensures that invalid or malformed attributes are handled properly, enhancing the robustness and security of the library.
Before you push a pull request, review this list:
mvn; that'smvnon the command line by itself.