Project

General

Profile

Actions

Task #2355

closed

Confidential storage cipher uses hardcoded initialization vector

Added by Petr Fišer almost 4 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Ondřej Kopr
Category:
Confidential Storage
Target version:
Start date:
07/01/2020
Due date:
09/16/2020
% Done:

100%

Estimated time:
Owner:

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

Related to IdStory Identity Manager - Feature #2391: Add support for changing AES-256 confidential storage keysClosedOndřej Kopr07/15/2020

Actions
Related to IdStory IdM containers - Task #2214: Allow stronger ciphers - enhance Java security policy fileClosedPetr Fišer04/17/2020

Actions
Related to IdStory Identity Manager - Feature #2652: Create a task to generate new initialization vector for values in the confidential storageClosedAlena Peterová01/21/2021

Actions
Actions #1

Updated by Petr Fišer almost 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.

The application behavior should thus be (for every separate piece of data put into confidential storage):
  • Saving new data into confidential storage
    1. Generate new random IV.
    2. Encrypt data using AES-CBC, key, IV.
    3. Store ciphertext and IV into the database.
  • Updating data in the confidential storage
    1. Generate new random IV.
    2. Encrypt updated data using AES-CBC, key, IV.
    3. 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.

Actions #2

Updated by Petr Fišer almost 4 years ago

  • Private changed from Yes to No
Actions #3

Updated by Petr Fišer almost 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)

Actions #4

Updated by Petr Fišer almost 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. :)

Actions #5

Updated by Ondřej Kopr almost 4 years ago

Generate new random vector for every item is possible with these changes:
  • 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

Actions #6

Updated by Petr Fišer almost 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!).

Desired application behavior:
  • Saving new data into confidential storage
    1. Generate new random IV.
    2. Encrypt data using AES-CBC, key, IV.
    3. Store ciphertext and IV into the database.
  • Updating data in the confidential storage
    1. Generate new random IV.
    2. Encrypt updated data using AES-CBC, key, IV.
    3. 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.
    1. IdM reads both ciphertext and corresponding IV from database.
    2. IdM initializes cipher in decrypt mode using AES-CBC, key, IV.
    3. IdM decrypts the ciphertext.
Notes:
  • 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.

Actions #7

Updated by Vít Švanda almost 4 years ago

  • Assignee changed from Petr Fišer to Ondřej Kopr
  • Target version set to 10.5.0
Actions #8

Updated by Vít Švanda almost 4 years ago

  • Estimated time set to 40.00 h
Actions #9

Updated by Vít Švanda over 3 years ago

  • Due date set to 09/16/2020
  • Target version changed from 10.5.0 to 10.6.0
Actions #10

Updated by Ondřej Kopr over 3 years ago

  • Status changed from New to In Progress
Actions #11

Updated by Ondřej Kopr over 3 years ago

  • Related to Feature #2391: Add support for changing AES-256 confidential storage keys added
Actions #12

Updated by Ondřej Kopr over 3 years ago

  • Related to Task #2214: Allow stronger ciphers - enhance Java security policy file added
Actions #13

Updated by Ondřej Kopr over 3 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:

installation documentation for linux was already updated before.

Actions #14

Updated by Ondřej Kopr over 3 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

Actions #15

Updated by Ondřej Kopr over 3 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.

Actions #16

Updated by Radek Tomiška over 3 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.

Actions #17

Updated by Radek Tomiška over 3 years ago

  • Assignee changed from Ondřej Kopr to Radek Tomiška
Actions #18

Updated by Radek Tomiška over 3 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?

Actions #19

Updated by Vít Švanda over 3 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.

Actions #20

Updated by Petr Fišer over 3 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.

Actions #21

Updated by Radek Tomiška over 3 years ago

  • Status changed from In Progress to Needs feedback
  • Assignee changed from Ondřej Kopr to Petr Fišer
Actions #22

Updated by Ondřej Kopr over 3 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.

Actions #23

Updated by Radek Tomiška over 3 years ago

  • Assignee changed from Petr Fišer to Ondřej Kopr
Actions #24

Updated by Radek Tomiška over 3 years ago

  • Status changed from Resolved to Closed
Actions #25

Updated by Alena Peterová over 3 years ago

  • Related to Feature #2652: Create a task to generate new initialization vector for values in the confidential storage added
Actions

Also available in: Atom PDF