This is an automated email from the ASF dual-hosted git repository.
mridulpathak pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/trunk by this push:
new cae2bb65c1 Fixed: EntityCrypto uses AES in ECB mode for encrypt="true"
fields, producing deterministic ciphertext (OFBIZ-13599) (#2055)
cae2bb65c1 is described below
commit cae2bb65c1cdfdd26044e706a1d7f09bb445abe1
Author: Mridul Pathak <[email protected]>
AuthorDate: Wed Sep 23 15:06:47 2026 +0530
Fixed: EntityCrypto uses AES in ECB mode for encrypt="true" fields,
producing deterministic ciphertext (OFBIZ-13599) (#2055)
- Route EncryptMethod.TRUE through the same cipher default already used by
SALT, instead of forcing AES into ECB mode (no IV, deterministic ciphertext for
identical plaintext). Keep the old ECB cipher only as a decrypt fallback for
values written before this fix.
- Retire encrypt="salt" as a separate option, now redundant with
encrypt="true".
- Fix reencryptFields (OFBIZ-6694): select the primary key alongside the
target field so store() can find the row afterward, and skip view entities,
since storing through a view fails.
- Remove encrypt="true" from FinAccount.finAccountCode: the field is used
for exact-match lookup as a redemption code
(FinAccountHelper.getFinAccountFromCode), which is incompatible with
non-deterministic encryption. finAccountPin remains encrypted. Add
decryptFinAccountCodes, in the accounting component (not framework/entityext,
to avoid a framework-to-application dependency), to migrate existing ciphertext
values back to plain text.
- Remove the cardNumber search parameter from the findOrders service and
admin order search, since exact-match search against an encrypted field is no
longer possible.
- Document the required post-upgrade migration steps.
---
.../accounting/config/AccountingErrorUiLabels.xml | 3 ++
.../accounting/servicedef/services_finaccount.xml | 6 +++
.../accounting/finaccount/FinAccountServices.java | 46 ++++++++++++++++++
.../datamodel/entitydef/accounting-entitymodel.xml | 2 +-
applications/order/servicedef/services.xml | 1 -
.../ofbiz/order/order/OrderLookupServices.java | 13 +----
framework/entity/dtd/entitymodel.xsd | 1 -
framework/entity/entitydef/entitymodel_test.xml | 6 ---
.../org/apache/ofbiz/entity/model/ModelField.java | 3 +-
.../org/apache/ofbiz/entity/util/EntityCrypto.java | 55 +++++++++-------------
.../ofbiz/entity/test/EntityCryptoTestSuite.java | 48 +++++++------------
.../ofbiz/entityext/data/EntityDataServices.java | 17 ++++++-
.../_include/sy-entity-field-encryption.adoc | 39 +++++++++++++++
framework/security/src/docs/asciidoc/security.adoc | 1 +
14 files changed, 153 insertions(+), 88 deletions(-)
diff --git a/applications/accounting/config/AccountingErrorUiLabels.xml
b/applications/accounting/config/AccountingErrorUiLabels.xml
index 30c80a21e5..7d5496632f 100644
--- a/applications/accounting/config/AccountingErrorUiLabels.xml
+++ b/applications/accounting/config/AccountingErrorUiLabels.xml
@@ -239,6 +239,9 @@
<value xml:lang="zh">无法把标识为 ${parameters.finAccountTransId}
的财务账目交易提交到普通日记账中，无法找到 ${creditDebit} ${requiredField}</value>
<value xml:lang="zh-TW">無法把標識為 ${parameters.finAccountTransId}
的財務帳目交易提交到普通日記帳中,無法找到 ${creditDebit} ${requiredField}</value>
</property>
+ <property key="AccountingFinAccountCodeDecryptPermissionError">
+ <value xml:lang="en">You do not have permission to run the FinAccount
finAccountCode decryption migration</value>
+ </property>
<property key="AccountingFinAccountExpired">
<value xml:lang="ar">انتهت صلاحية الحساب منذ ${thruDate}</value>
<value xml:lang="de">Konto ist zum ${thruDate} abgelaufen</value>
diff --git a/applications/accounting/servicedef/services_finaccount.xml
b/applications/accounting/servicedef/services_finaccount.xml
index 5e204f0e5e..305b46b201 100644
--- a/applications/accounting/servicedef/services_finaccount.xml
+++ b/applications/accounting/servicedef/services_finaccount.xml
@@ -614,4 +614,10 @@ under the License.
<description>Delete a FinAccountTypeAttr</description>
<auto-attributes include="pk" mode="IN" optional="false"/>
</service>
+
+ <service name="decryptFinAccountCodes" engine="java" auth="true"
transaction-timeout="14400"
+
location="org.apache.ofbiz.accounting.finaccount.FinAccountServices"
invoke="decryptFinAccountCodes">
+ <description>One-time migration (OFBIZ-13599): decrypt existing
FinAccount.finAccountCode values now
+ that the field is no longer encrypted. Safe to run more than
once.</description>
+ </service>
</services>
diff --git
a/applications/accounting/src/main/java/org/apache/ofbiz/accounting/finaccount/FinAccountServices.java
b/applications/accounting/src/main/java/org/apache/ofbiz/accounting/finaccount/FinAccountServices.java
index 4b246d704a..86795f19ff 100644
---
a/applications/accounting/src/main/java/org/apache/ofbiz/accounting/finaccount/FinAccountServices.java
+++
b/applications/accounting/src/main/java/org/apache/ofbiz/accounting/finaccount/FinAccountServices.java
@@ -33,15 +33,18 @@ import org.apache.ofbiz.base.util.UtilMisc;
import org.apache.ofbiz.base.util.UtilProperties;
import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.entity.Delegator;
+import org.apache.ofbiz.entity.EntityCryptoException;
import org.apache.ofbiz.entity.GenericEntityException;
import org.apache.ofbiz.entity.GenericValue;
import org.apache.ofbiz.entity.condition.EntityCondition;
import org.apache.ofbiz.entity.condition.EntityExpr;
import org.apache.ofbiz.entity.condition.EntityOperator;
+import org.apache.ofbiz.entity.model.ModelField;
import org.apache.ofbiz.entity.util.EntityListIterator;
import org.apache.ofbiz.entity.util.EntityQuery;
import org.apache.ofbiz.order.finaccount.FinAccountHelper;
import org.apache.ofbiz.product.store.ProductStoreWorker;
+import org.apache.ofbiz.security.Security;
import org.apache.ofbiz.service.DispatchContext;
import org.apache.ofbiz.service.GenericServiceException;
import org.apache.ofbiz.service.LocalDispatcher;
@@ -510,4 +513,47 @@ public class FinAccountServices {
return result;
}
+
+ /**
+ * One-time migration for OFBIZ-13599: FinAccount.finAccountCode is no
longer marked
+ * {@code encrypt="true"}, since the field needs exact-match lookup to
function as a redemption
+ * code (see FinAccountHelper.getFinAccountFromCode) and encryption never
provided real
+ * confidentiality for it in practice. Existing rows still hold ciphertext
from before this
+ * change; this decrypts them back to plaintext so they remain usable.
Safe to run more than
+ * once -- a row whose value is already plaintext fails to decrypt and is
left untouched.
+ */
+ public static Map<String, Object> decryptFinAccountCodes(DispatchContext
dctx, Map<String, Object> context) {
+ Delegator delegator = dctx.getDelegator();
+ Security security = dctx.getSecurity();
+ Locale locale = (Locale) context.get("locale");
+
+ GenericValue userLogin = (GenericValue) context.get("userLogin");
+ if (!security.hasPermission("ACCOUNTING_ADMIN", userLogin)) {
+ return
ServiceUtil.returnError(UtilProperties.getMessage(RES_ERROR,
"AccountingFinAccountCodeDecryptPermissionError", locale));
+ }
+
+ try {
+ List<GenericValue> rows =
EntityQuery.use(delegator).from("FinAccount")
+ .select("finAccountId", "finAccountCode").queryList();
+ for (GenericValue row: rows) {
+ String rawValue = row.getString("finAccountCode");
+ if (UtilValidate.isEmpty(rawValue)) {
+ continue;
+ }
+ String plainValue;
+ try {
+ plainValue = (String)
delegator.decryptFieldValue("FinAccount", ModelField.EncryptMethod.TRUE,
rawValue);
+ } catch (EntityCryptoException e) {
+ // Already plaintext -- either this migration already ran,
or the row was
+ // created after encrypt="true" was removed. Nothing to do.
+ continue;
+ }
+ row.setString("finAccountCode", plainValue);
+ row.store();
+ }
+ } catch (GenericEntityException gee) {
+ return ServiceUtil.returnError(gee.getMessage());
+ }
+ return ServiceUtil.returnSuccess();
+ }
}
diff --git a/applications/datamodel/entitydef/accounting-entitymodel.xml
b/applications/datamodel/entitydef/accounting-entitymodel.xml
index 87eede5cbb..cb28319597 100644
--- a/applications/datamodel/entitydef/accounting-entitymodel.xml
+++ b/applications/datamodel/entitydef/accounting-entitymodel.xml
@@ -341,7 +341,7 @@ under the License.
<field name="finAccountTypeId" type="id"></field>
<field name="statusId" type="id"></field>
<field name="finAccountName" type="name"></field>
- <field name="finAccountCode" type="long-varchar"
encrypt="true"></field>
+ <field name="finAccountCode" type="long-varchar"></field>
<field name="finAccountPin" type="long-varchar" encrypt="true"></field>
<field name="currencyUomId" type="id"/>
<field name="organizationPartyId" type="id"><description>The internal
organization Party that owns (or rather, is liable for) the
account.</description></field>
diff --git a/applications/order/servicedef/services.xml
b/applications/order/servicedef/services.xml
index b773973ef0..35381242c7 100644
--- a/applications/order/servicedef/services.xml
+++ b/applications/order/servicedef/services.xml
@@ -1111,7 +1111,6 @@ under the License.
<attribute name="billingAccountId" type="String" mode="IN"
optional="true"/>
<attribute name="finAccountId" type="String" mode="IN"
optional="true"/>
- <attribute name="cardNumber" type="String" mode="IN" optional="true"/>
<attribute name="accountNumber" type="String" mode="IN"
optional="true"/>
<attribute name="paymentStatusId" type="String" mode="IN"
optional="true"/>
diff --git
a/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderLookupServices.java
b/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderLookupServices.java
index cf32fd427b..ddb5856ede 100644
---
a/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderLookupServices.java
+++
b/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderLookupServices.java
@@ -444,7 +444,6 @@ public class OrderLookupServices {
// payment preference fields
String billingAccountId = (String) context.get("billingAccountId");
String finAccountId = (String) context.get("finAccountId");
- String cardNumber = (String) context.get("cardNumber");
String accountNumber = (String) context.get("accountNumber");
String paymentStatusId = (String) context.get("paymentStatusId");
@@ -452,7 +451,7 @@ public class OrderLookupServices {
paramList.add("paymentStatusId=" + paymentStatusId);
conditions.add(makeExpr("paymentStatusId", paymentStatusId));
}
- if (finAccountId != null || cardNumber != null || accountNumber !=
null || paymentStatusId != null) {
+ if (finAccountId != null || accountNumber != null || paymentStatusId
!= null) {
dve.addMemberEntity("OP", "OrderPaymentPreference");
dve.addAlias("OP", "finAccountId");
dve.addAlias("OP", "paymentMethodId");
@@ -472,16 +471,6 @@ public class OrderLookupServices {
conditions.add(makeExpr("finAccountId", finAccountId));
}
- // search by card number
- if (UtilValidate.isNotEmpty(cardNumber)) {
- dve.addMemberEntity("CC", "CreditCard");
- dve.addAlias("CC", "cardNumber");
- dve.addViewLink("OP", "CC", Boolean.FALSE, UtilMisc.toList(new
ModelKeyMap("paymentMethodId", "paymentMethodId")));
-
- paramList.add("cardNumber=" + cardNumber);
- conditions.add(makeExpr("cardNumber", cardNumber));
- }
-
// search by eft account number
if (UtilValidate.isNotEmpty(accountNumber)) {
dve.addMemberEntity("EF", "EftAccount");
diff --git a/framework/entity/dtd/entitymodel.xsd
b/framework/entity/dtd/entitymodel.xsd
index 55c6eb4799..a082ab60c9 100644
--- a/framework/entity/dtd/entitymodel.xsd
+++ b/framework/entity/dtd/entitymodel.xsd
@@ -156,7 +156,6 @@ under the License.
<xs:restriction base="xs:token">
<xs:enumeration value="true"/>
<xs:enumeration value="false"/>
- <xs:enumeration value="salt"/>
</xs:restriction>
</xs:simpleType>
</xs:attribute>
diff --git a/framework/entity/entitydef/entitymodel_test.xml
b/framework/entity/entitydef/entitymodel_test.xml
index 42bd4de849..f1cba2615b 100644
--- a/framework/entity/entitydef/entitymodel_test.xml
+++ b/framework/entity/entitydef/entitymodel_test.xml
@@ -223,7 +223,6 @@ under the License.
<field name="testingCryptoTypeId" type="id"/>
<field name="unencryptedValue" type="description"/>
<field name="encryptedValue" type="description" encrypt="true"/>
- <field name="saltedEncryptedValue" type="description" encrypt="salt"/>
<prim-key field="testingCryptoId"/>
</entity>
<view-entity entity-name="TestingCryptoRawView"
@@ -236,11 +235,6 @@ under the License.
<complex-alias-field entity-alias="TC" field="encryptedValue"/>
</complex-alias>
</alias>
- <alias name="rawSaltedEncryptedValue">
- <complex-alias operator="+">
- <complex-alias-field entity-alias="TC" field="saltedEncryptedValue"/>
- </complex-alias>
- </alias>
</view-entity>
<entity entity-name="TestingRemoveAll"
package-name="org.apache.ofbiz.entity.test"
diff --git
a/framework/entity/src/main/java/org/apache/ofbiz/entity/model/ModelField.java
b/framework/entity/src/main/java/org/apache/ofbiz/entity/model/ModelField.java
index e7411960d5..07f3e36a8b 100644
---
a/framework/entity/src/main/java/org/apache/ofbiz/entity/model/ModelField.java
+++
b/framework/entity/src/main/java/org/apache/ofbiz/entity/model/ModelField.java
@@ -41,8 +41,7 @@ public final class ModelField extends ModelChild {
public enum EncryptMethod {
FALSE,
- TRUE,
- SALT;
+ TRUE;
public boolean isEncrypted() {
return this != FALSE;
diff --git
a/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityCrypto.java
b/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityCrypto.java
index bae17e48a4..e52a2128f5 100644
---
a/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityCrypto.java
+++
b/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityCrypto.java
@@ -22,7 +22,6 @@ import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.security.Key;
import java.security.NoSuchAlgorithmException;
-import java.security.SecureRandom;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
@@ -55,8 +54,6 @@ public final class EntityCrypto {
private final ConcurrentMap<String, byte[]> keyMap = new
ConcurrentHashMap<>();
private final StorageHandler[] handlers;
- private static final SecureRandom SECURE_RANDOM = new SecureRandom();
-
public EntityCrypto(Delegator delegator, String kekText) throws
EntityCryptoException {
this.delegator = delegator;
byte[] kek;
@@ -231,21 +228,24 @@ public final class EntityCrypto {
protected static final class ShiroStorageHandler extends StorageHandler {
private final HashService hashService;
private final AesCipherService cipherService;
- private final AesCipherService saltedCipherService;
+ private final AesCipherService legacyCipherService;
private final byte[] kek;
protected ShiroStorageHandler(byte[] kek) {
hashService = new DefaultHashService();
cipherService = new AesCipherService();
- cipherService.setMode(OperationMode.ECB);
- cipherService.setPaddingScheme(PaddingScheme.PKCS5);
- saltedCipherService = new AesCipherService();
+ // Kept only to decrypt values written before the fix for
OFBIZ-13599, which forced AES into ECB
+ // mode (no IV, deterministic ciphertext) instead of using
AesCipherService's safer default.
+ // Never used for new encryption; existing ECB values are migrated
via the reencryptFields service.
+ legacyCipherService = new AesCipherService();
+ legacyCipherService.setMode(OperationMode.ECB);
+ legacyCipherService.setPaddingScheme(PaddingScheme.PKCS5);
this.kek = kek;
}
@Override
protected Key generateNewKey() {
- return saltedCipherService.generateNewKey();
+ return cipherService.generateNewKey();
}
@Override
@@ -263,7 +263,7 @@ public final class EntityCrypto {
protected byte[] decodeKeyBytes(String keyText) throws
GeneralException {
byte[] keyBytes = Base64.decodeBase64(keyText);
if (kek != null) {
- keyBytes = saltedCipherService.decrypt(keyBytes,
kek).getClonedBytes();
+ keyBytes = cipherService.decrypt(keyBytes,
kek).getClonedBytes();
}
return keyBytes;
}
@@ -271,7 +271,7 @@ public final class EntityCrypto {
@Override
protected String encodeKey(byte[] key) throws GeneralException {
if (kek != null) {
- return saltedCipherService.encrypt(key, kek).toBase64();
+ return cipherService.encrypt(key, kek).toBase64();
} else {
return Base64.encodeBase64String(key);
}
@@ -279,22 +279,20 @@ public final class EntityCrypto {
@Override
protected byte[] decryptValue(byte[] key, EncryptMethod encryptMethod,
String encryptedString) throws GeneralException {
- switch (encryptMethod) {
- case SALT:
- return
saltedCipherService.decrypt(Base64.decodeBase64(encryptedString),
key).getClonedBytes();
- default:
- return
cipherService.decrypt(Base64.decodeBase64(encryptedString),
key).getClonedBytes();
+ byte[] encryptedBytes = Base64.decodeBase64(encryptedString);
+ try {
+ return cipherService.decrypt(encryptedBytes,
key).getClonedBytes();
+ } catch (Exception e) {
+ // Fall back to the legacy ECB decrypt for values written
before the fix for OFBIZ-13599;
+ // see the comment on legacyCipherService above.
+ Debug.logInfo("Decrypt with current cipher failed, trying
legacy ECB cipher", MODULE);
+ return legacyCipherService.decrypt(encryptedBytes,
key).getClonedBytes();
}
}
@Override
protected String encryptValue(EncryptMethod encryptMethod, byte[] key,
byte[] objBytes) throws GeneralException {
- switch (encryptMethod) {
- case SALT:
- return saltedCipherService.encrypt(objBytes, key).toBase64();
- default:
- return cipherService.encrypt(objBytes, key).toBase64();
- }
+ return cipherService.encrypt(objBytes, key).toBase64();
}
}
@@ -416,17 +414,10 @@ public final class EntityCrypto {
@Override
protected String encryptValue(EncryptMethod encryptMethod, byte[] key,
byte[] objBytes) throws GeneralException {
- byte[] saltBytes;
- switch (encryptMethod) {
- case SALT:
- // random length 5-16
- saltBytes = new byte[5 + SECURE_RANDOM.nextInt(11)];
- SECURE_RANDOM.nextBytes(saltBytes);
- break;
- default:
- saltBytes = new byte[0];
- break;
- }
+ // This legacy handler is never used for new encryption --
EntityCrypto.encrypt() always
+ // encrypts through the primary ShiroStorageHandler; this method
exists only so the class
+ // implements StorageHandler for its decrypt fallback role.
+ byte[] saltBytes = new byte[0];
byte[] allBytes = new byte[1 + saltBytes.length + objBytes.length];
allBytes[0] = (byte) saltBytes.length;
System.arraycopy(saltBytes, 0, allBytes, 1, saltBytes.length);
diff --git
a/framework/entity/src/test/java/org/apache/ofbiz/entity/test/EntityCryptoTestSuite.java
b/framework/entity/src/test/java/org/apache/ofbiz/entity/test/EntityCryptoTestSuite.java
index a43e4f12c2..b06a83443b 100644
---
a/framework/entity/src/test/java/org/apache/ofbiz/entity/test/EntityCryptoTestSuite.java
+++
b/framework/entity/src/test/java/org/apache/ofbiz/entity/test/EntityCryptoTestSuite.java
@@ -57,15 +57,12 @@ public class EntityCryptoTestSuite implements
JupiterTestHelper {
assertNull(entity.getString("encryptedValue"));
entity.setString("unencryptedValue", nanoTime);
entity.setString("encryptedValue", nanoTime);
- entity.setString("saltedEncryptedValue", nanoTime);
assertEquals(nanoTime, entity.getString("unencryptedValue"));
assertEquals(nanoTime, entity.getString("encryptedValue"));
- assertEquals(nanoTime, entity.getString("saltedEncryptedValue"));
entity.store();
entity.refresh();
assertEquals(nanoTime, entity.getString("unencryptedValue"));
assertEquals(nanoTime, entity.getString("encryptedValue"));
- assertEquals(nanoTime, entity.getString("saltedEncryptedValue"));
}
/**
@@ -86,32 +83,23 @@ public class EntityCryptoTestSuite implements
JupiterTestHelper {
GenericValue entity =
EntityQuery.use(delegator).from("TestingCrypto").where("testingCryptoId",
"1").queryOne();
assertNull(entity.getString("unencryptedValue"));
assertNull(entity.getString("encryptedValue"));
- assertNull(entity.getString("saltedEncryptedValue"));
GenericValue view =
EntityQuery.use(delegator).from("TestingCryptoRawView").where("testingCryptoId",
"1").queryOne();
assertNull(view.getString("unencryptedValue"));
assertNull(view.getString("encryptedValue"));
- assertNull(view.getString("saltedEncryptedValue"));
assertNull(view.getString("rawEncryptedValue"));
- assertNull(view.getString("rawSaltedEncryptedValue"));
// Verify that encryption is taking place
entity.setString("unencryptedValue", nanoTime);
entity.setString("encryptedValue", nanoTime);
- entity.setString("saltedEncryptedValue", nanoTime);
entity.store();
view.refresh();
assertEquals(nanoTime, view.getString("unencryptedValue"));
assertEquals(nanoTime, view.getString("encryptedValue"));
- assertEquals(nanoTime, view.getString("saltedEncryptedValue"));
String initialValue = view.getString("rawEncryptedValue");
- String initialSaltedValue = view.getString("rawSaltedEncryptedValue");
assertFalse(nanoTime.equals(initialValue));
- assertFalse(nanoTime.equals(initialSaltedValue));
- assertFalse(initialValue.equals(initialSaltedValue));
// Verify that the same value stored repeatedly gives different raw
encrypted values.
entity.setString("encryptedValue", nanoTime);
- entity.setString("saltedEncryptedValue", nanoTime);
entity.store();
//entity.refresh(); // this is a bug; store() ends up setting the
encrypted value *into* the entity
assertEquals(nanoTime, entity.getString("unencryptedValue"));
@@ -120,16 +108,13 @@ public class EntityCryptoTestSuite implements
JupiterTestHelper {
view.refresh();
assertEquals(nanoTime, view.getString("unencryptedValue"));
assertEquals(nanoTime, view.getString("encryptedValue"));
- assertEquals(nanoTime, view.getString("saltedEncryptedValue"));
String updatedValue = view.getString("rawEncryptedValue");
- String updatedSaltedValue = view.getString("rawSaltedEncryptedValue");
assertFalse(nanoTime.equals(updatedValue));
- assertFalse(nanoTime.equals(updatedSaltedValue));
- assertFalse(updatedValue.equals(updatedSaltedValue));
- assertEquals(initialValue, updatedValue);
- assertFalse(initialSaltedValue.equals(updatedSaltedValue));
+ // Regression guard for OFBIZ-13599: encryption must not be
deterministic. Before the fix, AES was
+ // forced into ECB mode (no IV), so the same plaintext always produced
the same raw ciphertext.
+ assertFalse(initialValue.equals(updatedValue));
}
/**
@@ -145,20 +130,17 @@ public class EntityCryptoTestSuite implements
JupiterTestHelper {
delegator.removeByAnd("TestingCrypto",
UtilMisc.toMap("testingCryptoTypeId", "LOOKUP"));
delegator.create("TestingCrypto", UtilMisc.toMap("testingCryptoId",
"lookup-null", "testingCryptoTypeId", "LOOKUP"));
delegator.create("TestingCrypto", UtilMisc.toMap("testingCryptoId",
"lookup-value", "testingCryptoTypeId", "LOOKUP",
- "encryptedValue", nanoTime, "saltedEncryptedValue", nanoTime));
+ "encryptedValue", nanoTime));
+ // Null values are not encrypted, so lookups against null still work
normally.
// This ends up using EntityExpr contained in EntityConditionList
assertEquals(1,
(EntityQuery.use(delegator).from("TestingCrypto").where("testingCryptoTypeId",
"LOOKUP",
"encryptedValue", null).queryList()).size());
- assertEquals(1,
(EntityQuery.use(delegator).from("TestingCrypto").where("testingCryptoTypeId",
"LOOKUP",
- "saltedEncryptedValue", null).queryList()).size());
- assertEquals(1,
(EntityQuery.use(delegator).from("TestingCrypto").where("testingCryptoTypeId",
"LOOKUP",
- "encryptedValue", nanoTime).queryList()).size());
+ // Regression guard for OFBIZ-13599: encrypted fields are no longer
exact-match lookupable, since
+ // encryption is non-deterministic -- a freshly-encrypted search value
never matches previously
+ // stored ciphertext for the same plaintext.
assertEquals(0,
(EntityQuery.use(delegator).from("TestingCrypto").where("testingCryptoTypeId",
"LOOKUP",
- "saltedEncryptedValue", nanoTime).queryList()).size());
-
- assertEquals(1,
EntityQuery.use(delegator).from("TestingCrypto").where("testingCryptoTypeId",
"LOOKUP",
- "encryptedValue", nanoTime).queryList().size());
+ "encryptedValue", nanoTime).queryList()).size());
}
/**
@@ -170,7 +152,9 @@ public class EntityCryptoTestSuite implements
JupiterTestHelper {
return EntityCondition.makeCondition(
EntityCondition.makeCondition("testingCryptoTypeId",
EntityOperator.IN, UtilMisc.toList("SUB_SELECT_1", "SUB_SELECT_3")),
EntityOperator.AND,
- EntityCondition.makeCondition("encryptedValue",
EntityOperator.EQUALS, nanoTime));
+ // Uses the unencrypted field here: encrypted fields are no longer
exact-match lookupable
+ // (OFBIZ-13599), so this exercises the sub-select
condition-building logic on its own.
+ EntityCondition.makeCondition("unencryptedValue",
EntityOperator.EQUALS, nanoTime));
}
/**
@@ -200,13 +184,13 @@ public class EntityCryptoTestSuite implements
JupiterTestHelper {
delegator.removeByAnd("TestingCrypto",
UtilMisc.toMap("testingCryptoTypeId", "SUB_SELECT_3"));
delegator.create("TestingCrypto", UtilMisc.toMap("testingCryptoId",
"SUB_1", "testingCryptoTypeId", "SUB_SELECT_1",
- "encryptedValue", nanoTime));
+ "unencryptedValue", nanoTime));
delegator.create("TestingCrypto", UtilMisc.toMap("testingCryptoId",
"SUB_2", "testingCryptoTypeId", "SUB_SELECT_2",
- "encryptedValue", nanoTime));
+ "unencryptedValue", nanoTime));
delegator.create("TestingCrypto", UtilMisc.toMap("testingCryptoId",
"SUB_3", "testingCryptoTypeId", "SUB_SELECT_3",
- "encryptedValue", "constant"));
+ "unencryptedValue", "constant"));
- results =
EntityQuery.use(delegator).from("TestingCrypto").where("encryptedValue",
nanoTime).orderBy("testingCryptoId").queryList();
+ results =
EntityQuery.use(delegator).from("TestingCrypto").where("unencryptedValue",
nanoTime).orderBy("testingCryptoId").queryList();
assertEquals(2, results.size());
assertEquals("SUB_1", results.get(0).get("testingCryptoId"));
assertEquals("SUB_2", results.get(1).get("testingCryptoId"));
diff --git
a/framework/entityext/src/main/java/org/apache/ofbiz/entityext/data/EntityDataServices.java
b/framework/entityext/src/main/java/org/apache/ofbiz/entityext/data/EntityDataServices.java
index 38faf2e024..a38f3c77cf 100644
---
a/framework/entityext/src/main/java/org/apache/ofbiz/entityext/data/EntityDataServices.java
+++
b/framework/entityext/src/main/java/org/apache/ofbiz/entityext/data/EntityDataServices.java
@@ -28,10 +28,12 @@ import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.nio.charset.StandardCharsets;
+import java.util.LinkedHashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Locale;
import java.util.Map;
+import java.util.Set;
import org.apache.commons.codec.binary.Base64;
import org.apache.ofbiz.base.crypto.DesCrypt;
@@ -49,6 +51,7 @@ import org.apache.ofbiz.entity.datasource.GenericHelperInfo;
import org.apache.ofbiz.entity.jdbc.DatabaseUtil;
import org.apache.ofbiz.entity.model.ModelEntity;
import org.apache.ofbiz.entity.model.ModelField;
+import org.apache.ofbiz.entity.model.ModelViewEntity;
import org.apache.ofbiz.entity.util.EntityListIterator;
import org.apache.ofbiz.entity.util.EntityQuery;
import org.apache.ofbiz.security.Security;
@@ -526,11 +529,23 @@ public class EntityDataServices {
}
for (ModelEntity modelEntity: modelEntities.values()) {
+ if (modelEntity instanceof ModelViewEntity) {
+ // Skip view entities: an encrypted field exposed through a
view is really an alias of a
+ // real entity's field, which this loop also visits directly;
storing through a view here
+ // fails anyway unless the view happens to be uniquely
updatable.
+ continue;
+ }
List<ModelField> fields = modelEntity.getFieldsUnmodifiable();
for (ModelField field: fields) {
if (field.getEncryptMethod().isEncrypted()) {
try {
- List<GenericValue> rows =
EntityQuery.use(delegator).from(modelEntity.getEntityName()).select(field.getName()).queryList();
+ // Select the primary key alongside the encrypted
field: store() below needs the PK
+ // to find the row again, otherwise it fails with
GenericEntityNotFoundException
+ // (OFBIZ-6694).
+ Set<String> fieldsToSelect = new
LinkedHashSet<>(modelEntity.getPkFieldNames());
+ fieldsToSelect.add(field.getName());
+ List<GenericValue> rows =
EntityQuery.use(delegator).from(modelEntity.getEntityName())
+ .select(fieldsToSelect).queryList();
for (GenericValue row: rows) {
row.setString(field.getName(),
row.getString(field.getName()));
row.store();
diff --git
a/framework/security/src/docs/asciidoc/_include/sy-entity-field-encryption.adoc
b/framework/security/src/docs/asciidoc/_include/sy-entity-field-encryption.adoc
new file mode 100644
index 0000000000..fdd98bade5
--- /dev/null
+++
b/framework/security/src/docs/asciidoc/_include/sy-entity-field-encryption.adoc
@@ -0,0 +1,39 @@
+////
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+////
+
+= Entity field encryption (encrypt="true")
+
+Fields marked `encrypt="true"` in entitydef XML (for example
`CreditCard.cardNumber` and
+`PartyPrivateInformation.socialSecurityNumber`) are encrypted with AES through
+`org.apache.ofbiz.entity.util.EntityCrypto`, using whichever cipher
configuration is safe by
+default rather than a fixed mode (OFBIZ-13599). Values encrypted before this
change remain
+decryptable automatically, so upgrading does not make existing data unreadable.
+
+[CAUTION]
+Existing rows are not re-encrypted automatically. Run the `reencryptFields`
service (permission
+`ENTITY_MAINT`) after upgrading so that data written under the older cipher
configuration is
+rewritten under the current one. It walks every `encrypt="true"` field across
the entity group
+named by its `groupName` parameter (default `org.apache.ofbiz`) and is safe to
run more than once.
+
+`FinAccount.finAccountCode` is no longer marked `encrypt="true"`: it is used
as an exact-match
+redemption code (see `FinAccountHelper.getFinAccountFromCode`), which requires
the same input to
+always compare equal, a property incompatible with the safer, randomized
encryption above. Its
+sibling field `finAccountPin` remains encrypted. Existing `finAccountCode`
values written before
+this change are still ciphertext; run the `decryptFinAccountCodes` service
(permission
+`ACCOUNTING_ADMIN`, also safe to run more than once) to convert them back to
plain text.
diff --git a/framework/security/src/docs/asciidoc/security.adoc
b/framework/security/src/docs/asciidoc/security.adoc
index bb2d65f570..64ad2a4053 100644
--- a/framework/security/src/docs/asciidoc/security.adoc
+++ b/framework/security/src/docs/asciidoc/security.adoc
@@ -43,3 +43,4 @@ For that you may take as an example to follow
https://issues.apache.org/jira/bro
include::_include/sy-password-and-JWT.adoc[leveloffset=+1]
include::_include/sy-CSRF-defense.adoc[leveloffset=+1]
include::_include/sy-impersonation.adoc[leveloffset=+1]
+include::_include/sy-entity-field-encryption.adoc[leveloffset=+1]
