View Issue Details

IDProjectCategoryView StatusLast Update
16469Bug reportsSecuritypublic2020-08-06 17:32
ReporterDenisChenu Assigned ToDenisChenu  
PriorityimmediateSeveritycrash 
Status closedResolutionfixed 
Product Version4.3.3 
Summary16469: Any admin user can see any question (without read right on survey)
Description

questionEditor din't check Permission on survey from question, but sid send by url

Steps To Reproduce
  1. As superadmin create a survey + a question
  2. Copy link to see the question : example : /questionEditor/view?surveyid=352826&gid=320&qid=5146
  3. Create an TEST user with survey/create right
  4. Log out as super admin
  5. Log in as TEST user
  6. Create a survey (sample : 562152)
  7. Update your copied link and replace sid : /questionEditor/view?surveyid=562152&gid=320&qid=5146
Additional Information

In my opinion :
Before any other things

Get qid from url
If set : get gid : , check if gid from url === gid from qid
If gid: get sid from gid
Check if sid from url === sid from gid

Send a 400 error if there are issue : allow other protection system to do their job (for example fail2ban)

TagsNo tags attached.
Complete LimeSurvey version number (& build)4.3.3
I will donate to the project if issue is resolvedNo
Browsernot relevant
Database & DB-Versionnot relevant
Server OS (if known)not relevant
Webserver software & version (if known)not relevant
PHP Versionnot relevant

Relationships

related to 16470 new Development  Use real http header instead of redirect for permission denial 

Activities

DenisChenu

DenisChenu

2020-07-08 12:37

developer   ~58773

I didn't chek update, but think there are solution

DenisChenu

DenisChenu

2020-07-08 12:41

developer   ~58774

In 3.X … send a 200 white page … (with debug or not)

ollehar

ollehar

2020-07-08 13:30

administrator   ~58775

OK, thanks for testing, Denis.

ollehar

ollehar

2020-07-08 14:43

administrator   ~58780

I'm working on it now!

ollehar

ollehar

2020-07-08 14:52

administrator   ~58781

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

ollehar

ollehar

2020-07-08 14:58

administrator   ~58782

Another solution could be to never send both qid AND sid together. sid should come from the question and then be checked for permission.

DenisChenu

DenisChenu

2020-07-08 15:12

developer   ~58784

Another solution could be to never send both qid AND sid together. sid should come from the question and then be checked for permission.

Think it's the best solution, i already report this issue before 2.6lts

DenisChenu

DenisChenu

2020-07-08 15:13

developer   ~58785

question need onlky qid
group need only gid
answer need only aid (but we never edit a single answer)

pstelling

pstelling

2020-07-09 10:10

developer   ~58822

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

DenisChenu

DenisChenu

2020-07-09 11:57

developer   ~58824

@pstelling :

  1. no … access on survey id on URL is already done : your commit does nothing . Do you really read the way to reproduce ?
  2. https://github.com/LimeSurvey/LimeSurvey/commit/ab3562d77ae684afd93e4d0d2925d9b932cd9485 already fix the issue (but show no questin)
  3. can you check : https://bugs.limesurvey.org/view.php?id=16470 before make a new commit
DenisChenu

DenisChenu

2020-07-09 12:00

developer   ~58825

Erg sorry … issue is worst i think : there are NO control even in surevyid in url …

DenisChenu

DenisChenu

2020-07-09 12:10

developer   ~58826

Really worst :

http://limesurvey.local/master/questionEditor/GetAdvancedOptions?surveyid=713583&gid=296&qid=4917 : no control is done
http://limesurvey.local/master/questionEditor/GetQuestionPermissions?surveyid=352826&gid=320&iQuestionId=5146 : no control is done

Maybe other …

DenisChenu

DenisChenu

2020-07-09 12:12

developer   ~58827

Then :

Issues

  1. sid/surveyid is NEVER tested in vioew (hope it was in save)
  2. question access right are not tested on json part
  3. no control if sid of questionid are related to surveyid

We really need a better fix then the current quick fix

ollehar

ollehar

2020-07-09 12:14

administrator   ~58828

Can you link to github instead of your local repo? xD

DenisChenu

DenisChenu

2020-07-09 12:27

developer   ~58832

No : it's sample url :)

I can not really link to github , vbecause create a pull request is more quick than link to ght hub …

  1. https://github.com/LimeSurvey/LimeSurvey/blob/ab3562d77ae684afd93e4d0d2925d9b932cd9485/application/controllers/QuestionEditorController.php#L67 must be a 404
  2. https://github.com/LimeSurvey/LimeSurvey/blob/ab3562d77ae684afd93e4d0d2925d9b932cd9485/application/controllers/QuestionEditorController.php#L69 : must check on surveyid Permission and send a 401
  3. https://github.com/LimeSurvey/LimeSurvey/blob/ab3562d77ae684afd93e4d0d2925d9b932cd9485/application/controllers/QuestionEditorController.php#L69 : must check if qid->sid are equal to surveyid and send a a 400
  4. Must do the same on ALL function in this file

Maybe best to create a checked on LSBaseController , i do it after fixing TypeHint

ollehar

ollehar

2020-07-09 12:39

administrator   ~58833

401? That's not default behaviour of LS?

DenisChenu

DenisChenu

2020-07-09 13:08

developer   ~58836

Not a 401, a 403 : https://github.com/LimeSurvey/LimeSurvey/blob/9fd5e78aa6a67b1078a05746c29bceb9f8fab7c0/application/core/Survey_Common_Action.php#L193
Only if extend Survey_Common_Action and use _addPseudoParams

401 mean : you're not connected , you need to be conneceted to access this link
403 : your are identified , but you don't have access to this link

ollehar

ollehar

2020-07-09 13:10

administrator   ~58837

403 doesn't happy anywhere else in the software. Why change the behaviour here?

DenisChenu

DenisChenu

2020-07-09 13:57

developer   ~58841

403 for all no Permission on survey …

Else : show me current way : You mean leave the redirect (302) ? That broke external tool and XHR ?

And because : it's always a good idea to respect standard.

ollehar

ollehar

2020-07-09 14:15

administrator   ~58842

Redirect is already done EVERYWHERE in the software, Denis! It should be consistent.

ollehar

ollehar

2020-07-09 14:16

administrator   ~58843

grep -r 'Permission::model' application/controllers/ -A2 | grep redirect | wc

86 redirects after permission check.

ollehar

ollehar

2020-07-09 14:30

administrator   ~58844

Wait, you're talking about the ajax calls?

DenisChenu

DenisChenu

2020-07-09 14:39

developer   ~58845

Redirect is already done EVERYWHERE in the software, Denis! It should be consistent.
Still a bad solution.

It's not because old code are broken we need to add old code again and again …

I don't care about ajax or not : just send good http header

ollehar

ollehar

2020-07-09 14:43

administrator   ~58847

Well, I care about the software behaving consistently towards the user. :) If you want to change it to 403, make a separate dev issue or feature request. This ticket is about access bugs.

DenisChenu

DenisChenu

2020-07-09 15:06

developer   ~58853

https://github.com/LimeSurvey/LimeSurvey/pull/1479

DenisChenu

DenisChenu

2020-07-09 15:12

developer   ~58859

With invalid survey : it's already a 403 in 3.LTS,
Solution currently pushed do a redirect, this was not consistent

lime_release_bot

lime_release_bot

2020-07-13 12:35

administrator   ~58917

Fixed in Release 4.3.4+200713

DenisChenu

DenisChenu

2020-07-13 14:32

developer   ~58921

GetAdvancedOptions still not protected
GetQuestionPermission still not protected

Strange issue with updating sid (and not gid) : create a question without group ? See screencast
hard to check : thoirw error with devbug

DenisChenu

DenisChenu

2020-07-13 14:32

developer   ~58922

Peek 13-07-2020 14-32.gif (190,454 bytes)
DenisChenu

DenisChenu

2020-08-06 14:08

developer   ~59339

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

lime_release_bot

lime_release_bot

2020-08-06 17:32

administrator   ~59346

Fixed in Release 4.3.9+200806

Related Changesets

LimeSurvey: master ab3562d7

2020-07-08 14:50:20

ollehar

Details Diff
Fixed issue 16469: Any admin user can see any question Affected Issues
16469
mod - application/controllers/QuestionEditorController.php Diff File

LimeSurvey: master 9fd5e78a

2020-07-09 10:10:01

pstelling

Details Diff
Fixed issue 16469: Any admin user can see any question Affected Issues
16469
mod - application/controllers/QuestionEditorController.php Diff File

LimeSurvey: master 17f0a0d5

2020-08-06 14:08:31

DenisChenu


