fix: encode url query parameters DataSource (#1201)

BaseDataSource did not properly encode url parameters, meaning that users could not log in if their password contained illegal characters. The bug can be reproduced by setting the test user password to ';/?:@&=+$,' (a bunch of illegal characters for query parameters). Encode the parameters. Strictly speaking the parameter names should also be encoded but in this case they are a fixed set of words which only consist of safe characters. With the problem password, DriverTest also fails because it did not encode the parameters either. Encode the parameters in the test too. * fix: do not leak password to URL round-tripping a datasource through JNDI added the user and password to the ds properties as well as to the instance variables - which was not possible to do via setProperty. This may be a security issue if the URL is logged, and was in part why passwords with non-url-safe characters failed to connect in some circumstances. Set properties using setProperty, so that consistent logic applies to processing the user/password keys.
pgjdbc · Jun 30, 2018 · 9f3838f · 9f3838f
1 parent 5dc03f6
commit 9f3838f
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 17 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Wrong results when single statement is used with different bind types[PR 1137](https://github.com/pgjdbc/pgjdbc/pull/1137)
 - Support generated keys for WITH queries that miss RETURNING [PR 1138](https://github.com/pgjdbc/pgjdbc/pull/1138)
 - Support generated keys when INSERT/UPDATE/DELETE keyword is followed by a comment [PR 1138](https://github.com/pgjdbc/pgjdbc/pull/1138)
+- Properly encode special symbols in passwords in BaseDataSource [PR 1201](https://github.com/pgjdbc/pgjdbc/pull/1201)
 
 ## [42.2.1] (2018-01-25)
 ### Known issues

diff --git a/pgjdbc/src/main/java/org/postgresql/Driver.java b/pgjdbc/src/main/java/org/postgresql/Driver.java
@@ -13,12 +13,12 @@
 import org.postgresql.util.PSQLException;
 import org.postgresql.util.PSQLState;
 import org.postgresql.util.SharedTimer;
+import org.postgresql.util.URLCoder;
 import org.postgresql.util.WriterHandler;
 
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
-import java.net.URLDecoder;
 import java.security.AccessController;
 import java.security.PrivilegedActionException;
 import java.security.PrivilegedExceptionAction;
@@ -562,7 +562,7 @@ public static Properties parseURL(String url, Properties defaults) {
       if (slash == -1) {
         return null;
       }
-      urlProps.setProperty("PGDBNAME", URLDecoder.decode(l_urlServer.substring(slash + 1)));
+      urlProps.setProperty("PGDBNAME", URLCoder.decode(l_urlServer.substring(slash + 1)));
 
       String[] addresses = l_urlServer.substring(0, slash).split(",");
       StringBuilder hosts = new StringBuilder();
@@ -603,7 +603,7 @@ public static Properties parseURL(String url, Properties defaults) {
         urlProps.setProperty("PGHOST", "localhost");
       }
       if (defaults == null || !defaults.containsKey("PGDBNAME")) {
-        urlProps.setProperty("PGDBNAME", URLDecoder.decode(l_urlServer));
+        urlProps.setProperty("PGDBNAME", URLCoder.decode(l_urlServer));
       }
     }
 
@@ -617,7 +617,7 @@ public static Properties parseURL(String url, Properties defaults) {
       if (l_pos == -1) {
         urlProps.setProperty(token, "");
       } else {
-        urlProps.setProperty(token.substring(0, l_pos), URLDecoder.decode(token.substring(l_pos + 1)));
+        urlProps.setProperty(token.substring(0, l_pos), URLCoder.decode(token.substring(l_pos + 1)));
       }
     }
 

diff --git a/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java b/pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java
@@ -12,6 +12,7 @@
 import org.postgresql.util.GT;
 import org.postgresql.util.PSQLException;
 import org.postgresql.util.PSQLState;
+import org.postgresql.util.URLCoder;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
@@ -25,7 +26,6 @@
 import java.util.Properties;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-
 import javax.naming.NamingException;
 import javax.naming.RefAddr;
 import javax.naming.Reference;
@@ -1069,17 +1069,17 @@ public String getUrl() {
     if (portNumber != 0) {
       url.append(":").append(portNumber);
     }
-    url.append("/").append(databaseName);
+    url.append("/").append(URLCoder.encode(databaseName));
 
     StringBuilder query = new StringBuilder(100);
-    for (PGProperty property : PGProperty.values()) {
+    for (PGProperty property: PGProperty.values()) {
       if (property.isPresent(properties)) {
         if (query.length() != 0) {
           query.append("&");
         }
         query.append(property.getName());
         query.append("=");
-        query.append(property.get(properties));
+        query.append(URLCoder.encode(property.get(properties)));
       }
     }
 
@@ -1216,11 +1216,9 @@ public void setFromReference(Reference ref) {
       portNumber = Integer.parseInt(port);
     }
     serverName = getReferenceProperty(ref, "serverName");
-    user = getReferenceProperty(ref, "user");
-    password = getReferenceProperty(ref, "password");
 
     for (PGProperty property : PGProperty.values()) {
-      property.set(properties, getReferenceProperty(ref, property.getName()));
+      setProperty(property, getReferenceProperty(ref, property.getName()));
     }
   }
 

diff --git a/pgjdbc/src/main/java/org/postgresql/util/URLCoder.java b/pgjdbc/src/main/java/org/postgresql/util/URLCoder.java
@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) 2018, PostgreSQL Global Development Group
+ * See the LICENSE file in the project root for more information.
+ */
+
+package org.postgresql.util;
+
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+
+/**
+ * This class helps with URL encoding and decoding. UTF-8 encoding is used by default to make
+ * encoding consistent across the driver, and encoding might be changed via {@code
+ * postgresql.url.encoding} property
+ *
+ * Note: this should not be used outside of PostgreSQL source, this is not a public API of the
+ * driver.
+ */
+public final class URLCoder {
+  private static final String ENCODING_FOR_URL =
+      System.getProperty("postgresql.url.encoding", "UTF-8");
+
+  /**
+   * Decodes {@code x-www-form-urlencoded} string into Java string.
+   *
+   * @param encoded encoded value
+   * @return decoded value
+   * @see URLDecoder#decode(String, String)
+   */
+  public static String decode(String encoded) {
+    try {
+      return URLDecoder.decode(encoded, ENCODING_FOR_URL);
+    } catch (UnsupportedEncodingException e) {
+      throw new IllegalStateException(
+          "Unable to decode URL entry via " + ENCODING_FOR_URL + ". This should not happen", e);
+    }
+  }
+
+  /**
+   * Encodes Java string into {@code x-www-form-urlencoded} format
+   *
+   * @param plain input value
+   * @return encoded value
+   * @see URLEncoder#encode(String, String)
+   */
+  public static String encode(String plain) {
+    try {
+      return URLEncoder.encode(plain, "UTF-8");
+    } catch (UnsupportedEncodingException e) {
+      throw new IllegalStateException(
+          "Unable to encode URL entry via " + ENCODING_FOR_URL + ". This should not happen", e);
+    }
+  }
+}
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/DriverTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/DriverTest.java
@@ -16,6 +16,7 @@
 import org.postgresql.PGProperty;
 import org.postgresql.test.TestUtil;
 import org.postgresql.util.NullOutputStream;
+import org.postgresql.util.URLCoder;
 import org.postgresql.util.WriterHandler;
 
 import org.junit.Test;
@@ -106,7 +107,9 @@ public void testConnect() throws Exception {
 
     // Test with the username in the url
     con = DriverManager.getConnection(
-        TestUtil.getURL() + "&user=" + TestUtil.getUser() + "&password=" + TestUtil.getPassword());
+        TestUtil.getURL()
+            + "&user=" + URLCoder.encode(TestUtil.getUser())
+            + "&password=" + URLCoder.encode(TestUtil.getPassword()));
     assertNotNull(con);
     con.close();
 

diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/PGPropertyTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/PGPropertyTest.java
@@ -16,6 +16,7 @@
 import org.postgresql.ds.PGSimpleDataSource;
 import org.postgresql.ds.common.BaseDataSource;
 import org.postgresql.test.TestUtil;
+import org.postgresql.util.URLCoder;
 
 import org.junit.After;
 import org.junit.Before;
@@ -24,7 +25,6 @@
 import java.beans.BeanInfo;
 import java.beans.Introspector;
 import java.beans.PropertyDescriptor;
-import java.net.URLEncoder;
 import java.sql.DriverPropertyInfo;
 import java.util.ArrayList;
 import java.util.Map;
@@ -205,10 +205,10 @@ public void testEncodedUrlValues() {
     String userName = "&u%ser";
     String password = "p%a&s^s#w!o@r*";
     String url = "jdbc:postgresql://"
-            + "localhost" + ":" + 5432 + "/"
-            + URLEncoder.encode(databaseName)
-            + "?user=" + URLEncoder.encode(userName)
-            + "&password=" + URLEncoder.encode(password);
+        + "localhost" + ":" + 5432 + "/"
+        + URLCoder.encode(databaseName)
+        + "?user=" + URLCoder.encode(userName)
+        + "&password=" + URLCoder.encode(password);
     Properties parsed = Driver.parseURL(url, new Properties());
     assertEquals("database", databaseName, PGProperty.PG_DBNAME.get(parsed));
     assertEquals("user", userName, PGProperty.USER.get(parsed));
@@ -257,4 +257,25 @@ public void testLowerCamelCase() {
       }
     }
   }
+
+  @Test
+  public void testEncodedUrlValuesFromDataSource() {
+    String databaseName = "d&a%ta+base";
+    String userName = "&u%ser";
+    String password = "p%a&s^s#w!o@r*";
+    String applicationName = "Laurel&Hardy=Best?Yes";
+    PGSimpleDataSource dataSource = new PGSimpleDataSource();
+
+    dataSource.setDatabaseName(databaseName);
+    dataSource.setUser(userName);
+    dataSource.setPassword(password);
+    dataSource.setApplicationName(applicationName);
+
+    Properties parsed = Driver.parseURL(dataSource.getURL(), new Properties());
+    assertEquals("database", databaseName, PGProperty.PG_DBNAME.get(parsed));
+    // datasources do not pass username and password as URL parameters
+    assertFalse("user", PGProperty.USER.isPresent(parsed));
+    assertFalse("password", PGProperty.PASSWORD.isPresent(parsed));
+    assertEquals("APPLICATION_NAME", applicationName, PGProperty.APPLICATION_NAME.get(parsed));
+  }
 }
diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/optional/BaseDataSourceTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/optional/BaseDataSourceTest.java
@@ -5,6 +5,7 @@
 
 package org.postgresql.test.jdbc2.optional;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNotSame;
@@ -197,6 +198,7 @@ public void testPGConnection() {
   public void testJndi() {
     initializeDataSource();
     BaseDataSource oldbds = bds;
+    String oldurl = bds.getURL();
     InitialContext ic = getInitialContext();
     try {
       ic.rebind(DATA_SOURCE_JNDI, bds);
@@ -207,9 +209,12 @@ public void testJndi() {
       fail(e.getMessage());
     }
     oldbds = bds;
+    String url = bds.getURL();
     testUseConnection();
     assertSame("Test should not have changed DataSource (" + bds + " != " + oldbds + ")!",
         oldbds , bds);
+    assertEquals("Test should not have changed DataSource URL",
+        oldurl, url);
   }
 
   /**