Hello,

A brief summary of the encrypted field after the rework we've done.

Some issues are still present in encryption plugin:
- CSV import : can not import existing encrypted data
- Admin Tracker Encryption key : sometimes form is not well sent and user is disconnected from Tuleap

Some fields features are not supported:
- Modals
- Semantic
- XML Import (create empty values)
- Copy : create an empty value

Best regards

gerrit #7547 integrated into Tuleap 9.3.99.149.

gerrit #7520 integrated into Tuleap 9.3.99.147.

gerrit #7540 integrated into Tuleap 9.3.99.142.

gerrit #7509 integrated into Tuleap 9.3.99.138.

I update this request to add a new security issue found during a code review.

User authorization is not verified at all on the form allowing to update the RSA public key. The user does not need to be tracker admin or even logged in if the project is public and the platform allowed to the anonymous.
CVSSv3 score: 8.2 (CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:L/I:H/A:N).

To answer my follow up from last week, the decision which has been made is to purge the data from the field when a new key is published.

To sum up what need to be done:
Security:
There is maybe more but at least 2 issues have been detected during the review of the first patch.
* Denial of Service with large message. RSA is slow and the implementation we use is too. An attacker can craft large message to consume CPU power on the server. CVSSv3 score: 6.5 (CVSS:3.0/AV:N/AC:L/PR:L/UI:N/S:U/C:N/I:N/A:H).
* Form to add a public key is not protected against CSRF. CVSSv3 score: 5.4 (CVSS:3.0/AV:N/AC:L/PR:L/UI:R/S:C/C:L/I:L/A:N).

Development:
* The code is not PSR-2 compliant nor follow internal conventions described in the developer guide.
* JavaScript code should not be mixed with the HTML. This is a bad practice as it makes JavaScript code harder to debug and prevent us to use the tools provided by modern browsers to improve the security of our users (e.g. Content Security Policy).
* Mustache template must be preferred over constructing HTML directly in the PHP code, even so if for some reason it is needed (but in the concerned context I do not see why) all variables used the construction of the HTML must be properly escaped without any exception.
* Functionality should be unit tested.
* There is no reason to give specific permissions to plugins/tracker_encryption/www/index.php in the source code. Currently the file has 775 perms, I would have expected 664.
* You should inject your dependencies in your object instead of hiding them (e.g. plugins/tracker_encryption/include/EncryptionKeySettings_Presenter.class.php where you should inject Tracker_Key).
* Your field extend the functionnalities of the field string but there is not a lot in common between these two fields. I predict this will become hard to manage in the next contributions on this topic.

Functional (based on what @hosniah wrote 4 months ago in this request):
* The checklist for the creation of a new tracker field have not been followed, all the points listed here should be taken into account: https://documentation-tuleap.readthedocs.io/en/latest/developer-guide/trackers.html#create-a-new-field
* When you update a public key, the changeset history is rewritten and kinda broken. That should not happen.
* You should have a password field when you edit your field and there is no point to have the encrypted value in the edition mode.
* The encrypted value should not be displayed directly to the user, a large random value in base64 is not really something a normal can directly use.
* The check done to verify if we upload a public key is not correct. The header and footer is not a correct way to verify this point.
* There is no explanation on the expected type and format of key.
* There is no explanation on how to use the feature. A user does not know how the data are encrypted without looking at the code.
* We should not have a warning message saying I should upload a key when I do not have an encrypted field in this tracker.

Please do no try to fix all these points at once or this will become hell. Split your work in small and meaningful commits. Also, if something comes up during reviews that you do not understand/want to change for some reasons please do not ignore it and speak with the reviewer. A review is not a one way exchange.

