Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sort_by not being filtered in search form #1252

Merged
merged 1 commit into from Nov 9, 2021
Merged

Fix sort_by not being filtered in search form #1252

merged 1 commit into from Nov 9, 2021

Conversation

noobpk
Copy link
Contributor

@noobpk noobpk commented Nov 9, 2021

Check sort_by value before query

Check the sort_by value in the request with the default table before entering the query.
Fix bug Time-Based Blind SQL Injection

@noobpk

This comment has been minimized.

@glensc

This comment has been minimized.

@glensc

This comment has been minimized.

Check the sort_by value in the request with the default table before entering the query.
Fix bug Time-Based Blind SQL Injection

Disclosure: https://huntr.dev/bounties/668789af-8781-461c-99bb-d159e5a1d877/
Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>
@glensc glensc changed the title Fix bug XSS and Blind SQL Fix sort_by not being filtered in search form Nov 9, 2021
@glensc
Copy link
Member

glensc commented Nov 9, 2021

rebased, added changelog myself. enabled auto merge

ps: fixed "Disclosure" typo in your commit message.

@glensc glensc added this to the 3.10.8 milestone Nov 9, 2021
@glensc glensc merged commit aea72ef into eventum:master Nov 9, 2021
@glensc
Copy link
Member

glensc commented Nov 9, 2021

Damn, the PR is buggy. should have tested it locally.

Notice: Undefined variable: default_sort_by_options in eventum-3.10.7-18-g9b1e14f15/lib/eventum/class.search.php on line 377

Warning: in_array() expects parameter 2 to be array, null given in eventum-3.10.7-18-g9b1e14f15/lib/eventum/class.search.php on line 377

glensc added a commit that referenced this pull request Nov 9, 2021
@glensc
Copy link
Member

glensc commented Nov 9, 2021

However, the $sort_by was previously escaped by DB_Helper::escapeString():

are you saying the method is not escaping properly?

@glensc
Copy link
Member

glensc commented Nov 9, 2021

Also, this fix completely disables searching by custom fields:

@glensc
Copy link
Member

glensc commented Nov 9, 2021

And resetting the $sort_by = '' will totally crash the application with 500 error because then the SQL is built with order column but not value:

ORDER BY desc, iss_id DESC

@glensc
Copy link
Member

glensc commented Nov 9, 2021

@noobpk where or how did you obtain what to fill as $sort_by whitelisted values?

@glensc
Copy link
Member

glensc commented Nov 9, 2021

ok. there's dropdown for sort_by, let's go with that:

<label>Sort By<br />
  | <select name="sort_by">
  | <option value="last_action_date" >Last Action Date</option>
  | <option value="pri_rank" >Priority</option>
  | <option value="iss_id" >Issue ID</option>
  | <option value="sta_rank" >Status</option>
  | <option value="iss_summary" >Summary</option>
  | </select>
  | </label>

@glensc
Copy link
Member

glensc commented Nov 9, 2021

New fix:

so far the injected SQL is not appearing in the query log.

@noobpk
Copy link
Contributor Author

noobpk commented Nov 10, 2021

Damn, the PR is buggy. should have tested it locally.

Notice: Undefined variable: default_sort_by_options in eventum-3.10.7-18-g9b1e14f15/lib/eventum/class.search.php on line 377

Warning: in_array() expects parameter 2 to be array, null given in eventum-3.10.7-18-g9b1e14f15/lib/eventum/class.search.php on line 377

ops!! sorry about that. my mistake :((

@noobpk noobpk deleted the fix-timebase-sql branch November 10, 2021 04:12
@glensc
Copy link
Member

glensc commented Nov 10, 2021

ops!! sorry about that. my mistake :((

well. the whole thing was untested. see other notes as well. had to re-do the whole fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants