View Issue Details

This bug affects 1 person(s).
 4
IDProjectCategoryView StatusLast Update
14898Bug reportsImport/Exportpublic2020-03-09 15:36
Reporterrealitix Assigned ToDenisChenu  
PrioritynoneSeveritypartial_block 
Status closedResolutionfixed 
Product Version3.15.x 
Fixed in Version3.17.x 
Summary14898: Prevent SID of -1 during import
Description

Linked to 14609

Hello,

When you import a survey, if the sid is -1, it will crash all the application.
Indeed, an attacker can manually set the sid to -1 in the .lss file.

The import works but then the application will crash at several places and the survey is impossible to delete.
Warning, if you try to reproduce this bug, you will need to delete the survey in database.

I'm sending the pull request that fixes it.

My pull request fixes all Sid < 0.

Steps To Reproduce
  1. Create a new survey by importing the joined .lss file
  2. Try to delete it
TagsNo tags attached.
Attached Files
Bug heat4
Complete LimeSurvey version number (& build)master 3.15.9
I will donate to the project if issue is resolvedNo
Browser
Database type & version0
Server OS (if known)
Webserver software & version (if known)
PHP Version0

Users monitoring this issue

There are no users monitoring this issue.

Activities

realitix

realitix

2019-05-15 10:17

reporter   ~51939

Here the lss hacked

realitix

realitix

2019-05-15 10:20

reporter   ~51940

Here the pull request:
https://github.com/LimeSurvey/LimeSurvey/pull/1280

DenisChenu

DenisChenu

2019-06-12 14:27

developer   ~52361

@realitix : I'm OK to merge the issue, but maybe we can have a solution to «don't broke» for surveyid < 0 too ?

  1. Add it to model rules
  2. Disable loading of old survey id < 0
  3. Delete it in checkintegrity ?
realitix

realitix

2019-06-12 14:33

reporter   ~52362

Hello Denis,

From what I know, it's not possible to have a SID < 0.
So my simple patch may be enough.

I'm not sure I understood your comment.

DenisChenu

DenisChenu

2019-06-12 15:04

developer   ~52363

it will crash all the application.

I check what happen and see if we can automatically fix this before disable import bad lss :)

After we add the rules and other fix :)

DenisChenu

DenisChenu

2019-06-12 15:05

developer   ~52364

@realitix : unsure about the solution : why not return an error : «Invalid sid in LSS» ?

It's just a quick point of view :)

DenisChenu

DenisChenu

2019-06-12 15:43

developer   ~52367

Ok, can confirm, but still : i think it best if

  1. Add it in rules : array('sid', 'numerical', 'integerOnly'=>true,'min'=>1),
  2. Remove GetNewSurveyID function from import_helper
  3. Add the control in https://github.com/LimeSurvey/LimeSurvey/blob/fb787fb08733b9747cfb83a778becb3b4835005c/application/models/Survey.php#L935

I check now after imported if we can fix it too.

DenisChenu

DenisChenu

2019-06-12 15:50

developer   ~52368

Ok, broke with $baselang = $survey->language; with debug=2
broke with Call to a member function getAttributes() on null with debug = 0

checkintegrity don't fix it

Can be deleted in Survey listing Quick action : don't think need a fix for previous survey

DenisChenu

DenisChenu

2019-06-12 15:53

developer   ~52369

From what I know, it's not possible to have a SID < 0.

Unsure on this point, and it broke (you make the proof).

BUt : you fix it only for LSS import, not for TSV import or copy/hack HTML to set value to -1/Remote control

Move it to Model : it's fix in more situation :)

realitix

realitix

2019-06-12 16:00

reporter   ~52371

Yes indeed, the bug is larger than I thought.
I wait for your patch since you took the issue.

DenisChenu

DenisChenu

2019-06-12 16:07

developer   ~52372

I wait for your patch … i take only the responsibility to discuss and merge …

DenisChenu

DenisChenu

2019-06-12 16:07

developer   ~52373

I can't assign it to you in mantis or github …

DenisChenu

DenisChenu

2019-06-14 18:00

developer   ~52418

https://github.com/LimeSurvey/LimeSurvey/commit/676619f9491de9e138ebec1f43ac9297a82a6830

Next time i didn't assign me ;)

DenisChenu

DenisChenu

2019-06-28 12:27

developer   ~52628

Fix committed to master branch: http://bugs.limesurvey.org/plugin.php?page=Source/view&amp;id=29014

Related Changesets

LimeSurvey: master 676619f9

2019-06-14 17:59:28

DenisChenu

Details Diff
Fixed issue 14898: Prevent SID of -1 during import
Dev: move all check in rules
Dev: and use validate for sid to get a new one in insertNewSurvey
Affected Issues
14898
mod - application/controllers/admin/surveyadmin.php Diff File
mod - application/helpers/admin/import_helper.php Diff File
mod - application/models/Survey.php Diff File
mod - application/views/admin/survey/subview/tabCopy_view.php Diff File

Issue History

Date Modified Username Field Change
2019-05-15 10:17 realitix New Issue
2019-05-15 10:17 realitix File Added: limesurvey_survey_947165(1).lss
2019-05-15 10:17 realitix Note Added: 51939
2019-05-15 10:20 realitix Note Added: 51940
2019-06-12 14:27 DenisChenu Note Added: 52361
2019-06-12 14:33 realitix Note Added: 52362
2019-06-12 15:04 DenisChenu Note Added: 52363
2019-06-12 15:05 DenisChenu Note Added: 52364
2019-06-12 15:30 DenisChenu Assigned To => DenisChenu
2019-06-12 15:30 DenisChenu Status new => assigned
2019-06-12 15:43 DenisChenu Note Added: 52367
2019-06-12 15:50 DenisChenu Note Added: 52368
2019-06-12 15:53 DenisChenu Note Added: 52369
2019-06-12 16:00 realitix Note Added: 52371
2019-06-12 16:07 DenisChenu Note Added: 52372
2019-06-12 16:07 DenisChenu Note Added: 52373
2019-06-14 18:00 DenisChenu Status assigned => resolved
2019-06-14 18:00 DenisChenu Resolution open => fixed
2019-06-14 18:00 DenisChenu Fixed in Version => 3.17.x
2019-06-14 18:00 DenisChenu Note Added: 52418
2019-06-28 12:27 DenisChenu Changeset attached => LimeSurvey master 676619f9
2019-06-28 12:27 DenisChenu Note Added: 52628
2020-03-09 15:36 c_schmitz Status resolved => closed