While I was testing the current implementation (see gerrit #5484), I got a question and I do not think we have answered it yet.

Currently when we update the public key of a tracker the current content of the encrypted fields is kept which is not very practical for an user and could raise questions: Why can't I decrypt the value of this field when I have the private key corresponding to the public key indicated in the tracker configuration?

I see 2 solutions to this issue:
* we remove the content of the existing encrypting field. This is the logical way to follow if we deal with sensitive data. If your private key is compromised you do not want leave the encrypted data in your database.
* when we retrieve the data we also provided to the user which public key has been used.

Hum indeed it looks like I miss the fact that one of the dependency requires PHP 5.4+. However, phpseclib is available and already packaged in EPEL so we can use it directly. This means we will not have the nice API of EasyRSA at our disposal and it will require more attention.
Again, cryptography is hard and must be implemented correctly to have an interest. The end goal is to have something that is secure for everyone, not something that pass a vague compliance requirement.
I don't get the argument concerning the time of exposure of the data. The value is stored in the database until it is removed/edited/whatever, I'm not sure on how this qualify as short time. And even so, the proposal here is not about matching high security standards but just matching the current cryptography standards. I do not see how can we find acceptable to integrate something with known vulnerabilities.

We still have not clarified the threat model and the need (or not) of end to end encryption. I do not see how in the same model you could consider the fact that the database server can be compromised but not the Tuleap server. Should we plan a meeting to discuss about that?

On the practical side, this request should be split in multiple user stories (as already asked the 2016-04-27), otherwise I predict reviews are going to be big and painful on both sides. The tracker for the user stories is here: https://tuleap.net/plugins/tracker/?tracker=147

Hello Thomas,

We tried to use EasyRsa as you mentioned, but it's compatible only with php versions above 5.4, actually the password that will be encrypted will be available only for a short time (the lapse of time in which a jenkins server get the encrypted data), i don't see the need of a high security lib to use, i thins openssl standard PHP lib is sufficient.

waiting for your feedback.

Best regards,
Rafik

Hello Thomas,

Thanks a lot for sharing these interesting references.

To be honest with, i'm not the right person to argue the design of this feature. We should probably communicate with François Jean-Marie...

Actually, Rafik and Fares are currently working on the change, they'll try to use EasyRSA library  and implement check on the size f the key in order to mitigate risks you've raised in the current implementation.

Best regards,

Ahmed

Hi,

I understand the use case but as it is a security feature I try to establish a minimal threat model to see what it adds. Currently, I'm not able to see one that the current proposal fully covers but I probably miss something. From what you tell me, why do we need to add crypto into the mix? A field storing your value and making the content only accessible via the REST API seems to be enough. This way, your data is never displayed in clear to an end user.

No problem, I can understand you have operational constraints, but if by "openssl encryption" you mean RSA I just find it unfortunate that for a new feature we need to implement endangered (or at least challenged) cryptography when we have a new generation of primitive ready for production.
That's said, if RSA is the only choice we should be attentive to not implement broken cryptography. All what you will find in PHP APIs is broken if you try to use it directly. For example, openssl_public_encrypt() function use by default PKCS#1 version 1.5 [1] which will expose ourself to choosen ciphertext attack [2] [3]. The current standard is to use RSAES-OAEP [4] with MGF1-SHA256 [4] [5]. openssl_public_encrypt() only allow to use MGF1-SHA1, SHA1 [5] is known to have multiple weaknesses [6] [7] [8] [9] and should be avoided. So if we need to do some RSA operations in PHP I advise to use the EasyRSA library [10] which is maintained by Paragonie and take care of these implementations details for you.
Also, we should refuse to use a key with a size inferior to 2048 bits to not let the possibility to a user to use vulnerable keys and match current ANSSI recommendations [11].

[1] https://tools.ietf.org/html/rfc2313
[2] https://en.wikipedia.org/wiki/Chosen-ciphertext_attack
[3] http://archiv.infsec.ethz.ch/education/fs08/secsem/Bleichenbacher98.pdf (which is a must read to understand this kind of attack)
[4] https://tools.ietf.org/html/rfc3447
[5] http://csrc.nist.gov/publications/fips/fips180-4/fips-180-4.pdf
[6] https://eprint.iacr.org/2005/010.pdf
[7] https://www.schneier.com/blog/archives/2005/02/sha1_broken.html
[8] https://eprint.iacr.org/2010/413.pdf
[9] http://2012.sharcs.org/slides/stevens.pdf
[10] https://github.com/paragonie/EasyRSA
[11] http://www.ssi.gouv.fr/uploads/2014/11/RGS_v-2-0_B1.pdf (fr)

Hello Thomas,

Sorry for the reponse delay.

Basically all we need is an encrypted field value to be retrieved by an ACI agent (thru tracker rest api), there are no particular threats mentionned by specs authors. It sounds more like a one time password that should never be displayed in clear.

In the other hand, we've challenged the spec author to use GPG, but sounds that openssl encryption was a business requirement. Moreover, we do not manage the decryption part on Tuleap..

Anyway, i'm interested by any related documentation.

thanks and best regards,

Ahmed

As you are creating a new field, I suggest that you have a look at the following checklist in order to not forget important points:
http://documentation-tuleap.readthedocs.org/en/latest/developer-guide/trackers.html#create-a-new-field

Hello Nicolas,

The encrypted field is extending TV5 String field with openssl encryption for field values:

• Input field is an html password field (bullet are displayed during artifact creation / update), clear value should never be displayed in Tuleap (changsets, notification, etc...).
• openssl_public_encrypt() encrypts data using the "per tracker" public key and stores the crypted result in Tuleap Data base.
• Encrypted field value is exposed via REST route:

     A CI user having Read on this encrypted field should be then able to decrypt via openssl_private_decrypt(), the clear value of a tuleap encrypted field could be then read only by owner of the private key (which is the main use case for TRAM) .

Best regards,

Ahmed

Hi, could you please explain further the aim of "encrypted fields"?