From c71df6902597215839b7d59e9a32caf9b96dd683 Mon Sep 17 00:00:00 2001 From: binsky Date: Thu, 29 Oct 2026 16:36:35 +0000 Subject: [PATCH 01/13] reject SJCL cryptograms with missing or invalid values instead of crashing The native decryption dereferenced the JSON values without checking them, so a cryptogram without e.g. "iv", or with a value of the wrong type, crashed the whole app with a segmentation fault instead of throwing an exception. Numbers out of the int range were cast to int, which is undefined behavior. Missing values, values of the wrong type and numbers that are no positive int now let the decryption fail with the usual exception. The JSON tree is also released if the cryptogram isn't a JSON object. Signed-off-by: binsky --- .../offline/SjclInputValidationTest.java | 119 ++++++++++++++++++ app/src/main/cpp/SJCL.cpp | 66 +++++++--- 2 files changed, 171 insertions(+), 14 deletions(-) create mode 100644 app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java diff --git a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java new file mode 100644 index 00000000..4741c8c9 --- /dev/null +++ b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java @@ -0,0 +1,119 @@ +/** + * Passman Android App + * + * @copyright Copyright (c) 2026, Timo Triebensky (timo@binsky.org) + * @license GNU AGPL version 3 or any later version + *

+ * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + *

+ * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + *

+ * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package es.wolfi.app.passman.offline; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; + +import android.util.Base64; + +import androidx.test.ext.junit.runners.AndroidJUnit4; + +import org.json.JSONObject; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.nio.charset.StandardCharsets; + +import es.wolfi.app.passman.SJCLCrypto; + +/** + * Cryptograms come from the server or the offline cache, so the native SJCL implementation has to reject + * malformed ones with an exception instead of crashing the app. + */ +@RunWith(AndroidJUnit4.class) +public class SjclInputValidationTest { + private static final String PASSWORD = "vault password"; + private static final String PLAINTEXT = "secret"; + private static final String[] STRING_FIELDS = {"iv", "adata", "salt", "ct"}; + private static final String[] NUMBER_FIELDS = {"iter", "ks", "ts"}; + + private JSONObject parameters; + + @Before + public void encrypt() throws Exception { + parameters = new JSONObject(new String( + Base64.decode(SJCLCrypto.encryptString(PLAINTEXT, PASSWORD, true), Base64.DEFAULT), + StandardCharsets.UTF_8 + )); + } + + @Test + public void unmodifiedCryptogramIsDecrypted() throws Exception { + assertEquals(PLAINTEXT, SJCLCrypto.decryptString(cryptogram(parameters.toString()), PASSWORD)); + } + + @Test + public void noJsonObjectIsRejected() { + for (String json : new String[]{"no json", "[]", "\"text\"", "42"}) { + assertRejected(json, json); + } + } + + @Test + public void missingFieldIsRejected() throws Exception { + for (String field : concat(STRING_FIELDS, NUMBER_FIELDS)) { + JSONObject manipulated = new JSONObject(parameters.toString()); + manipulated.remove(field); + assertRejected("without " + field, manipulated.toString()); + } + } + + @Test + public void stringFieldOfWrongTypeIsRejected() throws Exception { + for (String field : STRING_FIELDS) { + for (Object value : new Object[]{1, true, JSONObject.NULL, new JSONObject()}) { + JSONObject manipulated = new JSONObject(parameters.toString()).put(field, value); + assertRejected(field + ": " + value, manipulated.toString()); + } + } + } + + @Test + public void numberFieldOfWrongTypeOrRangeIsRejected() throws Exception { + for (String field : NUMBER_FIELDS) { + for (Object value : new Object[]{"1000", true, JSONObject.NULL, 0, -1, 1.5, 1e300, -1e300, 2147483648L}) { + JSONObject manipulated = new JSONObject(parameters.toString()).put(field, value); + assertRejected(field + ": " + value, manipulated.toString()); + } + } + } + + private static void assertRejected(String message, String json) { + assertThrows(message, Exception.class, () -> SJCLCrypto.decryptStringCpp(cryptogram(json), PASSWORD)); + } + + private static String cryptogram(String json) { + return Base64.encodeToString(json.getBytes(StandardCharsets.UTF_8), Base64.NO_WRAP); + } + + /** + * A new array with the elements of both arrays, e.g. to check all string and number fields in one loop. + * Java has no built-in array concatenation, and Stream.concat would need API 24 (minSdk is 23). + */ + private static String[] concat(String[] first, String[] second) { + String[] result = new String[first.length + second.length]; + System.arraycopy(first, 0, result, 0, first.length); + System.arraycopy(second, 0, result, first.length, second.length); + return result; + } +} diff --git a/app/src/main/cpp/SJCL.cpp b/app/src/main/cpp/SJCL.cpp index 77b0a872..75769e4d 100644 --- a/app/src/main/cpp/SJCL.cpp +++ b/app/src/main/cpp/SJCL.cpp @@ -31,6 +31,8 @@ #include #include #include +#include +#include #define LOG_TAG "SJCL" @@ -232,6 +234,34 @@ char* wstring_to_char(wstring str) { return c; } +/** + * The string value of the key, nullptr if the key is missing or its value is no string. + * Replaces raw access like with food[L"iv"]. + */ +const wstring* getString(const JSONObject& object, const wchar_t* key) { + auto value = object.find(key); + if (value == object.end() || ! value->second->IsString()) { + return nullptr; + } + return &value->second->AsString(); +} + +/** + * The value of the key as positive int, 0 if the key is missing or its value is no such number. + */ +int getPositiveInt(const JSONObject& object, const wchar_t* key) { + auto value = object.find(key); + if (value == object.end() || ! value->second->IsNumber()) { + return 0; + } + double number = value->second->AsNumber(); + // the cast of a number out of the int range would be undefined behavior + if (! (number >= 1 && number <= std::numeric_limits::max()) || number != std::floor(number)) { + return 0; + } + return (int) number; +} + using namespace WLF::Crypto; char* SJCL::decrypt(const string& sjcl_json, const string& key) { @@ -239,25 +269,33 @@ char* SJCL::decrypt(const string& sjcl_json, const string& key) { if (data == nullptr || ! data->IsObject()) { __android_log_write(ANDROID_LOG_ERROR, LOG_TAG, "Error parsing the SJCL JSON"); + delete data; return nullptr; } - JSONObject food = data->AsObject(); - - int iter = 0, - key_size = 0, - tag_size = 0; - char *iv_64, *salt_64, *cyphertext, *adata; + const JSONObject& food = data->AsObject(); // Extract the requiered values from the JSON - iv_64 = wstring_to_char(food[L"iv"]->AsString()); - adata = wstring_to_char(food[L"adata"]->AsString()); - salt_64 = wstring_to_char(food[L"salt"]->AsString()); - cyphertext = wstring_to_char(food[L"ct"]->AsString()); - - iter = (int) food[L"iter"]->AsNumber(); - key_size = (int) food[L"ks"]->AsNumber(); - tag_size = (int) food[L"ts"]->AsNumber(); + const wstring *iv_value = getString(food, L"iv"), + *adata_value = getString(food, L"adata"), + *salt_value = getString(food, L"salt"), + *ct_value = getString(food, L"ct"); + + int iter = getPositiveInt(food, L"iter"), + key_size = getPositiveInt(food, L"ks"), + tag_size = getPositiveInt(food, L"ts"); + + if (iv_value == nullptr || adata_value == nullptr || salt_value == nullptr || ct_value == nullptr + || iter == 0 || key_size == 0 || tag_size == 0) { + __android_log_write(ANDROID_LOG_ERROR, LOG_TAG, "The SJCL JSON lacks a required value"); + delete data; + return nullptr; + } + + char *iv_64 = wstring_to_char(*iv_value), + *adata = wstring_to_char(*adata_value), + *salt_64 = wstring_to_char(*salt_value), + *cyphertext = wstring_to_char(*ct_value); tag_size = (tag_size +7) / 8; // Make it bytes! key_size = (key_size +7) / 8; // Make it bytes From 49d9fb40bf06b1600d8ec2ec41b2a26a898aa258 Mon Sep 17 00:00:00 2001 From: binsky Date: Thu, 29 Oct 2026 19:47:35 +0000 Subject: [PATCH 02/13] reject (unsupported) SJCL cryptograms with key sizes other than 256 bit; the result of the PBKDF2 key derivation is now checked as well, so a failed derivation can no longer encrypt or decrypt with an uninitialized key Signed-off-by: binsky --- .../offline/SjclInputValidationTest.java | 9 +++++ app/src/main/cpp/SJCL.cpp | 36 +++++++++++++------ 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java index 4741c8c9..e5d4c9a3 100644 --- a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java +++ b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java @@ -98,6 +98,15 @@ public void numberFieldOfWrongTypeOrRangeIsRejected() throws Exception { } } + @Test + public void unsupportedKeySizeIsRejected() throws Exception { + // the web client and the app only use AES-256 + for (int keySize : new int[]{128, 192, 1, 512, Integer.MAX_VALUE}) { + JSONObject manipulated = new JSONObject(parameters.toString()).put("ks", keySize); + assertRejected("ks: " + keySize, manipulated.toString()); + } + } + private static void assertRejected(String message, String json) { assertThrows(message, Exception.class, () -> SJCLCrypto.decryptStringCpp(cryptogram(json), PASSWORD)); } diff --git a/app/src/main/cpp/SJCL.cpp b/app/src/main/cpp/SJCL.cpp index 75769e4d..eae9f302 100644 --- a/app/src/main/cpp/SJCL.cpp +++ b/app/src/main/cpp/SJCL.cpp @@ -292,6 +292,13 @@ char* SJCL::decrypt(const string& sjcl_json, const string& key) { return nullptr; } + // The cipher is always AES-256, it would read a smaller key past its end + if (key_size != 256) { + __android_log_write(ANDROID_LOG_ERROR, LOG_TAG, "Unsupported key size, only 256 bit keys are supported"); + delete data; + return nullptr; + } + char *iv_64 = wstring_to_char(*iv_value), *adata = wstring_to_char(*adata_value), *salt_64 = wstring_to_char(*salt_value), @@ -315,13 +322,16 @@ char* SJCL::decrypt(const string& sjcl_json, const string& key) { unsigned char* plaintext = (unsigned char*) std::calloc(cryptogram_length +1, sizeof(unsigned char)); string s = string("The allocated string is: ") + string((char*)plaintext); - /* PBKDF2 Key derivation with SHA256 as SJCL does by default */ - PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt->data, salt->length, iter, EVP_sha256(), key_size, derived_key); - char* ret = nullptr; - // Decrypt the data - int plaintext_len = decryptccm(cryptogram->data, cyphertext_data_length, (unsigned char *) adata, strlen(adata), - &cryptogram->data[cyphertext_data_length], derived_key, iv_raw->data, plaintext); + int plaintext_len = -1; + /* PBKDF2 Key derivation with SHA256 as SJCL does by default */ + if (1 == PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt->data, salt->length, iter, EVP_sha256(), key_size, derived_key)) { + // Decrypt the data + plaintext_len = decryptccm(cryptogram->data, cyphertext_data_length, (unsigned char *) adata, strlen(adata), + &cryptogram->data[cyphertext_data_length], derived_key, iv_raw->data, plaintext); + } else { + handleErrors("Error deriving the key"); + } if (0 < plaintext_len) { Datagram* plaintext_base64 = BASE64::encode(plaintext, plaintext_len); @@ -399,14 +409,18 @@ char* SJCL::encrypt(char* plaintext, const string& key) { RAND_bytes(iv, iv_len); RAND_bytes(salt, salt_len); - // PBKDF2 Key derivation with SHA256 as SJCL does by default - PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt, salt_len, iter, EVP_sha256(), ks, derived_key); - // Assuming ciphertext will not be bigger that the plaintext length * ciphertext_allocation_multiplicator ciphertext = (unsigned char *) malloc(sizeof(unsigned char) * strlen(plaintext) * ciphertext_allocation_multiplicator); - unsigned char *tmp_plaintext = reinterpret_cast(plaintext); - int ciphertext_len = encryptccm(tmp_plaintext, strlen(plaintext), additional, strlen ((char *)additional), derived_key, iv, iv_len, ciphertext, tag, ts); + int ciphertext_len = -1; + // PBKDF2 Key derivation with SHA256 as SJCL does by default + if (1 == PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt, salt_len, iter, EVP_sha256(), ks, derived_key)) { + unsigned char *tmp_plaintext = reinterpret_cast(plaintext); + ciphertext_len = encryptccm(tmp_plaintext, strlen(plaintext), additional, strlen ((char *)additional), derived_key, iv, iv_len, ciphertext, tag, ts); + } else { + handleErrors("Error deriving the key"); + } + if (0 < ciphertext_len) { uint8_t *ciphertext_with_tag = static_cast(malloc(sizeof(char *) * (ciphertext_len + ts))); memcpy(ciphertext_with_tag, ciphertext, ciphertext_len); From 60bc63833063e2da53dc14b5b723a19e0821514d Mon Sep 17 00:00:00 2001 From: binsky Date: Thu, 29 Oct 2026 20:19:27 +0000 Subject: [PATCH 03/13] reject SJCL cryptograms with tag sizes other than 64 bit Signed-off-by: binsky --- .../passman/offline/SjclInputValidationTest.java | 9 +++++++++ app/src/main/cpp/SJCL.cpp | 13 ++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java index e5d4c9a3..74919b3e 100644 --- a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java +++ b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java @@ -107,6 +107,15 @@ public void unsupportedKeySizeIsRejected() throws Exception { } } + @Test + public void unsupportedTagSizeIsRejected() throws Exception { + // the web client and the app only use 64 bit tags + for (int tagSize : new int[]{32, 96, 128, 8, 1, Integer.MAX_VALUE}) { + JSONObject manipulated = new JSONObject(parameters.toString()).put("ts", tagSize); + assertRejected("ts: " + tagSize, manipulated.toString()); + } + } + private static void assertRejected(String message, String json) { assertThrows(message, Exception.class, () -> SJCLCrypto.decryptStringCpp(cryptogram(json), PASSWORD)); } diff --git a/app/src/main/cpp/SJCL.cpp b/app/src/main/cpp/SJCL.cpp index eae9f302..e5d1a763 100644 --- a/app/src/main/cpp/SJCL.cpp +++ b/app/src/main/cpp/SJCL.cpp @@ -41,7 +41,7 @@ void handleErrors(const char* error){ } int decryptccm(unsigned char *ciphertext, int ciphertext_len, unsigned char *aad, - int aad_len, unsigned char *tag, unsigned char *key, unsigned char *iv, + int aad_len, unsigned char *tag, int tag_len, unsigned char *key, unsigned char *iv, unsigned char *plaintext ) { EVP_CIPHER_CTX *ctx; @@ -68,7 +68,7 @@ int decryptccm(unsigned char *ciphertext, int ciphertext_len, unsigned char *aad } /* Set expected tag value. */ - if (1 != EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_CCM_SET_TAG, 8, tag)) { + if (1 != EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_CCM_SET_TAG, tag_len, tag)) { handleErrors("Error setting TAG value"); goto CLEANUP; } @@ -299,6 +299,13 @@ char* SJCL::decrypt(const string& sjcl_json, const string& key) { return nullptr; } + // Passman only uses 64 bit tags, a smaller one would be read past the end of the cryptogram + if (tag_size != 64) { + __android_log_write(ANDROID_LOG_ERROR, LOG_TAG, "Unsupported tag size, only 64 bit tags are supported"); + delete data; + return nullptr; + } + char *iv_64 = wstring_to_char(*iv_value), *adata = wstring_to_char(*adata_value), *salt_64 = wstring_to_char(*salt_value), @@ -328,7 +335,7 @@ char* SJCL::decrypt(const string& sjcl_json, const string& key) { if (1 == PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt->data, salt->length, iter, EVP_sha256(), key_size, derived_key)) { // Decrypt the data plaintext_len = decryptccm(cryptogram->data, cyphertext_data_length, (unsigned char *) adata, strlen(adata), - &cryptogram->data[cyphertext_data_length], derived_key, iv_raw->data, plaintext); + &cryptogram->data[cyphertext_data_length], tag_size, derived_key, iv_raw->data, plaintext); } else { handleErrors("Error deriving the key"); } From 3c3d59b10186325e14f3d48fcee597d683b7aa7f Mon Sep 17 00:00:00 2001 From: binsky Date: Thu, 29 Oct 2026 20:45:28 +0000 Subject: [PATCH 04/13] reject SJCL cryptograms with a too short ciphertext or IV note: the required IV length depends on the cryptogram length (13 bytes below 64 KB, 12 below 16 MB, 11 above), so decryptccm checks it where the nonce length is calculated Signed-off-by: binsky --- .../offline/SjclInputValidationTest.java | 24 +++++++++++++++++- app/src/main/cpp/SJCL.cpp | 25 +++++++++++++------ 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java index 74919b3e..3c9e965e 100644 --- a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java +++ b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java @@ -116,12 +116,34 @@ public void unsupportedTagSizeIsRejected() throws Exception { } } + @Test + public void cryptogramShorterThanTheTagIsRejected() throws Exception { + // the 64 bit tag takes 8 bytes + for (int length : new int[]{0, 1, 4, 7}) { + JSONObject manipulated = new JSONObject(parameters.toString()).put("ct", base64(new byte[length])); + assertRejected("ct of " + length + " bytes", manipulated.toString()); + } + } + + @Test + public void tooShortIvIsRejected() throws Exception { + // the nonce of a cryptogram below 64 KB takes 13 bytes + for (int length : new int[]{0, 1, 8, 12}) { + JSONObject manipulated = new JSONObject(parameters.toString()).put("iv", base64(new byte[length])); + assertRejected("iv of " + length + " bytes", manipulated.toString()); + } + } + private static void assertRejected(String message, String json) { assertThrows(message, Exception.class, () -> SJCLCrypto.decryptStringCpp(cryptogram(json), PASSWORD)); } private static String cryptogram(String json) { - return Base64.encodeToString(json.getBytes(StandardCharsets.UTF_8), Base64.NO_WRAP); + return base64(json.getBytes(StandardCharsets.UTF_8)); + } + + private static String base64(byte[] data) { + return Base64.encodeToString(data, Base64.NO_WRAP); } /** diff --git a/app/src/main/cpp/SJCL.cpp b/app/src/main/cpp/SJCL.cpp index e5d1a763..331f2ec5 100644 --- a/app/src/main/cpp/SJCL.cpp +++ b/app/src/main/cpp/SJCL.cpp @@ -42,7 +42,7 @@ void handleErrors(const char* error){ int decryptccm(unsigned char *ciphertext, int ciphertext_len, unsigned char *aad, int aad_len, unsigned char *tag, int tag_len, unsigned char *key, unsigned char *iv, - unsigned char *plaintext + int iv_len, unsigned char *plaintext ) { EVP_CIPHER_CTX *ctx; int len; @@ -53,6 +53,12 @@ int decryptccm(unsigned char *ciphertext, int ciphertext_len, unsigned char *aad if (ciphertext_len >= 1<<16) lol++; if (ciphertext_len >= 1<<24) lol++; + // OpenSSL reads the first 15-lol bytes of the IV as nonce + if (iv_len < 15-lol) { + handleErrors("The IV is too short"); + return -1; + } + /* Create and initialise the context */ if (!(ctx = EVP_CIPHER_CTX_new())) handleErrors("Error initializing context"); @@ -331,13 +337,18 @@ char* SJCL::decrypt(const string& sjcl_json, const string& key) { char* ret = nullptr; int plaintext_len = -1; - /* PBKDF2 Key derivation with SHA256 as SJCL does by default */ - if (1 == PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt->data, salt->length, iter, EVP_sha256(), key_size, derived_key)) { - // Decrypt the data - plaintext_len = decryptccm(cryptogram->data, cyphertext_data_length, (unsigned char *) adata, strlen(adata), - &cryptogram->data[cyphertext_data_length], tag_size, derived_key, iv_raw->data, plaintext); + // The cryptogram is the ciphertext followed by the tag, so it can't be shorter than the tag + if (0 <= cyphertext_data_length) { + /* PBKDF2 Key derivation with SHA256 as SJCL does by default */ + if (1 == PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt->data, salt->length, iter, EVP_sha256(), key_size, derived_key)) { + // Decrypt the data + plaintext_len = decryptccm(cryptogram->data, cyphertext_data_length, (unsigned char *) adata, strlen(adata), + &cryptogram->data[cyphertext_data_length], tag_size, derived_key, iv_raw->data, iv_raw->length, plaintext); + } else { + handleErrors("Error deriving the key"); + } } else { - handleErrors("Error deriving the key"); + handleErrors("The cryptogram is shorter than the tag"); } if (0 < plaintext_len) { From 970b65cc07d829fee65d1cb99e4412aab5d8ce23 Mon Sep 17 00:00:00 2001 From: binsky Date: Fri, 30 Oct 2026 11:00:41 +0000 Subject: [PATCH 05/13] reject SJCL cryptograms with undecodable base64 instead of crashing the decoding no longer exits the app when it runs out of memory, but returns NULL; it no longer leaks the Datagram struct when returning NULL Signed-off-by: binsky --- .../offline/SjclInputValidationTest.java | 16 +++++++ app/src/main/cpp/SJCL.cpp | 44 ++++++++++++------- app/src/main/cpp/base64.cpp | 6 ++- app/src/main/cpp/passman-lib.cpp | 7 +++ 4 files changed, 57 insertions(+), 16 deletions(-) diff --git a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java index 3c9e965e..475ae228 100644 --- a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java +++ b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java @@ -134,6 +134,22 @@ public void tooShortIvIsRejected() throws Exception { } } + @Test + public void invalidBase64IsRejected() throws Exception { + // a single character before the padding can't be decoded, + // unlike characters outside of the base64 alphabet, which the native base64 decoding skips + String[] invalid = {"A=", "AAAAA=="}; + for (String value : invalid) { + assertThrows(value, Exception.class, () -> SJCLCrypto.decryptStringCpp(value, PASSWORD)); + } + for (String field : new String[]{"ct", "salt", "iv"}) { + for (String value : invalid) { + JSONObject manipulated = new JSONObject(parameters.toString()).put(field, value); + assertRejected(field + ": " + value, manipulated.toString()); + } + } + } + private static void assertRejected(String message, String json) { assertThrows(message, Exception.class, () -> SJCLCrypto.decryptStringCpp(cryptogram(json), PASSWORD)); } diff --git a/app/src/main/cpp/SJCL.cpp b/app/src/main/cpp/SJCL.cpp index 331f2ec5..05221abd 100644 --- a/app/src/main/cpp/SJCL.cpp +++ b/app/src/main/cpp/SJCL.cpp @@ -270,6 +270,13 @@ int getPositiveInt(const JSONObject& object, const wchar_t* key) { using namespace WLF::Crypto; +void releaseDatagram(Datagram* datagram) { + if (datagram != nullptr) { + free(datagram->data); + delete datagram; + } +} + char* SJCL::decrypt(const string& sjcl_json, const string& key) { JSONValue *data = JSON::Parse(sjcl_json.c_str()); @@ -320,14 +327,27 @@ char* SJCL::decrypt(const string& sjcl_json, const string& key) { tag_size = (tag_size +7) / 8; // Make it bytes! key_size = (key_size +7) / 8; // Make it bytes - // The actual cryptogram includes the tag size, so we need to take this into account later on! Datagram* cryptogram = BASE64::decode((unsigned char *) cyphertext, strlen(cyphertext)); - int cyphertext_data_length = cryptogram->length - tag_size; - Datagram* salt = BASE64::decode((unsigned char *) salt_64, strlen(salt_64)); Datagram* iv_raw = BASE64::decode((unsigned char *) iv_64, strlen(iv_64)); // Datagram* aadata = BASE64::decode((const unsigned char *) "", 0); // Not sure if this is required since we don't use adata + if (cryptogram == nullptr || salt == nullptr || iv_raw == nullptr) { + __android_log_write(ANDROID_LOG_ERROR, LOG_TAG, "The SJCL JSON contains invalid base64"); + free(iv_64); + free(adata); + free(salt_64); + free(cyphertext); + delete data; + releaseDatagram(cryptogram); + releaseDatagram(salt); + releaseDatagram(iv_raw); + return nullptr; + } + + // The actual cryptogram includes the tag size, so we need to take this into account later on! + int cyphertext_data_length = cryptogram->length - tag_size; + unsigned char* derived_key = (unsigned char*) malloc(sizeof(unsigned char) * key_size); // Assuming plaintext will always be smaller than the sjcl cyphertext which includes the tag and padding and stuff @@ -377,12 +397,9 @@ char* SJCL::decrypt(const string& sjcl_json, const string& key) { free(salt_64); free(cyphertext); delete data; - free(cryptogram->data); - delete cryptogram; - free(salt->data); - delete salt; - free(iv_raw->data); - delete iv_raw; + releaseDatagram(cryptogram); + releaseDatagram(salt); + releaseDatagram(iv_raw); OPENSSL_cleanse(plaintext, cryptogram_length + 1); free(plaintext); OPENSSL_cleanse(derived_key, key_size); @@ -474,12 +491,9 @@ char* SJCL::encrypt(char* plaintext, const string& key) { ret = (char *) std::calloc(retrn.length() +1, sizeof(char)); strcpy(ret, retrn.c_str()); - free(ciphertext_base64->data); - delete ciphertext_base64; - free(salt_base64->data); - delete salt_base64; - free(iv_base64->data); - delete iv_base64; + releaseDatagram(ciphertext_base64); + releaseDatagram(salt_base64); + releaseDatagram(iv_base64); } // Free up resources, wipe the secrets before diff --git a/app/src/main/cpp/base64.cpp b/app/src/main/cpp/base64.cpp index 7c02dbe2..f4ea52a2 100644 --- a/app/src/main/cpp/base64.cpp +++ b/app/src/main/cpp/base64.cpp @@ -63,13 +63,15 @@ Datagram* BASE64::decode(const unsigned char *str, int length) { ret->data = (unsigned char *)malloc(length+1); if (ret->data == NULL) { fprintf(stderr, "out of memory!\n"); - exit(1); + delete ret; + return NULL; } while ((ch = *current++) != '\0' && length-- > 0) { if (ch == base64_pad) { if (*current != '=' && (i % 4) == 1) { free(ret->data); + delete ret; return NULL; } continue; @@ -80,6 +82,7 @@ Datagram* BASE64::decode(const unsigned char *str, int length) { continue; } else if (ch == -2) { free(ret->data); + delete ret; return NULL; } @@ -108,6 +111,7 @@ Datagram* BASE64::decode(const unsigned char *str, int length) { switch(i % 4) { case 1: free(ret->data); + delete ret; return NULL; case 2: k++; diff --git a/app/src/main/cpp/passman-lib.cpp b/app/src/main/cpp/passman-lib.cpp index dcb443c5..39d900c4 100644 --- a/app/src/main/cpp/passman-lib.cpp +++ b/app/src/main/cpp/passman-lib.cpp @@ -75,6 +75,13 @@ jstring Java_es_wolfi_app_passman_SJCLCrypto_decryptStringCpp(JNIEnv *env, jclas WLF::Crypto::Datagram *t = WLF::Crypto::BASE64::decode((const unsigned char *) dt.c_str(), dt.length()); + if (t == NULL) { + __android_log_write(ANDROID_LOG_ERROR, LOG_TAG, "error decoding the cryptogram"); + wipe(password); + throwJavaException(env, "Error decrypting"); + return NULL; + } + std::string json = (char *) t->data; free(t->data); delete t; From 8ccd4d877a44423aadb0b69e15e027da60204c5c Mon Sep 17 00:00:00 2001 From: binsky Date: Fri, 30 Oct 2026 11:04:22 +0000 Subject: [PATCH 06/13] add test to make sure a rejected decryption releases its buffers as well Signed-off-by: binsky --- .../offline/NativeCryptoMemoryTest.java | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/app/src/androidTest/java/es/wolfi/app/passman/offline/NativeCryptoMemoryTest.java b/app/src/androidTest/java/es/wolfi/app/passman/offline/NativeCryptoMemoryTest.java index d199d09f..bd1f8d62 100644 --- a/app/src/androidTest/java/es/wolfi/app/passman/offline/NativeCryptoMemoryTest.java +++ b/app/src/androidTest/java/es/wolfi/app/passman/offline/NativeCryptoMemoryTest.java @@ -21,16 +21,21 @@ package es.wolfi.app.passman.offline; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import android.os.Debug; +import android.util.Base64; import android.util.Log; import androidx.test.ext.junit.runners.AndroidJUnit4; +import org.json.JSONObject; import org.junit.Test; import org.junit.runner.RunWith; +import java.nio.charset.StandardCharsets; + import es.wolfi.app.passman.SJCLCrypto; /** @@ -82,6 +87,26 @@ public void encryptDoesNotLeakNativeMemory() throws Exception { assertGrowthBelowLimit("encrypt", before, Debug.getNativeHeapAllocatedSize()); } + @Test + public void rejectedDecryptionDoesNotLeakNativeMemory() throws Exception { + JSONObject parameters = new JSONObject(new String( + Base64.decode(SJCLCrypto.encryptString(PLAINTEXT, PASSWORD, true), Base64.DEFAULT), + StandardCharsets.UTF_8 + )); + // the large ct and the salt are decoded before the undecodable iv lets the decryption fail + parameters.put("iv", "A="); + String rejected = Base64.encodeToString(parameters.toString().getBytes(StandardCharsets.UTF_8), Base64.NO_WRAP); + for (int i = 0; i < 20; i++) { + assertThrows(Exception.class, () -> SJCLCrypto.decryptStringCpp(rejected, PASSWORD)); + } + + long before = Debug.getNativeHeapAllocatedSize(); + for (int i = 0; i < RUNS; i++) { + assertThrows(Exception.class, () -> SJCLCrypto.decryptStringCpp(rejected, PASSWORD)); + } + assertGrowthBelowLimit("rejected decrypt", before, Debug.getNativeHeapAllocatedSize()); + } + private static void assertGrowthBelowLimit(String operation, long before, long after) { long growth = after - before; Log.i(TAG, operation + ": native heap grew by " + growth + " bytes in " + RUNS + " runs"); From fed076180be753c2f10ba5df981ce350b1232171 Mon Sep 17 00:00:00 2001 From: binsky Date: Fri, 30 Oct 2026 16:38:45 +0000 Subject: [PATCH 07/13] let a failed base64 encoding fail the native crypto instead of the app the native base64 encoding exited the whole app when it ran out of memory, and returned a result without data for oversized input, which its callers didn't check. It now returns NULL in both cases, and the encryption and decryption fail with the usual exception instead Signed-off-by: binsky --- app/src/main/cpp/SJCL.cpp | 52 +++++++++++++++++--------------- app/src/main/cpp/base64.cpp | 8 ++--- app/src/main/cpp/passman-lib.cpp | 13 +++++--- 3 files changed, 40 insertions(+), 33 deletions(-) diff --git a/app/src/main/cpp/SJCL.cpp b/app/src/main/cpp/SJCL.cpp index 05221abd..c0cb9872 100644 --- a/app/src/main/cpp/SJCL.cpp +++ b/app/src/main/cpp/SJCL.cpp @@ -373,8 +373,12 @@ char* SJCL::decrypt(const string& sjcl_json, const string& key) { if (0 < plaintext_len) { Datagram* plaintext_base64 = BASE64::encode(plaintext, plaintext_len); - ret = reinterpret_cast(plaintext_base64->data); - delete plaintext_base64; + if (plaintext_base64 != nullptr) { + ret = reinterpret_cast(plaintext_base64->data); + delete plaintext_base64; + } else { + handleErrors("Error encoding the plaintext"); + } /* do decoding now in the java part * @@ -463,33 +467,33 @@ char* SJCL::encrypt(char* plaintext, const string& key) { Datagram* ciphertext_base64 = BASE64::encode( reinterpret_cast(ciphertext_with_tag), ciphertext_len + ts); - char *tmpct64 = reinterpret_cast(ciphertext_base64->data); free(ciphertext_with_tag); Datagram* salt_base64 = BASE64::encode(salt, salt_len); - char *tmpsalt64 = reinterpret_cast(salt_base64->data); - Datagram* iv_base64 = BASE64::encode(iv, iv_len); - char *tmpiv64 = reinterpret_cast(iv_base64->data); - - - nlohmann::json food; - food["ct"] = tmpct64; - food["salt"] = tmpsalt64; - food["iv"] = tmpiv64; - - food["v"] = 1; - food["iter"] = iter; - food["ks"] = key_size; - food["ts"] = tag_size; - food["mode"] = "ccm"; - food["adata"] = ""; - food["cipher"] = "aes"; - - string retrn = food.dump(); - ret = (char *) std::calloc(retrn.length() +1, sizeof(char)); - strcpy(ret, retrn.c_str()); + // ensure all three encodings worked before continuing + if (ciphertext_base64 != nullptr && salt_base64 != nullptr && iv_base64 != nullptr) { + nlohmann::json food; + + food["ct"] = reinterpret_cast(ciphertext_base64->data); + food["salt"] = reinterpret_cast(salt_base64->data); + food["iv"] = reinterpret_cast(iv_base64->data); + + food["v"] = 1; + food["iter"] = iter; + food["ks"] = key_size; + food["ts"] = tag_size; + food["mode"] = "ccm"; + food["adata"] = ""; + food["cipher"] = "aes"; + + string retrn = food.dump(); + ret = (char *) std::calloc(retrn.length() +1, sizeof(char)); + strcpy(ret, retrn.c_str()); + } else { + handleErrors("Error encoding the cryptogram"); + } releaseDatagram(ciphertext_base64); releaseDatagram(salt_base64); diff --git a/app/src/main/cpp/base64.cpp b/app/src/main/cpp/base64.cpp index f4ea52a2..ac1af2e6 100644 --- a/app/src/main/cpp/base64.cpp +++ b/app/src/main/cpp/base64.cpp @@ -131,15 +131,15 @@ Datagram* BASE64::encode(const unsigned char *str, int length) { unsigned char *p; if ((length + 2) < 0 || ((length + 2) / 3) >= (1 << (sizeof(int) * 8 - 2))) { - ret->data = NULL; - ret->length = 0; - return ret; + delete ret; + return NULL; } ret->data = (unsigned char *)malloc((((length + 2) / 3) * 4)*(sizeof(char))+(1)); if (ret->data == NULL) { fprintf(stderr, "out of memory!\n"); - exit(1); + delete ret; + return NULL; } p = ret->data; diff --git a/app/src/main/cpp/passman-lib.cpp b/app/src/main/cpp/passman-lib.cpp index 39d900c4..6376f1a4 100644 --- a/app/src/main/cpp/passman-lib.cpp +++ b/app/src/main/cpp/passman-lib.cpp @@ -130,13 +130,16 @@ jstring Java_es_wolfi_app_passman_SJCLCrypto_encryptStringCpp(JNIEnv *env, jclas //__android_log_write(ANDROID_LOG_ERROR, LOG_TAG, result); WLF::Crypto::Datagram *t = WLF::Crypto::BASE64::encode((const unsigned char *) result, strlen(arr_result)); + free(result); - jstring str = env->NewStringUTF((char *) t->data); + if (t != NULL) { + jstring str = env->NewStringUTF((char *) t->data); - free(t->data); - delete t; - free(result); - return str; + free(t->data); + delete t; + return str; + } + __android_log_write(ANDROID_LOG_ERROR, LOG_TAG, "error encoding the cryptogram"); } throwJavaException(env, "Error encrypting"); From a00886387e69a32b339a93264c1848b520c1a3f8 Mon Sep 17 00:00:00 2001 From: binsky Date: Fri, 30 Oct 2026 17:05:18 +0000 Subject: [PATCH 08/13] fix throwing the intended exception when the native encryption fails basically just by removing two useless lines :D Signed-off-by: binsky --- .../app/passman/offline/SjclInputValidationTest.java | 11 ++++++++++- app/src/main/cpp/passman-lib.cpp | 5 +---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java index 475ae228..5ec0db13 100644 --- a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java +++ b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java @@ -38,7 +38,7 @@ /** * Cryptograms come from the server or the offline cache, so the native SJCL implementation has to reject - * malformed ones with an exception instead of crashing the app. + * malformed ones with an exception instead of crashing the app. The same applies to a failed encryption. */ @RunWith(AndroidJUnit4.class) public class SjclInputValidationTest { @@ -150,6 +150,15 @@ public void invalidBase64IsRejected() throws Exception { } } + /** + * (it was a surprise it did not work before this fix commit) + */ + @Test + public void failedEncryptionThrowsAnException() { + // the native encryption fails for an empty plaintext, the app avoids this by JSON encoding it + assertThrows(Exception.class, () -> SJCLCrypto.encryptStringCpp("", PASSWORD)); + } + private static void assertRejected(String message, String json) { assertThrows(message, Exception.class, () -> SJCLCrypto.decryptStringCpp(cryptogram(json), PASSWORD)); } diff --git a/app/src/main/cpp/passman-lib.cpp b/app/src/main/cpp/passman-lib.cpp index 6376f1a4..c817f6b6 100644 --- a/app/src/main/cpp/passman-lib.cpp +++ b/app/src/main/cpp/passman-lib.cpp @@ -119,9 +119,6 @@ jstring Java_es_wolfi_app_passman_SJCLCrypto_encryptStringCpp(JNIEnv *env, jclas wipe(password); //__android_log_write(ANDROID_LOG_ERROR, LOG_TAG, result); - char *arr_result = &result[0]; - std::string lengthAsString = to_string(strlen(arr_result)); - if (result == NULL) { __android_log_write(ANDROID_LOG_ERROR, LOG_TAG, "error encrypting"); } @@ -129,7 +126,7 @@ jstring Java_es_wolfi_app_passman_SJCLCrypto_encryptStringCpp(JNIEnv *env, jclas //__android_log_write(ANDROID_LOG_ERROR, LOG_TAG, "WHOOP!"); //__android_log_write(ANDROID_LOG_ERROR, LOG_TAG, result); WLF::Crypto::Datagram *t = WLF::Crypto::BASE64::encode((const unsigned char *) result, - strlen(arr_result)); + strlen(result)); free(result); if (t != NULL) { From fa9dee191b97fe89504ff1d444ee764201403911 Mon Sep 17 00:00:00 2001 From: binsky Date: Fri, 30 Oct 2026 18:28:57 +0000 Subject: [PATCH 09/13] return null instead of a C string from the failed native decryption (to align with the encryptStringCpp behaviour) the input validation tests now check the exact exception and message, which are the same before and after this change Signed-off-by: binsky --- .../offline/SjclInputValidationTest.java | 24 +++++++++++++++++-- app/src/main/cpp/passman-lib.cpp | 2 +- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java index 5ec0db13..9be27695 100644 --- a/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java +++ b/app/src/androidTest/java/es/wolfi/app/passman/offline/SjclInputValidationTest.java @@ -30,6 +30,7 @@ import org.json.JSONObject; import org.junit.Before; import org.junit.Test; +import org.junit.function.ThrowingRunnable; import org.junit.runner.RunWith; import java.nio.charset.StandardCharsets; @@ -62,6 +63,12 @@ public void unmodifiedCryptogramIsDecrypted() throws Exception { assertEquals(PLAINTEXT, SJCLCrypto.decryptString(cryptogram(parameters.toString()), PASSWORD)); } + @Test + public void wrongPasswordThrowsTheDecryptionError() { + // the exception the app gets from the Java wrapper + assertError("Error decrypting", () -> SJCLCrypto.decryptString(cryptogram(parameters.toString()), PASSWORD + "x")); + } + @Test public void noJsonObjectIsRejected() { for (String json : new String[]{"no json", "[]", "\"text\"", "42"}) { @@ -156,11 +163,24 @@ public void invalidBase64IsRejected() throws Exception { @Test public void failedEncryptionThrowsAnException() { // the native encryption fails for an empty plaintext, the app avoids this by JSON encoding it - assertThrows(Exception.class, () -> SJCLCrypto.encryptStringCpp("", PASSWORD)); + assertError("Error encrypting", () -> SJCLCrypto.encryptStringCpp("", PASSWORD)); } private static void assertRejected(String message, String json) { - assertThrows(message, Exception.class, () -> SJCLCrypto.decryptStringCpp(cryptogram(json), PASSWORD)); + assertError(message, "Error decrypting", () -> SJCLCrypto.decryptStringCpp(cryptogram(json), PASSWORD)); + } + + private static void assertError(String expectedMessage, ThrowingRunnable runnable) { + assertError(null, expectedMessage, runnable); + } + + /** + * The native crypto reports every failure with a plain RuntimeException and a fixed message. + */ + private static void assertError(String message, String expectedMessage, ThrowingRunnable runnable) { + Exception exception = assertThrows(message, Exception.class, runnable); + assertEquals(message, RuntimeException.class, exception.getClass()); + assertEquals(message, expectedMessage, exception.getMessage()); } private static String cryptogram(String json) { diff --git a/app/src/main/cpp/passman-lib.cpp b/app/src/main/cpp/passman-lib.cpp index c817f6b6..898bd880 100644 --- a/app/src/main/cpp/passman-lib.cpp +++ b/app/src/main/cpp/passman-lib.cpp @@ -105,7 +105,7 @@ jstring Java_es_wolfi_app_passman_SJCLCrypto_decryptStringCpp(JNIEnv *env, jclas } throwJavaException(env, "Error decrypting"); - return (jstring) ""; + return NULL; } jstring Java_es_wolfi_app_passman_SJCLCrypto_encryptStringCpp(JNIEnv *env, jclass jthis, jstring plaintext, jstring key) { From 4f70228671088f8b8c7d2aaa7acd0fbf52a692e5 Mon Sep 17 00:00:00 2001 From: binsky Date: Fri, 30 Oct 2026 18:41:19 +0000 Subject: [PATCH 10/13] fix native decryptccm (error handling) flow for when the cipher context can't be created Signed-off-by: binsky --- app/src/main/cpp/SJCL.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/src/main/cpp/SJCL.cpp b/app/src/main/cpp/SJCL.cpp index c0cb9872..e7416397 100644 --- a/app/src/main/cpp/SJCL.cpp +++ b/app/src/main/cpp/SJCL.cpp @@ -60,7 +60,10 @@ int decryptccm(unsigned char *ciphertext, int ciphertext_len, unsigned char *aad } /* Create and initialise the context */ - if (!(ctx = EVP_CIPHER_CTX_new())) handleErrors("Error initializing context"); + if (!(ctx = EVP_CIPHER_CTX_new())) { + handleErrors("Error initializing context"); + goto CLEANUP; + } /* Initialise the decryption operation. */ if(1 != EVP_DecryptInit_ex(ctx, EVP_aes_256_ccm(), nullptr, nullptr, nullptr)) { From 401db70898ab718a9fd44569575805368b97b563 Mon Sep 17 00:00:00 2001 From: binsky Date: Fri, 30 Oct 2026 18:57:27 +0000 Subject: [PATCH 11/13] a failed random number generation now lets the encryption fail with an exception ( instead of encrypting with an IV and salt of leftover memory) Signed-off-by: binsky --- app/src/main/cpp/SJCL.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/app/src/main/cpp/SJCL.cpp b/app/src/main/cpp/SJCL.cpp index e7416397..5de66013 100644 --- a/app/src/main/cpp/SJCL.cpp +++ b/app/src/main/cpp/SJCL.cpp @@ -448,19 +448,21 @@ char* SJCL::encrypt(char* plaintext, const string& key) { salt = (unsigned char *) malloc(sizeof(unsigned char) * salt_len); derived_key = (unsigned char *) malloc(sizeof(unsigned char) * ks); - RAND_bytes(iv, iv_len); - RAND_bytes(salt, salt_len); - // Assuming ciphertext will not be bigger that the plaintext length * ciphertext_allocation_multiplicator ciphertext = (unsigned char *) malloc(sizeof(unsigned char) * strlen(plaintext) * ciphertext_allocation_multiplicator); int ciphertext_len = -1; - // PBKDF2 Key derivation with SHA256 as SJCL does by default - if (1 == PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt, salt_len, iter, EVP_sha256(), ks, derived_key)) { - unsigned char *tmp_plaintext = reinterpret_cast(plaintext); - ciphertext_len = encryptccm(tmp_plaintext, strlen(plaintext), additional, strlen ((char *)additional), derived_key, iv, iv_len, ciphertext, tag, ts); + // Without random bytes, the IV and salt would be leftover memory and could repeat between encryptions + if (1 == RAND_bytes(iv, iv_len) && 1 == RAND_bytes(salt, salt_len)) { + // PBKDF2 Key derivation with SHA256 as SJCL does by default + if (1 == PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt, salt_len, iter, EVP_sha256(), ks, derived_key)) { + unsigned char *tmp_plaintext = reinterpret_cast(plaintext); + ciphertext_len = encryptccm(tmp_plaintext, strlen(plaintext), additional, strlen ((char *)additional), derived_key, iv, iv_len, ciphertext, tag, ts); + } else { + handleErrors("Error deriving the key"); + } } else { - handleErrors("Error deriving the key"); + handleErrors("Error generating the IV and salt"); } if (0 < ciphertext_len) { From 89ea1f05c36203261ffd4d1cb2d6ae8fcafc92da Mon Sep 17 00:00:00 2001 From: binsky Date: Fri, 30 Oct 2026 21:45:47 +0000 Subject: [PATCH 12/13] let a failed allocation fail the native crypto instead of crashing the app rare possibility, but technically possible Signed-off-by: binsky --- app/src/main/cpp/SJCL.cpp | 94 ++++++++++++++++++++++++++------------- 1 file changed, 64 insertions(+), 30 deletions(-) diff --git a/app/src/main/cpp/SJCL.cpp b/app/src/main/cpp/SJCL.cpp index 5de66013..9093c3bd 100644 --- a/app/src/main/cpp/SJCL.cpp +++ b/app/src/main/cpp/SJCL.cpp @@ -236,6 +236,9 @@ int encryptccm(unsigned char *plaintext, int plaintext_len, */ char* wstring_to_char(wstring str) { char* c = (char *) malloc(sizeof(char) * str.length() + 1); + if (c == nullptr) { + return nullptr; + } const wchar_t* data = str.c_str(); for (int i = 0; i <= str.length(); i++) { c[i] = (char) data[i]; @@ -327,6 +330,16 @@ char* SJCL::decrypt(const string& sjcl_json, const string& key) { *salt_64 = wstring_to_char(*salt_value), *cyphertext = wstring_to_char(*ct_value); + if (iv_64 == nullptr || adata == nullptr || salt_64 == nullptr || cyphertext == nullptr) { + handleErrors("Out of memory"); // ... i guess + free(iv_64); + free(adata); + free(salt_64); + free(cyphertext); + delete data; + return nullptr; + } + tag_size = (tag_size +7) / 8; // Make it bytes! key_size = (key_size +7) / 8; // Make it bytes @@ -356,22 +369,25 @@ char* SJCL::decrypt(const string& sjcl_json, const string& key) { // Assuming plaintext will always be smaller than the sjcl cyphertext which includes the tag and padding and stuff int cryptogram_length = cryptogram->length; unsigned char* plaintext = (unsigned char*) std::calloc(cryptogram_length +1, sizeof(unsigned char)); - string s = string("The allocated string is: ") + string((char*)plaintext); char* ret = nullptr; int plaintext_len = -1; - // The cryptogram is the ciphertext followed by the tag, so it can't be shorter than the tag - if (0 <= cyphertext_data_length) { - /* PBKDF2 Key derivation with SHA256 as SJCL does by default */ - if (1 == PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt->data, salt->length, iter, EVP_sha256(), key_size, derived_key)) { - // Decrypt the data - plaintext_len = decryptccm(cryptogram->data, cyphertext_data_length, (unsigned char *) adata, strlen(adata), - &cryptogram->data[cyphertext_data_length], tag_size, derived_key, iv_raw->data, iv_raw->length, plaintext); + if (derived_key != nullptr && plaintext != nullptr) { + // The cryptogram is the ciphertext followed by the tag, so it can't be shorter than the tag + if (0 <= cyphertext_data_length) { + /* PBKDF2 Key derivation with SHA256 as SJCL does by default */ + if (1 == PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt->data, salt->length, iter, EVP_sha256(), key_size, derived_key)) { + // Decrypt the data + plaintext_len = decryptccm(cryptogram->data, cyphertext_data_length, (unsigned char *) adata, strlen(adata), + &cryptogram->data[cyphertext_data_length], tag_size, derived_key, iv_raw->data, iv_raw->length, plaintext); + } else { + handleErrors("Error deriving the key"); + } } else { - handleErrors("Error deriving the key"); + handleErrors("The cryptogram is shorter than the tag"); } } else { - handleErrors("The cryptogram is shorter than the tag"); + handleErrors("Out of memory"); } if (0 < plaintext_len) { @@ -407,10 +423,14 @@ char* SJCL::decrypt(const string& sjcl_json, const string& key) { releaseDatagram(cryptogram); releaseDatagram(salt); releaseDatagram(iv_raw); - OPENSSL_cleanse(plaintext, cryptogram_length + 1); - free(plaintext); - OPENSSL_cleanse(derived_key, key_size); - free(derived_key); + if (plaintext != nullptr) { + OPENSSL_cleanse(plaintext, cryptogram_length + 1); + free(plaintext); + } + if (derived_key != nullptr) { + OPENSSL_cleanse(derived_key, key_size); + free(derived_key); + } return ret; } @@ -452,27 +472,35 @@ char* SJCL::encrypt(char* plaintext, const string& key) { ciphertext = (unsigned char *) malloc(sizeof(unsigned char) * strlen(plaintext) * ciphertext_allocation_multiplicator); int ciphertext_len = -1; - // Without random bytes, the IV and salt would be leftover memory and could repeat between encryptions - if (1 == RAND_bytes(iv, iv_len) && 1 == RAND_bytes(salt, salt_len)) { - // PBKDF2 Key derivation with SHA256 as SJCL does by default - if (1 == PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt, salt_len, iter, EVP_sha256(), ks, derived_key)) { - unsigned char *tmp_plaintext = reinterpret_cast(plaintext); - ciphertext_len = encryptccm(tmp_plaintext, strlen(plaintext), additional, strlen ((char *)additional), derived_key, iv, iv_len, ciphertext, tag, ts); + if (iv != nullptr && salt != nullptr && derived_key != nullptr && ciphertext != nullptr) { + // Without random bytes, the IV and salt would be leftover memory and could repeat between encryptions + if (1 == RAND_bytes(iv, iv_len) && 1 == RAND_bytes(salt, salt_len)) { + // PBKDF2 Key derivation with SHA256 as SJCL does by default + if (1 == PKCS5_PBKDF2_HMAC(key.c_str(), key.length(), salt, salt_len, iter, EVP_sha256(), ks, derived_key)) { + unsigned char *tmp_plaintext = reinterpret_cast(plaintext); + ciphertext_len = encryptccm(tmp_plaintext, strlen(plaintext), additional, strlen ((char *)additional), derived_key, iv, iv_len, ciphertext, tag, ts); + } else { + handleErrors("Error deriving the key"); + } } else { - handleErrors("Error deriving the key"); + handleErrors("Error generating the IV and salt"); } } else { - handleErrors("Error generating the IV and salt"); + handleErrors("Out of memory"); } if (0 < ciphertext_len) { + // without memory for the ciphertext with tag, the missing ciphertext_base64 fails the encryption below + Datagram* ciphertext_base64 = nullptr; uint8_t *ciphertext_with_tag = static_cast(malloc(sizeof(char *) * (ciphertext_len + ts))); - memcpy(ciphertext_with_tag, ciphertext, ciphertext_len); - memcpy(ciphertext_with_tag + ciphertext_len, tag, ts); + if (ciphertext_with_tag != nullptr) { + memcpy(ciphertext_with_tag, ciphertext, ciphertext_len); + memcpy(ciphertext_with_tag + ciphertext_len, tag, ts); - Datagram* ciphertext_base64 = BASE64::encode( - reinterpret_cast(ciphertext_with_tag), ciphertext_len + ts); - free(ciphertext_with_tag); + ciphertext_base64 = BASE64::encode( + reinterpret_cast(ciphertext_with_tag), ciphertext_len + ts); + free(ciphertext_with_tag); + } Datagram* salt_base64 = BASE64::encode(salt, salt_len); Datagram* iv_base64 = BASE64::encode(iv, iv_len); @@ -495,7 +523,11 @@ char* SJCL::encrypt(char* plaintext, const string& key) { string retrn = food.dump(); ret = (char *) std::calloc(retrn.length() +1, sizeof(char)); - strcpy(ret, retrn.c_str()); + if (ret != nullptr) { + strcpy(ret, retrn.c_str()); + } else { + handleErrors("Out of memory"); + } } else { handleErrors("Error encoding the cryptogram"); } @@ -509,8 +541,10 @@ char* SJCL::encrypt(char* plaintext, const string& key) { free(ciphertext); free(iv); free(salt); - OPENSSL_cleanse(derived_key, ks); - free(derived_key); + if (derived_key != nullptr) { + OPENSSL_cleanse(derived_key, ks); + free(derived_key); + } return ret; } From 9df6cc23377fb075939d59f4357a618cbf2084a1 Mon Sep 17 00:00:00 2001 From: binsky Date: Fri, 30 Oct 2026 22:37:30 +0000 Subject: [PATCH 13/13] catch C++ exceptions at the JNI boundary and rethrow them as usual Java exception Signed-off-by: binsky --- app/src/main/cpp/passman-lib.cpp | 35 +++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/app/src/main/cpp/passman-lib.cpp b/app/src/main/cpp/passman-lib.cpp index 898bd880..952559d4 100644 --- a/app/src/main/cpp/passman-lib.cpp +++ b/app/src/main/cpp/passman-lib.cpp @@ -66,9 +66,7 @@ void wipe(std::string &secret) OPENSSL_cleanse(&secret[0], secret.size()); } -extern "C" { - -jstring Java_es_wolfi_app_passman_SJCLCrypto_decryptStringCpp(JNIEnv *env, jclass jthis, jstring cryptogram, jstring key) { +jstring decryptString(JNIEnv *env, jstring cryptogram, jstring key) { std::string dt = toStdString(env, cryptogram); std::string password = toStdString(env, key); @@ -108,7 +106,7 @@ jstring Java_es_wolfi_app_passman_SJCLCrypto_decryptStringCpp(JNIEnv *env, jclas return NULL; } -jstring Java_es_wolfi_app_passman_SJCLCrypto_encryptStringCpp(JNIEnv *env, jclass jthis, jstring plaintext, jstring key) { +jstring encryptString(JNIEnv *env, jstring plaintext, jstring key) { std::string dt = toStdString(env, plaintext); std::string password = toStdString(env, key); @@ -143,4 +141,31 @@ jstring Java_es_wolfi_app_passman_SJCLCrypto_encryptStringCpp(JNIEnv *env, jclas return NULL; } -} \ No newline at end of file +/* + * A C++ exception leaving a JNI function aborts the app, e.g. the std::bad_alloc of a failed allocation + * in std::string or the JSON libraries, so the JNI functions turn it into the usual Java exception. + * (hopefully fixing a lot of random crashes, reported in the google play console) + */ +extern "C" { + +jstring Java_es_wolfi_app_passman_SJCLCrypto_decryptStringCpp(JNIEnv *env, jclass jthis, jstring cryptogram, jstring key) { + try { + return decryptString(env, cryptogram, key); + } catch (...) { + __android_log_write(ANDROID_LOG_ERROR, LOG_TAG, "unexpected error decrypting"); + throwJavaException(env, "Error decrypting"); + return NULL; + } +} + +jstring Java_es_wolfi_app_passman_SJCLCrypto_encryptStringCpp(JNIEnv *env, jclass jthis, jstring plaintext, jstring key) { + try { + return encryptString(env, plaintext, key); + } catch (...) { + __android_log_write(ANDROID_LOG_ERROR, LOG_TAG, "unexpected error encrypting"); + throwJavaException(env, "Error encrypting"); + return NULL; + } +} + +}