Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid String use with secure strings #1813

Merged
merged 2 commits into from
Apr 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java
Original file line number Diff line number Diff line change
Expand Up @@ -1584,11 +1584,6 @@ void enableSSL(String host, int port, String clientCertificate, String clientKey
String trustStoreFileName = con.activeConnectionProperties
.getProperty(SQLServerDriverStringProperty.TRUST_STORE.toString());

String trustStorePassword = null;
if (con.encryptedTrustStorePassword != null) {
trustStorePassword = SecureStringUtil.getInstance().getDecryptedString(con.encryptedTrustStorePassword);
}

String hostNameInCertificate = con.activeConnectionProperties
.getProperty(SQLServerDriverStringProperty.HOSTNAME_IN_CERTIFICATE.toString());

Expand Down Expand Up @@ -1656,7 +1651,7 @@ else if (con.getTrustManagerClass() != null) {
// If we are using the system default trustStore and trustStorePassword
// then we can skip all of the KeyStore loading logic below.
// The security provider's implementation takes care of everything for us.
if (null == trustStoreFileName && null == trustStorePassword && !isTDSS) {
if (null == trustStoreFileName && null == con.encryptedTrustStorePassword && !isTDSS) {
if (logger.isLoggable(Level.FINER)) {
logger.finer(toString() + " Using system default trust store and password");
}
Expand All @@ -1683,9 +1678,13 @@ else if (con.getTrustManagerClass() != null) {
if (logger.isLoggable(Level.FINEST))
logger.finest(toString() + " Loading key store");

char[] trustStorePassword = SecureStringUtil.getInstance()
.getDecryptedChars(con.encryptedTrustStorePassword);
try {
ks.load(is, (null == trustStorePassword) ? null : trustStorePassword.toCharArray());
ks.load(is, (null == trustStorePassword) ? null : trustStorePassword);
} finally {
if (trustStorePassword != null)
Arrays.fill(trustStorePassword, ' ');
// We are also done with the trust store input stream.
if (null != is) {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ public class SQLServerConnection implements ISQLServerConnection, java.io.Serial
/** flag indicating whether prelogin TLS handshake is required */
private boolean isTDSS = false;

String encryptedTrustStorePassword = null;
byte[] encryptedTrustStorePassword = null;

/**
* Return an existing cached SharedTimer associated with this Connection or create a new one.
Expand Down Expand Up @@ -1844,7 +1844,8 @@ Connection connectInternal(Properties propsIn,
String trustStorePassword = activeConnectionProperties
.getProperty(SQLServerDriverStringProperty.TRUST_STORE_PASSWORD.toString());
if (trustStorePassword != null) {
encryptedTrustStorePassword = SecureStringUtil.getInstance().getEncryptedString(trustStorePassword);
encryptedTrustStorePassword = SecureStringUtil.getInstance()
.getEncryptedBytes(trustStorePassword.toCharArray());
activeConnectionProperties.remove(SQLServerDriverStringProperty.TRUST_STORE_PASSWORD.toString());
}

Expand Down
27 changes: 18 additions & 9 deletions src/main/java/com/microsoft/sqlserver/jdbc/SecureStringUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import java.security.SecureRandom;
import java.text.MessageFormat;
import java.util.Base64;
import java.util.Arrays;

import javax.crypto.Cipher;
import javax.crypto.KeyGenerator;
Expand Down Expand Up @@ -88,24 +88,26 @@ private SecureStringUtil() throws SQLServerException {
/**
* Get encrypted value of given string
*
* @param str
* @param chars
* string to encrypt
*
* @return encrypted string
*
* @throws SQLServerException
* if error
*/
String getEncryptedString(String str) throws SQLServerException {
byte[] getEncryptedBytes(char[] chars) throws SQLServerException {
if (chars == null)
return null;
SecureRandom random = new SecureRandom();
random.nextBytes(iv);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very fragile. Out of order encrypt-decrypt calls will not work because iv is stored in instance variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmimica Thanks for catching that! #1858 should address this.

GCMParameterSpec ivParamSpec = new GCMParameterSpec(TAG_LENGTH * 8, iv);

try {
encryptCipher.init(Cipher.ENCRYPT_MODE, secretKey, ivParamSpec);

byte[] cipherText = encryptCipher.doFinal(str.getBytes());
return Base64.getEncoder().encodeToString(cipherText);
byte[] cipherText = encryptCipher.doFinal(Util.charsToBytes(chars));
return cipherText;
} catch (Exception e) {
MessageFormat form = new MessageFormat(SQLServerException.getErrString("R_EncryptionFailed"));
Object[] msgArgs = {e.getMessage()};
Expand All @@ -116,24 +118,31 @@ String getEncryptedString(String str) throws SQLServerException {
/**
* Get decrypted value of an encrypted string
*
* @param str
* @param bytes
*
* @return decrypted string
*
* @throws SQLServerException
*/
String getDecryptedString(String str) throws SQLServerException {
char[] getDecryptedChars(byte[] bytes) throws SQLServerException {
if (bytes == null)
return null;
GCMParameterSpec ivParamSpec = new GCMParameterSpec(TAG_LENGTH * 8, iv);

byte[] plainText = null;
try {
decryptCipher.init(Cipher.DECRYPT_MODE, secretKey, ivParamSpec);

byte[] plainText = decryptCipher.doFinal(Base64.getDecoder().decode(str));
return new String(plainText);
plainText = decryptCipher.doFinal(bytes);
return Util.bytesToChars(plainText);
} catch (Exception e) {
MessageFormat form = new MessageFormat(SQLServerException.getErrString("R_DecryptionFailed"));
Object[] msgArgs = {e.getMessage()};
throw new SQLServerException(this, form.format(msgArgs), null, 0, false);
} finally {
if (plainText != null) {
Arrays.fill(plainText, (byte) 0);
}
}
}
}
21 changes: 21 additions & 0 deletions src/main/java/com/microsoft/sqlserver/jdbc/Util.java
Original file line number Diff line number Diff line change
Expand Up @@ -1006,6 +1006,27 @@ static String convertInputStreamToString(java.io.InputStream is) throws IOExcept
static String zeroOneToYesNo(int i) {
return 0 == i ? "NO" : "YES";
}

static byte[] charsToBytes(char[] chars) {
if (chars == null)
return null;
byte[] bytes = new byte[chars.length * 2];
for (int i = 0; i < chars.length; i++) {
bytes[i * 2] = (byte) (0xff & (chars[i] >> 8));
bytes[i * 2 + 1] = (byte) (0xff & (chars[i]));
}
return bytes;
}

static char[] bytesToChars(byte[] bytes) {
if (bytes == null)
return null;
char[] chars = new char[bytes.length / 2];
for (int i = 0; i < chars.length; i++) {
chars[i] = (char) (((0xFF & (bytes[i * 2])) << 8) | (0xFF & bytes[i * 2 + 1]));
}
return chars;
}
}


Expand Down
20 changes: 20 additions & 0 deletions src/test/java/com/microsoft/sqlserver/jdbc/UtilTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package com.microsoft.sqlserver.jdbc;

import static org.junit.Assert.assertEquals;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;

import java.sql.SQLException;
import java.util.Properties;
Expand Down Expand Up @@ -64,6 +65,25 @@ public void testparseUrl() throws SQLException {
assertEquals(prt.getProperty("password"), "pasS}");
}

private static String testString = "A ß € 嗨 𝄞 🙂ăѣ𝔠ծềſģȟᎥ𝒋ǩľḿꞑȯ𝘱𝑞𝗋𝘴ȶ𝞄𝜈ψ𝒙𝘆𝚣1234567890!@#$%^&*()-_=+[{]};:'\",<.>/?~𝘈Ḇ𝖢𝕯٤ḞԍНǏ𝙅ƘԸⲘ𝙉০Ρ𝗤Ɍ𝓢ȚЦ𝒱Ѡ𝓧ƳȤѧᖯć𝗱ễ𝑓𝙜Ⴙ𝞲𝑗𝒌ļṃʼnо𝞎𝒒ᵲꜱ𝙩ừ𝗏ŵ𝒙𝒚ź1234567890!@#$%^&*()-_=+[{]};:'\",<.>/?~АḂⲤ𝗗𝖤𝗙ꞠꓧȊ𝐉𝜥ꓡ𝑀𝑵Ǭ𝙿𝑄Ŗ𝑆𝒯𝖴𝘝𝘞ꓫŸ𝜡ả𝘢ƀ𝖼ḋếᵮℊ𝙝Ꭵ𝕛кιṃդⱺ𝓅𝘲𝕣𝖘ŧ𝑢ṽẉ𝘅ყž1234567890!@#$%^&*()-_=+[{]};:'\",<.>/?~Ѧ𝙱ƇᗞΣℱԍҤ١𝔍К𝓛𝓜ƝȎ𝚸𝑄Ṛ𝓢ṮṺƲᏔꓫ𝚈𝚭𝜶Ꮟçძ𝑒𝖿𝗀ḧ𝗂𝐣ҝɭḿ𝕟𝐨𝝔𝕢ṛ𝓼тú𝔳ẃ⤬𝝲𝗓1234567890!@#$%^&*()-_=+[{]};:'\",<.>/?~𝖠Β𝒞𝘋𝙴𝓕ĢȞỈ𝕵ꓗʟ𝙼ℕ০𝚸𝗤ՀꓢṰǓⅤ𝔚Ⲭ𝑌𝙕𝘢𝕤";

@Test
public void testArrayConversions() {
char[] chars = testString.toCharArray();
byte[] bytes = Util.charsToBytes(chars);
char[] newChars = Util.bytesToChars(bytes);
assertArrayEquals(chars, newChars);
String end = String.valueOf(newChars);
assertEquals(testString, end);
}

@Test
public void testSecureStringUtil() throws SQLException {
byte[] bytes = SecureStringUtil.getInstance().getEncryptedBytes(testString.toCharArray());
String end = String.valueOf(SecureStringUtil.getInstance().getDecryptedChars(bytes));
assertEquals(testString, end);
}

private void writeAndReadLong(long valueToTest) {
byte[] buffer = new byte[8];
Util.writeLong(valueToTest, buffer, 0);
Expand Down