Skip to content

Commit

Permalink
[Fix] Fix vulnerabilities in the present SDK version (#383)
Browse files Browse the repository at this point in the history
## What changes are proposed in this pull request?

- **What** : 
- Update commons.io to fix the [CVE in the present
version](https://mvnrepository.com/artifact/com.databricks/databricks-sdk-java/0.34.0).
Looks like depandabot PRs are no longer being created/merged.
[[Link](https://github.com/databricks/databricks-sdk-java/pull/261/files)]
  - Change ini4j configuration because of vulnerability.

- **Why** 
- ini4j 0.5.4 version has an infinite loop situation in the following
piece of code. This loop can cause excessive memory and CPU usage,
potentially crashing the application. Alternate libraries like Apache
Commons Configuration gracefully handle the situation (by limiting the
recursions internally). I will raise a PR on SDK later today to replace
the ini4j library. Moreover : the official site of ini4j [is up for
sale](http://www.ini4j.org/) and the last update to this maven package
was done in [2015](https://mvnrepository.com/artifact/org.ini4j/ini4j).
There is no reason we should continue to use this package.

```
Ini ini = new Ini();
  ini.load(new ByteArrayInputStream("""
      [deploy]
      a = ${test/a}
      b = ${doc/b}
              
      [test]
      a = ${deploy/a}
      b = ${deploy/b}
              
      [doc]
      a = 15
      b = 45
      """.getBytes(StandardCharsets.UTF_8)));
  
  // Will cause stack overflow
  ini.get("deploy").fetch("a");
```

## How is this tested?

- The existing unit tests run fine.
  • Loading branch information
samikshya-db authored Nov 7, 2024
1 parent 5deef7d commit 849cd3d
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 24 deletions.
8 changes: 4 additions & 4 deletions databricks-sdk-java/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.ini4j</groupId>
<artifactId>ini4j</artifactId>
<version>0.5.4</version>
<groupId>org.apache.commons</groupId>
<artifactId>commons-configuration2</artifactId>
<version>2.11.0</version>
<scope>compile</scope>
</dependency>
<dependency>
Expand All @@ -67,7 +67,7 @@
<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>2.13.0</version>
<version>2.14.0</version>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
package com.databricks.sdk.core;

import com.databricks.sdk.core.utils.Environment;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.lang.reflect.Field;
import java.net.MalformedURLException;
import java.net.URL;
import java.nio.file.Paths;
import java.util.*;
import org.ini4j.Ini;
import org.ini4j.Profile;
import org.apache.commons.configuration2.INIConfiguration;
import org.apache.commons.configuration2.SubnodeConfiguration;
import org.apache.commons.configuration2.ex.ConfigurationException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -40,7 +41,7 @@ public static DatabricksConfig resolve(DatabricksConfig cfg) throws DatabricksEx
}
}

static void loadFromEnvironmentVariables(DatabricksConfig cfg) throws IllegalAccessException {
static void loadFromEnvironmentVariables(DatabricksConfig cfg) {
if (cfg.getEnv() == null) {
return;
}
Expand All @@ -57,7 +58,7 @@ static void loadFromEnvironmentVariables(DatabricksConfig cfg) throws IllegalAcc
}
accessor.setValueOnConfig(cfg, env);
}
} catch (DatabricksException e) {
} catch (DatabricksException | IllegalAccessException e) {
String msg =
String.format("%s auth: %s", cfg.getCredentialsProvider().authType(), e.getMessage());
throw new DatabricksException(msg, e);
Expand Down Expand Up @@ -86,46 +87,46 @@ static void loadFromConfig(DatabricksConfig cfg) throws IllegalAccessException {
configFile = configFile.replaceFirst("^~", userHome);
}

Ini ini = parseDatabricksCfg(configFile, isDefaultConfig);
INIConfiguration ini = parseDatabricksCfg(configFile, isDefaultConfig);
if (ini == null) return;

String profile = cfg.getProfile();
boolean hasExplicitProfile = !isNullOrEmpty(profile);
if (!hasExplicitProfile) {
profile = "DEFAULT";
}

Profile.Section section = ini.get(profile);
if (section == null && !hasExplicitProfile) {
SubnodeConfiguration section = ini.getSection(profile);
boolean sectionNotPresent = section == null || section.isEmpty();
if (sectionNotPresent && !hasExplicitProfile) {
LOG.info("{} has no {} profile configured", configFile, profile);
return;
}

if (section == null) {
if (sectionNotPresent) {
String msg = String.format("resolve: %s has no %s profile configured", configFile, profile);
throw new DatabricksException(msg);
}

for (ConfigAttributeAccessor accessor : accessors) {
String value = section.get(accessor.getName());
String value = section.getString(accessor.getName());
if (!isNullOrEmpty(accessor.getValueFromConfig(cfg))) {
continue;
}
accessor.setValueOnConfig(cfg, value);
}
}

private static Ini parseDatabricksCfg(String configFile, boolean isDefaultConfig) {
Ini ini = new Ini();
try {
ini.load(new File(configFile));
private static INIConfiguration parseDatabricksCfg(String configFile, boolean isDefaultConfig) {
INIConfiguration iniConfig = new INIConfiguration();
try (FileReader reader = new FileReader(configFile)) {
iniConfig.read(reader);
} catch (FileNotFoundException e) {
if (isDefaultConfig) {
return null;
}
} catch (IOException e) {
} catch (IOException | ConfigurationException e) {
throw new DatabricksException("Cannot load " + configFile, e);
}
return ini;
return iniConfig;
}

public static void fixHostIfNeeded(DatabricksConfig cfg) {
Expand Down Expand Up @@ -230,12 +231,12 @@ public static String debugString(DatabricksConfig cfg) {
if (!attrsUsed.isEmpty()) {
buf.add(String.format("Config: %s", String.join(", ", attrsUsed)));
} else {
buf.add(String.format("Config: <empty>"));
buf.add("Config: <empty>");
}
if (!envsUsed.isEmpty()) {
buf.add(String.format("Env: %s", String.join(", ", envsUsed)));
} else {
buf.add(String.format("Env: <none>"));
buf.add("Env: <none>");
}
return String.join(". ", buf);
} catch (IllegalAccessException e) {
Expand Down

0 comments on commit 849cd3d

Please sign in to comment.