Add null check for return value of CsvParser::parseLine

[arthurscchan]

This is a proposed fix to stability issue discovered by OSS-Fuzz when fuzzing the powsybl-core module. The original OSS-Fuzz issue can be found in https://issues.oss-fuzz.com/u/1/issues/406925425.

[arthurscchan]

A NullPointerException (NPE) can occur when calling the parseRecords method in AbstractRecordGroup or any subclasses, if one of the records passed to it is null. This happens because the method does not validate inputs before processing, and directly passes each record string into the CSV parser.

The problematic code is shown below.

powsybl-core/psse/psse-model/src/main/java/com/powsybl/psse/model/io/AbstractRecordGroup.java

Lines 171 to 174 in 455fd74

	for (String record : records) {
	String[] fields = parser.parseLine(record);
	context.setCurrentRecordNumFields(fields.length);
	}

The loop assumes that each record is a valid, non-null string containing a delimited CSV line. However, if record is null or malformed, CsvParser.parseLine(null) returns null, and the subsequent access to fields.length will throw a NullPointerException. It is found that CsvParser.parseLine(String) actually returns null in many situation, mostly in situation like malformed csv line.

Thus, the missed validation of the null return value from CsvParser cause unexpected NullPointerException and that affect the stability of the code.

[arthurscchan]

The CsvParser uses a null value to indicate that it has failed to parse a line of CSV data. As a result, this constitutes a stability issue in the powsybl module due to the absence of a check for a null return value from the CsvParser when parsing potentially untrusted CSV data. Below is a simple proof of concept to reproduce the issue.

The proof-of-concept (PoC) code includes dummy classes to simulate the instantiation of the target object and calls to the problematic code. Since these methods are protected, the PoC must reside in the same package as the target class to facilitate testing. However, in practice, several call paths do not require this setup and could still eventually invoke the vulnerable method.

package com.powsybl.psse.model.io;

import com.powsybl.psse.model.io.RecordGroupIdentification.JsonObjectType;
import com.univocity.parsers.csv.CsvParserSettings;
import java.util.Collections;
import java.util.List;

public class ProofOfConcept {
    public static void main(String[] args) {
        DummyRecordGroup group = new DummyRecordGroup();
        List<String> records = Collections.singletonList(null);
        group.parseRecords(records, new String[]{"field"}, new Context());
    }

    public static class DummyRecord {
        private String field;
        public String getField() { return field; }
        public void setField(String field) { this.field = field; }
    }

    public static class DummyRecordGroup extends AbstractRecordGroup<DummyRecord> {
        public DummyRecordGroup() {
            super(new RecordGroupIdentification() {
                @Override
                public String getDataName() {
                    return "dummy";
                }

                @Override
                public String getJsonNodeName() {
                    return "dummyJson";
                }

                @Override
                public String getLegacyTextName() {
                    return "dummyLegacy";
                }

                @Override
                public JsonObjectType getJsonObjectType() {
                    return JsonObjectType.DATA_TABLE;
                }
            }, "field");
        }

        @Override
        protected Class<DummyRecord> psseTypeClass() {
            return DummyRecord.class;
        }
    }
}

[arthurscchan]

To execute and test the PoC, follow the steps below. It is assumed that OpenJDK 17.0.2 and Maven 3.9.9 is used. Also, because of the protected status of the target method, the proof of concept class needed to be in the same package of the target class.

# Prepare OpenJDK 17.0.2
wget https://download.java.net/java/GA/jdk17.0.2/dfd4a8d0985749f896bed50d7138ee7f/8/GPL/openjdk-17.0.2_linux-x64_bin.tar.gz && tar zxvf openjdk-17.0.2_linux-x64_bin.tar.gz && rm openjdk-17.0.2_linux-x64_bin.tar.gz
export JAVA_HOME=./jdk-17.0.2
export PATH=$JAVA_HOME/bin:$PATH

# Prepare Maven 3.9.9
wget https://dlcdn.apache.org/maven/maven-3/3.9.9/binaries/apache-maven-3.9.9-bin.tar.gz && tar zxvf apache-maven-3.9.9-bin.tar.gz && rm apache-maven-3.9.9-bin.tar.gz
export PATH_TO_MVN=./apache-maven-3.9.9/bin/mvn

# Build Powsybl-metrix
git clone https://github.com/powsybl/powsybl-core
cd powsybl-core
$PATH_TO_MVN clean package -DskipTests

# Group jar files
mkdir jar
for jar in $(find ./ -type f -name "*.jar"); do cp $jar jar/; done

# Build and run PoC (The java file need to be in the directory following the needed packages)
javac -cp "jar/*" com/powsybl/psse/model/io/ProofOfConcept.java
java -cp "jar/*:./" com.powsybl.psse.model.io.ProofOfConcept

You will get the following exception stack trace.

Exception in thread "main" java.lang.NullPointerException: Cannot read the array length because "fields" is null
        at com.powsybl.psse.model.io.AbstractRecordGroup.parseRecords(AbstractRecordGroup.java:173)
        at com.powsybl.psse.model.io.ProofOfConcept.main(ProofOfConcept.java:12)

[arthurscchan]

The root cause is down at the AbstractRecordGroup::parseRecords method where the logic does not check for possible null value returned from CsvParser::parseLine method because of malformed records or other reason. This cause unexpected NPE and it is suggested to add a null check after the invocation of the CsvParser::parseLine method and throw the PsseException for parsing error instead of unexpected NPE for stability.