Adding Keystore Provider and Keyhandle for Knox by Team Vulcans Limes (D) #9
Conversation
noah-pe
commented
Jun 3, 2024
noah-pe
commented
- Implemented provider.rs and key_handle.rs for Knox Vault
- Implemented create_instance for KnoxProvider
- Together with Team Netwatch (Android) we changed Box to Box
- added Java Files for our Wrapper
- added CreationError as a new SecurityModuleError
# Conflicts: # src/tpm/android/knox/provider.rs
# Conflicts: # Documentation/Doc.md
Removed all other keysizes besides 2048 because the device does not support them.
|
@ngussek ready for pull |
| IOException, NoSuchAlgorithmException, NoSuchProviderException, | ||
| InvalidAlgorithmParameterException, KeyStoreException { | ||
| String[] keyGenInfoArr = keyGenInfo.split(";"); | ||
| String KEY_ALGORITHM = keyGenInfoArr[0]; |
There was a problem hiding this comment.
You are assuming that there will always be 4 entries available. If this is not true, then an exception will be thrown. If the exception is not caught, then the software will close, which can be seen as a denial-of-service. Please check that keyGenInfoArr contains at least 4 entries here. Also, if any value used to create keyGenInfo contained a ';' then injections would be possible. See https://owasp.org/Top10/A03_2021-Injection/
| InvalidAlgorithmParameterException, KeyStoreException { | ||
| String[] keyGenInfoArr = keyGenInfo.split(";"); | ||
| String KEY_ALGORITHM = keyGenInfoArr[0]; | ||
| int KEY_SIZE = Integer.parseInt(keyGenInfoArr[1]); |
There was a problem hiding this comment.
You are assuming keyGenInfoArray[1],keyGenInfoArray[2] and keyGenInfoArray[3] contain data in the correct format. If this turns out not to be the case, then you'll end up with rather difficult problems to debug.
| String PADDING = keyGenInfoArr[3]; | ||
|
|
||
| KEY_NAME = key_id; | ||
| keyStore.load(null); |
There was a problem hiding this comment.
We need to load the keystore before storing the key inside of it, otherwise the keystore will throw an exception.
We set KEY_NAME = key_id, as we thought it will enhance readablillity, future-proofing and conformity.
Does this answer your question?
| KeyStoreException, IllegalBlockSizeException, BadPaddingException, InvalidKeySpecException, | ||
| NoSuchProviderException { | ||
|
|
||
| keyStore.load(null); |
There was a problem hiding this comment.
We need to load the keystore before using the needed key inside of it, if we dont the keystore will throw an exception.
Does this answer your question?
| byte[] iv = cipher.getIV(); | ||
| if (TRANSFORMATION.contains("/GCM/")) { | ||
| assert iv.length == 12; // GCM standard IV size is 12 Byte | ||
| } else if(TRANSFORMATION.contains("DESede")) { |
There was a problem hiding this comment.
The method description mentions GCM, CBC and CTR. Is DES support required?
| byteBuffer.get(iv); | ||
| encryptedData = new byte[byteBuffer.remaining()]; | ||
| byteBuffer.get(encryptedData); | ||
| GCMParameterSpec gcmParameterSpec = new GCMParameterSpec(128, iv); // 128 is the recommended TagSize |
There was a problem hiding this comment.
Refactor magic value into constant.
| SecretKey secretKey = (SecretKey) key; | ||
| SecretKeyFactory factory = SecretKeyFactory.getInstance(secretKey.getAlgorithm(), ANDROID_KEY_STORE); | ||
| keyInfo = (KeyInfo) factory.getKeySpec(secretKey, KeyInfo.class); | ||
| keyPadding = keyInfo.getEncryptionPaddings()[0]; |
There was a problem hiding this comment.
Ensure that paddings are actually available.
| PrivateKey privateKey = (PrivateKey) key; | ||
| KeyFactory factory = KeyFactory.getInstance(privateKey.getAlgorithm(), ANDROID_KEY_STORE); | ||
| keyInfo = factory.getKeySpec(privateKey, KeyInfo.class); | ||
| keyPadding = keyInfo.getSignaturePaddings()[0]; |
There was a problem hiding this comment.
Ensure that paddings are actually available.
| } else { | ||
| throw new KeyStoreException("Unsupported key type"); | ||
| } | ||
| return keyAlgorithm + "/" + keyInfo.getBlockModes()[0] + "/" + keyPadding; |
There was a problem hiding this comment.
Ensure that getBlockModes() contain at least one array entry.
| InvalidKeySpecException { | ||
| KeyFactory keyFactory = KeyFactory.getInstance(privateKey.getAlgorithm(), ANDROID_KEY_STORE); | ||
| KeyInfo keyInfo = keyFactory.getKeySpec(privateKey, KeyInfo.class); | ||
| String hashAlgorithm = keyInfo.getDigests()[0].replaceAll("-", ""); |
There was a problem hiding this comment.
Ensure that at least one digest exists.
There was a problem hiding this comment.
https://security.snyk.io/package/npm/jquery/3.6.1
Although no vulnerabilities in file, as part of security best practise it would be best to update to latest version - if this (javadocs) will be deployed somewhere that external people can access, and if there is a sensible way to update javadoc dependencies without having to edit code (updating javadoc version perhaps). If updating javadoc does not help, then let's rather move for acceptance on this matter - no point in trying to hack an upgrade.
src/tpm/android/knox/interface.rs
Outdated
There was a problem hiding this comment.
Something to think about: https://developer.android.com/training/articles/perf-jni
Quoting:
The JNIEnv is used for thread-local storage. For this reason, you cannot share a JNIEnv between threads. If a piece of code has no other way to get its JNIEnv, you should share the JavaVM, and use GetEnv to discover the thread's JNIEnv. (Assuming it has one; see AttachCurrentThread below.)
No code changes needed, but may be useful to mention this in a readme or doc.
Changed the IV constants to be constants in the code and not magic numbers. Updated JavaDoc to include the special case of IVs when using 3DES with CBC.
Pentest feedback implemented, updated JavaDoc to fit current code.
generateKeyPair EC and RSA RegEx fixed
