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

IllegalArgumentException getting a master key due to localization #1879

Closed
dbwiddis opened this issue Dec 1, 2023 · 1 comment · Fixed by #1880
Closed

IllegalArgumentException getting a master key due to localization #1879

dbwiddis opened this issue Dec 1, 2023 · 1 comment · Fixed by #1880

Comments

@dbwiddis
Copy link
Contributor

dbwiddis commented Dec 1, 2023

Problem:

Coming from opensearch-project/flow-framework#233 (comment)

The code in JceMasterKey.getInstance() does an uppercase conversion without locale information:

public static JceMasterKey getInstance(
final SecretKey key,
final String provider,
final String keyId,
final String wrappingAlgorithm) {
switch (wrappingAlgorithm.toUpperCase()) {
case "AES/GCM/NOPADDING":
return new JceMasterKey(provider, keyId, JceKeyCipher.aesGcm(key));
default:
throw new IllegalArgumentException("Right now only AES/GCM/NoPadding is supported");
}
}

When operating in some locales, the case check fails. For example, locale tr-TR converts the "i" in padding to this unicode character.

This is noted here:

For instance with the Turkish language, when converting the small letter 'i' to upper case, the result is capital letter 'I' with a dot over it.

Solution:

The toUppercase() call in the above code (and everywhere, really) should specify Locale.ROOT.

String WRAPPING_ALGORITHM = "AES/GCM/NoPadding";
String t1 = WRAPPING_ALGORITHM.toUpperCase();
String t2 = WRAPPING_ALGORITHM.toUpperCase(Locale.ROOT);

t1 in hex: 41 45 53 2F 47 43 4D 2F 4E 4F 50 41 44 44 C4 B0 4E 47
t2 in hex: 41 45 53 2F 47 43 4D 2F 4E 4F 50 41 44 44 49 4E 47

@dbwiddis
Copy link
Contributor Author

dbwiddis commented Dec 8, 2023

Note all these tests fail if run in TR locale:

/**
* Calls JceMasterKey.getInstance with differently cased wrappingAlgorithm names. Passes if no
* Exception is thrown. Relies on passing an invalid algorithm name to result in an Exception.
*/
@Test
public void testGetInstanceAllLowercase() {
jceGetInstance("aes/gcm/nopadding");
}
@Test
public void testGetInstanceMixedCasing() {
jceGetInstance("AES/GCm/NOpadding");
}
@Test
public void testGetInstanceAsymmetricAllLowercase() {
jceGetInstanceAsymmetric("rsa/ecb/oaepwithsha-256andmgf1padding");
}
@Test
public void testGetInstanceAsymmetricMixedCasing() {
jceGetInstanceAsymmetric("RSA/ECB/OAepwithsha-256andmgf1padding");
}

seebees pushed a commit that referenced this issue Dec 12, 2023
Adds Locale.ROOT to all toUpperCase() string conversions.

See #1879 for more details

---------

Signed-off-by: Daniel Widdis <widdis@gmail.com>
Co-authored-by: texastony <5892063+texastony@users.noreply.github.com>
josecorella pushed a commit that referenced this issue Jun 12, 2024
## [3.0.1](v3.0.0...v3.0.1) (2024-06-12)

### Fixes

* Add Locale.ROOT to String uppercase conversions ([#1880](#1880)) ([9a9950e](9a9950e)), closes [#1879](#1879)
* Update DecryptionMaterials code to support legacy custom CMMs ([#2037](#2037)) ([8807d79](8807d79))

### Maintenance

* deprecate getMasterKeyIds() in CryptoResult ([#1976](#1976)) ([1890ebb](1890ebb))
* **deps:** bump bcprov-jdk18on from 1.77 to 1.78.1 ([#2032](#2032)) ([713ca11](713ca11))
* **deps:** udpate org.bouncycastle to bcprov-jdk18on ([#1891](#1891)) ([32a92a9](32a92a9))
* **deps:** update dependencies ([#1973](#1973)) ([800bd01](800bd01))
* **Examples:** Customize KMS Client ([#2001](#2001)) ([e94ee85](e94ee85))
* fix release script ([#1912](#1912)) ([57e8a0b](57e8a0b))
* **README:** update README.md ([#1940](#1940)) ([7a0899e](7a0899e))
* update node version in version step ([#1959](#1959)) ([905385d](905385d))
* Update SUPPORT_POLICY.rst ([#1924](#1924)) ([57e40b5](57e40b5))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant