Task #2355
closedConfidential storage cipher uses hardcoded initialization vector
100%
Description
During analysis of #2214 (more info, commands and examples therein), I noticed that there is a hardcoded IV in the IdM source: https://github.com/bcvsolutions/CzechIdMng/blob/develop/Realization/backend/core/core-impl/src/main/java/eu/bcvsolutions/idm/core/security/service/impl/DefaultCryptService.java#L57 .
This is a security problem, because we use the same secret key for encryption of each confidential information (aka. "message") that is put into storage. When using the same key to encrypt multiple messages, the AES-CBC mode should use unique (and random) initialization vector for each message. Such setup slightly lowers the security, but the encryption holds.
Now we are in a situation that both secret key and IV are the same. Therefore AES-CBC effectively degenerates and can be broken relatively easily, as a variant of a Book cipher. The attack still has some difficulties but it is doable.
This is analysis/design ticket. We should start producing unique IV for each message stored in the confidential storage (and even regenerate and update those IVs on message updates).
Related issues
Updated by Petr Fišer over 4 years ago
To correct product behavior, we simply need to generate new (and unique) IV for each encryption performed. Effectively: (re)encrypt data saved into confidential storage if changed.
Because IV does not need to be secret, we can store it along the ciphertext in the same database table. That way we will know which IV to use for which ciphertext during decryption.
- Saving new data into confidential storage
- Generate new random IV.
- Encrypt data using AES-CBC, key, IV.
- Store ciphertext and IV into the database.
- Updating data in the confidential storage
- Generate new random IV.
- Encrypt updated data using AES-CBC, key, IV.
- Update ciphertext and IV in the database, overwriting old values.
- Removing data from confidential storage.
- Without change (no enc/dec operations are performed in this step).
Secret key will be stored in the app config (separated from the database) the same way as we do now.
Source for clarification: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38a.pdf (Appendix C)
To quote:
The CBC, CFB, and OFB modes require an initialization vector as input, in addition to the plaintext. An IV must be generated for each execution of the encryption operation, and the same IV is necessary for the corresponding execution of the decryption operation. Therefore, the IV, or information that is sufficient to calculate the IV, must be available to each party to the communication. The IV need not be secret, so the IV, or information sufficient to determine the IV, may be transmitted with the ciphertext. For the CBC and CFB modes, the IVs must be unpredictable. In particular, for any given plaintext, it must not be possible to predict the IV that will be associated to the plaintext in advance of the generation of the IV. There are two recommended methods for generating unpredictable IVs. The first method is to apply the forward cipher function, under the same key that is used for the encryption of the plaintext, to a nonce. The nonce must be a data block that is unique to each execution of the encryption operation. For example, the nonce may be a counter, as described in Appendix B, or a message number. The second method is to generate a random data block using a FIPS-approved random number generator.
Updated by Petr Fišer over 4 years ago
The SecureRandom Java class (https://docs.oracle.com/javase/8/docs/api/java/security/SecureRandom.html) conforms with FIPS 140-2 and can be used for our purposes (generating IVs).
But there is an important note:
Depending on the implementation, the generateSeed and nextBytes methods may block as entropy is being gathered, for example, if they need to read from /dev/random on various Unix-like operating systems.
We already have that covered by starting Tomcat with java.security.egd=/dev/./urandom because urandom is unblocking even when there is not enough entropy gathered. This construct also makes Java use SHA1PRNG (stronger) instead of NativePRNG.
(More info: https://docs.oracle.com/en/java/javase/11/docs/specs/security/standard-names.html#securerandom-number-generation-algorithms)
Updated by Petr Fišer over 4 years ago
@kopro You wrote the original crypt service. Could you please look at this? I just need a quick estimate of what would it mean to change IdM behavior to conform with #2355#note-1. Tyvm. :)
Updated by Ondřej Kopr over 4 years ago
- vector will be different for every item - must be saved somewhere - new column in confidential storage?,
- check if this changes will be compatible with another implementation for confidential storage - vault (maybe it is compatible but for sure check it),
- add random generation for vector,
- the whole process with initialization and reinitialization cipher with new vector will be slower, check how big is difference with old behavior,
- change algorithm for encrypt and decrypt,
- new behavior isnt backward compatible, some LRT must be create for the first reinitialization with dynamic vector.
Implementation will change DefaultIdmConfidentialStorage and DefaultCryptService. For crypt service the change will not be compatible backward compatible (api will be changed).
Implementation+test+documentation+checks is about ~4MD
Updated by Petr Fišer over 4 years ago
Implementation spec
We need to correct the hardcoded confidential storage's cipher IV in a way that a random (and unique) IV is generated every time we store something in the confidential storage.
This means changing DefaultIdmConfidentialStorage and DefaultCryptService, the change should be internal to a product and probably will not propagate "outside" to modules and scripts (but analysis needed there!).
- Saving new data into confidential storage
- Generate new random IV.
- Encrypt data using AES-CBC, key, IV.
- Store ciphertext and IV into the database.
- Updating data in the confidential storage
- Generate new random IV.
- Encrypt updated data using AES-CBC, key, IV.
- Update ciphertext and IV in the database, overwriting old values.
- Removing data from confidential storage.
- Without change (no enc/dec operations are performed in this step).
- Reading data from confidential storage.
- IdM reads both ciphertext and corresponding IV from database.
- IdM initializes cipher in decrypt mode using AES-CBC, key, IV.
- IdM decrypts the ciphertext.
- IV is always 16B long (because AES in CBC mode).
- IV must be generated by CSRNG, this means using java.security.SecureRandom.nextBytes(...). For each encryption, new IV must be generated.
- IV must be generated server-side. Client must not have any way to influence the IV.
- IV can (and will) be stored in the same database table as encrypted data.
- This implementation change must be backwards compatible on deployments.
- We will create a flyway script that adds current IV into the confidential storage. We need to write the flyway anyway to add the db table column. (Other option to this is to have the original IV still in the Java source and use it only if there is not IV stored in the database - but this is ugly.)
- Maybe something else will be needed, we will consult ad-hoc.
- The cipher's secret key is stored as part of app configuration as it already is; no change there.
- Already used AES-CBC-PKCS5Padding cipher stays; no change there.
Estimate for implementation is about 4MD according to Ondra #2355#note-5.
See other notes in this ticket / ask if something needs clarification.
Updated by Vít Švanda over 4 years ago
- Assignee changed from Petr Fišer to Ondřej Kopr
- Target version set to 10.5.0
Updated by Vít Švanda over 4 years ago
- Due date set to 09/16/2020
- Target version changed from 10.5.0 to 10.6.0
Updated by Ondřej Kopr over 4 years ago
- Related to Feature #2391: Add support for changing AES-256 confidential storage keys added
Updated by Ondřej Kopr over 4 years ago
- Related to Task #2214: Allow stronger ciphers - enhance Java security policy file added
Updated by Ondřej Kopr over 4 years ago
- % Done changed from 0 to 70
Into confidential storage table was added new column for store IV vector. Dynamic generating was also added and now I started with testing.
Documentation was already updated:- https://wiki.czechidm.com/devel/documentation/security/dev/confidential-storage
- https://wiki.czechidm.com/tutorial/adm/czechidm_installation_win#local_confidential_storage
installation documentation for linux was already updated before.
Updated by Ondřej Kopr over 4 years ago
- % Done changed from 70 to 80
Dynamic vector was implemented. The old static vector still exists as backward compatibility way.
For first initialization was implemented method renewVector and new init processors InitConfidentialStorageVectorProcessor. The processor process all confidential values without vector and generate new vector for them.
Commit: https://github.com/bcvsolutions/CzechIdMng/commit/e78c526be8c8a3b7e9bb0b53d24c59c838f15fc0 (branch: okopr/2355-dynamic-vector)
Implemented together with #2391
Now I have trouble with one failed test and I must check mssql flyway script correctly
Updated by Ondřej Kopr about 4 years ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Ondřej Kopr to Radek Tomiška
- % Done changed from 80 to 90
Sorry guys for long progress by my side. I merged last develop into my branch and check test with core and acc and both works well (sometimes fail some async test, but sometimes not).
After tests I check mssql database with my tests and its works correctly.
With MSSQL I had little issue, my OLD database was really old and after I made upgrade to new CzechIdM version, the error appears:
Caused by: com.microsoft.sqlserver.jdbc.SQLServerException: Violation of PRIMARY KEY constraint 'idm_token_pkey'. Cannot insert duplicate key in object 'dbo.idm_token'. The duplicate key value is (0x9d1b31684f694e30bd7a3e1627fce7f4).
As workaround was clean this one record in idm_token table. Then everything works.
Please @tomiskar could you provide me a feedback? Thank you.
Updated by Radek Tomiška about 4 years ago
- Status changed from Needs feedback to In Progress
- Assignee changed from Radek Tomiška to Ondřej Kopr
I did test and code review, it works and code looks nice, thx! I've merged features into develop.
Review notes:
- InitConfidentialStorageVectorProcessor, renew method (+changelog note) can be deleted from my point of view and previous default IV can be used defensivelly for maintain backward compatibility (it's used anyway now from deprecated methods => can be moved lower). It's safer than resave all confidential values on start + audit fields are changed from user to [SYSTEM] on every record - this can be consulted and original requirement can be reviewed. Renew vector took 6s for 1500 values in my environment, so it's ok.
- method generateIV (~generateVector) can be moved to api and reused in confidential storage and tests
- default length can be added into IdmConfidentialStorageValue.iv column (see value)
- deprecated warnings are in test - you can supress deprecated warning or use new methods (but coverage will decrease)
Just notes:
- I personally don't like "iv" abrevation :), but it's ok.
Updated by Radek Tomiška about 4 years ago
- Assignee changed from Ondřej Kopr to Radek Tomiška
Updated by Radek Tomiška about 4 years ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Radek Tomiška to Vít Švanda
All review notes are implemented (except rename 'iv' :)), commit:
https://github.com/bcvsolutions/CzechIdMng/commit/358373d8c099554a1edd37aff28bba548ab7fbed
Could you provide me a feedback, please?
Updated by Vít Švanda about 4 years ago
- Status changed from Needs feedback to Resolved
- Assignee changed from Vít Švanda to Ondřej Kopr
- % Done changed from 90 to 100
I did review and tested last changes. Works well. Thanks for that you both.
Updated by Petr Fišer about 4 years ago
- Status changed from Resolved to In Progress
Please do not mark it as resolved yet. I talked to Ondra that I want to see the result before they go into product.
It is a crypto so I would like to do additional check of how it is actually implemented.
Updated by Radek Tomiška about 4 years ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Ondřej Kopr to Petr Fišer
Updated by Ondřej Kopr about 4 years ago
- Status changed from Needs feedback to Resolved
Together with @fiserp we check this new changes and it's OK. After consultation was made only one change in changelog.
Commit: https://github.com/bcvsolutions/CzechIdMng/commit/766e8366766b799d406c1af6a1d13b7c873cbfb8 (branch: develop)
Now we can resolve the ticket.
Updated by Radek Tomiška about 4 years ago
- Assignee changed from Petr Fišer to Ondřej Kopr
Updated by Radek Tomiška about 4 years ago
- Status changed from Resolved to Closed
Updated by Alena Peterová almost 4 years ago
- Related to Feature #2652: Create a task to generate new initialization vector for values in the confidential storage added