Committer: GitHub Details Diff
Master ls base controller check params (#1479)

Fixed issue 16469: Any admin user can see any question (without read right on survey)
Fixed issue #16467: Reflected XSS vulnerabilities
Dev: add a function to validate int parameters
Dev: throw 400/403 or 404 error

Dev: remove some uneeded redirect and filter

Dev: Move functionnality to an helper function to call
Dev : getValidateSurveyId return a validated sid related with qid and gid

Dev: rename to getValidatedSurveyId
Dev: getQuestionObject must be reviewed : $sureyId must be a mandatory param

Dev: more details in function doc and fix partially phpDoc

Dev: getValidatedSurveyId get the final sid by param
Dev: review test :
Affected Issues
16467, 16469
mod - application/controllers/LSBaseController.php Diff File
mod - application/controllers/QuestionEditorController.php Diff File
mod - tests/functional/backend/AdminViewsTest.php Diff File

Issue History

Date Modified Username Field Change
2020-07-08 12:37 DenisChenu New Issue
2020-07-08 12:37 DenisChenu Note Added: 58773
2020-07-08 12:38 DenisChenu Steps to Reproduce Updated View Revisions
2020-07-08 12:38 ollehar Priority none => immediate
2020-07-08 12:38 ollehar Severity minor => crash
2020-07-08 12:41 DenisChenu Note Added: 58774
2020-07-08 13:30 ollehar Note Added: 58775
2020-07-08 14:39 pstelling Assigned To => pstelling
2020-07-08 14:39 pstelling Status new => assigned
2020-07-08 14:43 ollehar Note Added: 58780
2020-07-08 14:52 ollehar Changeset attached => LimeSurvey master ab3562d7
2020-07-08 14:52 ollehar Note Added: 58781
2020-07-08 14:52 ollehar Assigned To pstelling => ollehar
2020-07-08 14:52 ollehar Resolution open => fixed
2020-07-08 14:58 ollehar Note Added: 58782
2020-07-08 15:12 DenisChenu Note Added: 58784
2020-07-08 15:13 DenisChenu Note Added: 58785
2020-07-09 10:10 pstelling Changeset attached => LimeSurvey master 9fd5e78a
2020-07-09 10:10 pstelling Note Added: 58822
2020-07-09 10:10 pstelling Assigned To ollehar => pstelling
2020-07-09 11:57 DenisChenu Note Added: 58824
2020-07-09 12:00 DenisChenu Note Added: 58825
2020-07-09 12:10 DenisChenu Note Added: 58826
2020-07-09 12:11 DenisChenu Relationship added related to 16470
2020-07-09 12:12 DenisChenu Note Added: 58827
2020-07-09 12:14 ollehar Note Added: 58828
2020-07-09 12:27 DenisChenu Note Added: 58832
2020-07-09 12:39 ollehar Note Added: 58833
2020-07-09 13:08 DenisChenu Note Added: 58836
2020-07-09 13:10 ollehar Note Added: 58837
2020-07-09 13:57 DenisChenu Note Added: 58841
2020-07-09 14:15 ollehar Note Added: 58842
2020-07-09 14:16 ollehar Note Added: 58843
2020-07-09 14:30 ollehar Note Added: 58844
2020-07-09 14:39 DenisChenu Note Added: 58845
2020-07-09 14:43 ollehar Note Added: 58847
2020-07-09 15:06 DenisChenu Note Added: 58853
2020-07-09 15:12 DenisChenu Note Added: 58859
2020-07-13 12:35 lime_release_bot Note Added: 58917
2020-07-13 12:35 lime_release_bot Status assigned => closed
2020-07-13 14:32 DenisChenu Status closed => feedback
2020-07-13 14:32 DenisChenu Resolution fixed => reopened
2020-07-13 14:32 DenisChenu Note Added: 58921
2020-07-13 14:32 DenisChenu Note Added: 58922
2020-07-13 14:32 DenisChenu File Added: Peek 13-07-2020 14-32.gif
2020-07-13 14:32 DenisChenu Status feedback => assigned
2020-08-06 14:08 DenisChenu Changeset attached => LimeSurvey master 17f0a0d5
2020-08-06 14:08 DenisChenu Note Added: 59339
2020-08-06 14:08 DenisChenu Assigned To pstelling => DenisChenu
2020-08-06 14:08 DenisChenu Resolution reopened => fixed
2020-08-06 17:32 lime_release_bot Note Added: 59346
2020-08-06 17:32 lime_release_bot Status assigned => closed