From 849cd3dad8fd1cd551a4b7e43987ff2c3ecb2b0b Mon Sep 17 00:00:00 2001 From: Samikshya Chand <148681192+samikshya-db@users.noreply.github.com> Date: Thu, 7 Nov 2024 18:35:23 +0530 Subject: [PATCH] [Fix] Fix vulnerabilities in the present SDK version (#383) ## 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. --- databricks-sdk-java/pom.xml | 8 ++-- .../com/databricks/sdk/core/ConfigLoader.java | 41 ++++++++++--------- 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/databricks-sdk-java/pom.xml b/databricks-sdk-java/pom.xml index cba1f7855..a49f22054 100644 --- a/databricks-sdk-java/pom.xml +++ b/databricks-sdk-java/pom.xml @@ -49,9 +49,9 @@ provided - org.ini4j - ini4j - 0.5.4 + org.apache.commons + commons-configuration2 + 2.11.0 compile @@ -67,7 +67,7 @@ commons-io commons-io - 2.13.0 + 2.14.0 org.junit.jupiter diff --git a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/ConfigLoader.java b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/ConfigLoader.java index 933fa50ac..47779d0ee 100644 --- a/databricks-sdk-java/src/main/java/com/databricks/sdk/core/ConfigLoader.java +++ b/databricks-sdk-java/src/main/java/com/databricks/sdk/core/ConfigLoader.java @@ -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; @@ -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; } @@ -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); @@ -86,27 +87,27 @@ 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; } @@ -114,18 +115,18 @@ static void loadFromConfig(DatabricksConfig cfg) throws IllegalAccessException { } } - 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) { @@ -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: ")); + buf.add("Config: "); } if (!envsUsed.isEmpty()) { buf.add(String.format("Env: %s", String.join(", ", envsUsed))); } else { - buf.add(String.format("Env: ")); + buf.add("Env: "); } return String.join(". ", buf); } catch (IllegalAccessException e) {