BT1-T142 : Check interaction between XSS and encryption #2149

Shnoulle · 2021-11-22T14:21:58Z

Dev: encryptSave validate by default
Dev: allow attribute filtering when encryotSave (see save function)

To be checked

application/models/SurveyDynamic.php:123: $record->encryptSave(); // In insertRecords : no 3.X check false retuened.
application/models/Participant.php:654: $result = $oParticipant->encryptSave(true);
application/models/Participant.php:1746: if (!$oToken->encryptSave(true)) {
application/models/Participant.php:2051: $oParticipant->encryptSave();
application/models/Participant.php:2055: $oTokenDynamic->encryptSave();
application/helpers/frontend_helper.php:419: $token->encryptSave();
application/helpers/remotecontrol/remotecontrol_handle.php:1893: if ($token->encryptSave(true)) {
application/helpers/remotecontrol/remotecontrol_handle.php:2072: if ($oToken->encryptSave(true)) {
application/helpers/remotecontrol/remotecontrol_handle.php:2888: $survey_dynamic->encryptSave();
application/helpers/remotecontrol/remotecontrol_handle.php:2992: $bResult = $aResponses[0]->encryptSave(true);
application/helpers/admin/import_helper.php:2204: if (!$token->encryptSave(true)) {
application/helpers/admin/import_helper.php:2533: if ($oSurvey->encryptSave()) {
application/helpers/expressions/em_manager_helper.php:5183: if (!$oResponse->encryptSave()) {
application/controllers/RegisterController.php:355: $oToken->encryptSave(true);
application/controllers/admin/participantsaction.php:718: $participant->encryptSave(true);
application/controllers/admin/dataentry.php:1491: if (!$oReponse->encryptSave()) {
application/controllers/admin/dataentry.php:1686: $new_response->encryptSave();
application/controllers/admin/dataentry.php:1752: $aToken->encryptSave(true);
application/controllers/admin/tokens.php:462: $bUpdateSuccess = $token->encryptSave(true);
application/controllers/admin/tokens.php:595: $inresult = $token->encryptSave(true);
application/controllers/admin/tokens.php:742: $result = $token->encryptSave(true);
application/controllers/admin/tokens.php:904: $token->encryptSave(true);
application/controllers/admin/tokens.php:1428: if (!$oToken->encryptSave(true)) {
application/controllers/admin/tokens.php:2154: if (!$oToken->encryptSave()) {
tests/unit/models/EncryptAttributesTest.php:38: $token->encryptSave(false);
tests/unit/models/EncryptAttributesTest.php:69: $token->encryptSave(true);
tests/unit/models/EncryptAttributesTest.php:108: $response->encryptSave(false);
tests/unit/models/EncryptAttributesTest.php:145: $response->encryptSave(true);

Dev: allow attribute filtering when encryotSave (see save function) Dev: 1st part, test waiting

Dev: Token need update only on participant_id

Dev: since there are no rules, no way to return false. Dev: then doing it for future

Dev: EM array_keys for attributes

Shnoulle · 2021-11-22T16:23:27Z

OK, right : removed unused params from child function …

gabrieljenik

Structure of code seems ok

gabrieljenik

I believe there is a problem in the frontend helper.
Initially, it decrypts all attributes.

Then encrypt, but only some.
Then the others are decrypted.

It does not break, because it is not saved.
But then it tries to decrypt everything back, and since they are already decrypted, an error occurs.

Will keep on reviewing later

Shnoulle · 2021-12-22T07:41:50Z

Then encrypt, but only some. Then the others are decrypted.

It does not break, because it is not saved. But then it tries to decrypt everything back, and since they are already decrypted, an error occurs.

related here : https://github.com/LimeSurvey/LimeSurvey/pull/2149/files#diff-4b5f6522204521a4e24e45dd60c31295cdc7298b0174dd96cf6c1e7d0334f4ffR420 ?

See 3.X :

LimeSurvey/application/helpers/frontend_helper.php

Line 433 in 43bb509

$token->save(true, ['completed', 'usesleft']);

or here :
https://github.com/LimeSurvey/LimeSurvey/pull/2149/files#diff-fa8d35264aa59c85d37ca2d1756472996499f06fb346865594378102f5f7abc0R5184

In 3.X : we use updateByPk but here we need encrypt and save only needed values

LimeSurvey/application/helpers/expressions/em_manager_helper.php

Line 5622 in 43bb509

Response::model($this->sid)->updateByPk($oResponse->id,$aResponseAttributes);

It's needed : we need to save only this values, encryot must happen only when save in my opinion :).

It's not an urgent fix here ;) January seems OK.

gabrieljenik · 2021-12-23T15:02:14Z

Hi Denis,

Generally speaking I am not too much in love in how encryption/decryption works for the token model.
Not a fan of havin to encrypt and decrypt everything, just for getting a single attribute.
Also, not sure if decrypting everything and keeping in status "decrypted" is the best.
I like more the ide of having getters that decrypt the info as needed and having the info stored all time encrypted inside the model.

Still, this is not the issue we are discussing.

On the curent code, if you follow these steps, you will get a 500

Create a new survey that uses tokens.
Set couple of attributes as encrypted.
Submit a response
A 500 error appears.

That happens as:

Frontendehelper:418 token is decrypted (all attributes)
Encryptsave encrypts some attributes (actually I think none, as "completed" and "usesleft" are sent as params)
Frontendehelper:423 token is decrypted (again, although no attributed is encrypted)

I would just remove Frontendehelper:418 and see if things workout like that.

Shnoulle · 2021-12-23T15:38:08Z

Strange : i think i do a test with token …

I can get a look in January : but i really think we must not encrypt what we don't save … Here :

LimeSurvey/application/models/LSActiveRecord.php

Line 297 in d9ee030

    
           public function encryptAttributeValues($attributes = null, $bEncryptedOnly = false, $bReplaceValues = true)

But you're right :

LimeSurvey/application/helpers/frontend_helper.php

Line 423 in d9ee030

$token->decrypt();

try to decrypt not encrypted value …

Different way to fix :

Encrypt all attributes when save …
Return decrypted Model after encryptSave …
Add a filter on decrypt function
Keep track of state of each attribute encrypted

I don't like 1 because if we don't need something we don't need to do it. I think 2 can broke at some other place For 3 : code need to track itself each attributes : seems not the best solution .

4 seems the best (in my opinion) ?

Shnoulle · 2021-12-23T15:40:45Z

PS :

Not a fan of havin to encrypt and decrypt everything, just for getting a single attribute.

Me too: don't like it :).

I like more the ide of having getters that decrypt the info as needed and having the info stored all time encrypted inside the model.

But you need to have it decrypted to use it, non ? Have 2 object : one for crypted and one decrypted ?
The have 2 Token object + 2 Response Object … arg …

gabrieljenik · 2021-12-24T11:33:02Z

But you need to have it decrypted to use it, non ? Have 2 object : one for crypted and one decrypted ?
The have 2 Token object + 2 Response Object … arg …

Not actually. The attribute is always held encrypted on the model. A getter for attribute would check if it needs decryption. If so, does it, but don't store it, just return it for the script to use it as needed.

As it the data is always encrypted in the model and there is no diff in how the data is picked up or saved to the DB.

Shnoulle · 2021-12-24T13:53:46Z

Not actually. The attribute is always held encrypted on the model. A getter for attribute would check if it needs decryption. If so, does it, but don't store it, just return it for the script to use it as needed.

But this need to review all usage of token value and response value ?

gabrieljenik · 2021-12-24T14:26:50Z

But this need to review all usage of token value and response value ?

As with every other approach I guess.
We need to look for the current decrypt() calls to track the places where needed.

Shnoulle · 2021-12-24T15:14:15Z

Not actually. The attribute is always held encrypted on the model. A getter for attribute would check if it needs decryption. If so, does it, but don't store it, just return it for the script to use it as needed.

This make a lot more work …

Shnoulle · 2021-12-24T15:16:45Z

Must check the need … i can easily fix double decrypt, but rewriting all done is out of project i think.

Shnoulle · 2022-01-05T16:22:08Z

@gabrieljenik i fix the issue. Currently : when try to save a single attribute : encryot all attributes (like before).

Here : we can update $token->encryptSave(true, ['completed', 'usesleft']); by $token->save(true, ['completed', 'usesleft']); but in my opinion : for token and response : encryptSave function must not exist : save function must encrypt if needed.

Else : i give my opinion previously. In my opinion : all model->attribute must be decrypted transparently (and save as crypted transparently too).

gabrieljenik · 2022-01-06T14:52:48Z

You're opinion : model->attribute must stay always crypted, must create or update getter and setter function for decrypt/uncrtyot

Yes, that was plan A.
Plan B is to decrypt on load, encrypt on save.
From a controller, not much difference from a regular model.

My (new) opinion : model->attribute must be always uncrypted, no need specific function or call. This can be done directly in
getAttributes($names=null)

Why there? What if someone tries to pick a specific encrypted attribute driectly. Like $model->attribute_encrtyped ?

but in my opinion : for token and response : encryptSave function must not exist : save function must encrypt if needed.

Agree. And load should unencrypt.

Else : i give my opinion previously. In my opinion : all model->attribute must be decrypted transparently (and save as crypted transparently too).

Agree. Question is what is the best way to accomplish that.

Shnoulle · 2022-01-06T14:57:53Z

Why there? What if someone tries to pick a specific encrypted attribute driectly. Like $model->attribute_encrtyped ?

I don't see any reason here ? But you're right …

The question now is 1. Make decision about all of this crypt system , 2. time of dev for project.

Both are done encryptSave validate by default + allow attribute filtering when encryptSave (see save function)

gabrieljenik · 2022-01-06T14:59:27Z

The question now is 1. Make decision about all of this crypt system , 2. time of dev for project.

Yes, this is more decision for @olleharstedt

olleharstedt · 2022-01-06T15:40:17Z

OK, I might have to spend some time reading this. ^^

c-schmitz · 2022-02-25T10:15:22Z

I think this is a good effort. @ptelu should take a look at this next sprint so this can be finished.

c-schmitz · 2022-05-11T07:55:27Z

@Shnoulle Can you fix the conflicts and then I will make sure it is reviewed?

…nto master_encryptSave

Shnoulle · 2022-05-11T17:30:08Z

No conflicts on local merge …
Someone can test Participant again ?

Shnoulle · 2022-06-08T06:02:50Z

No conflicts on local merge … Someone can test Participant again ?

Shnoulle · 2022-09-16T08:21:48Z

@olleharstedt Need to fix conflict or work for nothing ?

olleharstedt · 2022-09-16T10:11:10Z

@olleharstedt Need to fix conflict or work for nothing ?

Idk man. We have a problem of ownership internally, when it comes to pull requests... :/

Shnoulle · 2022-09-16T10:31:18Z

Idk man. We have a problem of ownership internally, when it comes to pull requests... :/

See that …

olleharstedt · 2022-09-16T10:38:31Z

Problem is that Ahmed is still not replaced. We have a meeting about it later today. Hopefully we can get something moving.

Shnoulle · 2023-01-17T08:52:29Z

@olleharstedt after more than one year … maybe we can close this PR ?

olleharstedt · 2023-02-09T10:06:15Z

@olleharstedt after more than one year … maybe we can close this PR ?

But there are still security issues solved by this PR or?

Also only one conflict so far.

Shnoulle · 2023-02-09T10:10:29Z

@olleharstedt after more than one year … maybe we can close this PR ?

But there are still security issues solved by this PR or?

I don't know … you don't explain the final reason. You ask me to encryptSave validate by default.

I think there are potential issue when adding feature or create plugin or currently exist but hidden

Also only one conflict so far.

Yes, but if I fix conflict toay , i don't want to fix it again in 4 month … I already have the impression to have developed for nothing on this pull request ...

olleharstedt · 2023-02-09T17:18:19Z

Yes, but if I fix conflict toay , i don't want to fix it again in 4 month … I already have the impression to have developed for nothing on this pull request ...

Yes, don't do anything now.

Shnoulle · 2023-11-03T10:12:52Z

Follow up on #3596

Dev: encryotSave validate by default

adc24a6

Dev: allow attribute filtering when encryotSave (see save function) Dev: 1st part, test waiting

Shnoulle marked this pull request as draft November 22, 2021 14:22

Shnoulle added 5 commits November 22, 2021 15:32

Dev: $oParticipant->save(false) in 3.X

6b37519

Dev: Token need update only on participant_id

Dev: mantis issue #17610 (no need encryptSave but clearer like that)

ab7a95b

Dev: EM update only needed value (3.X use updateByPk)

54a6df7

Dev: fix usage of save retuen control in remote control

cfee07d

Dev: check if new response saved before using it

4a697a3

Dev: since there are no rules, no way to return false. Dev: then doing it for future

Shnoulle marked this pull request as ready for review November 22, 2021 15:20

Shnoulle marked this pull request as draft November 22, 2021 15:24

Shnoulle added 2 commits November 22, 2021 16:35

Dev: really save only needed attributes

3a28562

Dev: EM array_keys for attributes

Dev: add unused $attributes param

d9ee030

Shnoulle marked this pull request as ready for review November 22, 2021 16:23

glimz added Needs testing Needs code review and removed Needs testing labels Nov 23, 2021

glimz requested a review from gabrieljenik November 26, 2021 09:37

gabrieljenik reviewed Nov 29, 2021

View reviewed changes

glimz requested review from ptelu and gabrieljenik December 21, 2021 15:27

gabrieljenik reviewed Dec 21, 2021

View reviewed changes

Shnoulle requested a review from gabrieljenik January 5, 2022 16:18

Shnoulle added the Needs update by author label May 11, 2022

Shnoulle added 2 commits May 11, 2022 19:22

Merge branch 'HEAD' of https://github.com/LimeSurvey/LimeSurvey.git i…

3aa4912

…nto master_encryptSave

Dev: 108 | ERROR | [x] Whitespace found at end of line

e34eb5b

Dev: 1719 | ERROR | [x] Expected 1 space(s) after IF keyword; 0 found

62b8adf

Shnoulle added Needs testing and removed Needs update by author labels May 11, 2022

gabrieljenik added this to the Development milestone Jul 13, 2022

c-schmitz force-pushed the master branch from e9ddc84 to c4b6f1c Compare September 1, 2022 11:04

Shnoulle added the Decision needed Use when a discussion does not lead to consensus, to get a decision from Carsten/Olle/other label Oct 1, 2022

Shnoulle mentioned this pull request Nov 3, 2023

Bug/16572 encrypt save by default #3596

Open

Shnoulle closed this Nov 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BT1-T142 : Check interaction between XSS and encryption #2149

BT1-T142 : Check interaction between XSS and encryption #2149

Shnoulle commented Nov 22, 2021 •

edited by gabrieljenik

Loading

Shnoulle commented Nov 22, 2021

gabrieljenik left a comment

gabrieljenik left a comment

Shnoulle commented Dec 22, 2021

gabrieljenik commented Dec 23, 2021

Shnoulle commented Dec 23, 2021 •

edited

Loading

Shnoulle commented Dec 23, 2021 •

edited

Loading

gabrieljenik commented Dec 24, 2021 •

edited

Loading

Shnoulle commented Dec 24, 2021

gabrieljenik commented Dec 24, 2021

Shnoulle commented Dec 24, 2021

Shnoulle commented Dec 24, 2021 •

edited

Loading

Shnoulle commented Jan 5, 2022

gabrieljenik commented Jan 6, 2022 •

edited by Shnoulle

Loading

Shnoulle commented Jan 6, 2022 •

edited

Loading

gabrieljenik commented Jan 6, 2022

olleharstedt commented Jan 6, 2022

c-schmitz commented Feb 25, 2022

c-schmitz commented May 11, 2022

Shnoulle commented May 11, 2022

Shnoulle commented Jun 8, 2022

Shnoulle commented Sep 16, 2022 •

edited

Loading

olleharstedt commented Sep 16, 2022

Shnoulle commented Sep 16, 2022

olleharstedt commented Sep 16, 2022 •

edited

Loading

Shnoulle commented Jan 17, 2023

olleharstedt commented Feb 9, 2023 •

edited

Loading

Shnoulle commented Feb 9, 2023

olleharstedt commented Feb 9, 2023 •

edited

Loading

Shnoulle commented Nov 3, 2023

BT1-T142 : Check interaction between XSS and encryption #2149

BT1-T142 : Check interaction between XSS and encryption #2149

Conversation

Shnoulle commented Nov 22, 2021 • edited by gabrieljenik Loading

Shnoulle commented Nov 22, 2021

gabrieljenik left a comment

Choose a reason for hiding this comment

gabrieljenik left a comment

Choose a reason for hiding this comment

Shnoulle commented Dec 22, 2021

gabrieljenik commented Dec 23, 2021

Shnoulle commented Dec 23, 2021 • edited Loading

Shnoulle commented Dec 23, 2021 • edited Loading

gabrieljenik commented Dec 24, 2021 • edited Loading

Shnoulle commented Dec 24, 2021

gabrieljenik commented Dec 24, 2021

Shnoulle commented Dec 24, 2021

Shnoulle commented Dec 24, 2021 • edited Loading

Shnoulle commented Jan 5, 2022

gabrieljenik commented Jan 6, 2022 • edited by Shnoulle Loading

Shnoulle commented Jan 6, 2022 • edited Loading

gabrieljenik commented Jan 6, 2022

olleharstedt commented Jan 6, 2022

c-schmitz commented Feb 25, 2022

c-schmitz commented May 11, 2022

Shnoulle commented May 11, 2022

Shnoulle commented Jun 8, 2022

Shnoulle commented Sep 16, 2022 • edited Loading

olleharstedt commented Sep 16, 2022

Shnoulle commented Sep 16, 2022

olleharstedt commented Sep 16, 2022 • edited Loading

Shnoulle commented Jan 17, 2023

olleharstedt commented Feb 9, 2023 • edited Loading

Shnoulle commented Feb 9, 2023

olleharstedt commented Feb 9, 2023 • edited Loading

Shnoulle commented Nov 3, 2023

Shnoulle commented Nov 22, 2021 •

edited by gabrieljenik

Loading

Shnoulle commented Dec 23, 2021 •

edited

Loading

Shnoulle commented Dec 23, 2021 •

edited

Loading

gabrieljenik commented Dec 24, 2021 •

edited

Loading

Shnoulle commented Dec 24, 2021 •

edited

Loading

gabrieljenik commented Jan 6, 2022 •

edited by Shnoulle

Loading

Shnoulle commented Jan 6, 2022 •

edited

Loading

Shnoulle commented Sep 16, 2022 •

edited

Loading

olleharstedt commented Sep 16, 2022 •

edited

Loading

olleharstedt commented Feb 9, 2023 •

edited

Loading

olleharstedt commented Feb 9, 2023 •

edited

Loading