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

[SECURITY] Denial of service because of unsafe regex processing #8680

Closed
ghost opened this issue Jun 10, 2021 · 10 comments · Fixed by #8751
Closed

[SECURITY] Denial of service because of unsafe regex processing #8680

ghost opened this issue Jun 10, 2021 · 10 comments · Fixed by #8751

Comments

@ghost
Copy link

ghost commented Jun 10, 2021

I have tried to contact you by cbioportal@cbio.mskcc.org and asked for any other email in #8658. Nobody replied.

The cBioPortal is vulnerable to regex injection that may lead to Denial of Service.

User controlled heatmap and alteration are used to build and run a regex expression:

String heatMap = request.getParameter("heat_map");
String gene = request.getParameter("gene");
String alterationType = request.getParameter("alteration");

The value end up in getAlteredCases
if (parts[1].equals(alterationType)
|| parts[1].matches("^"+alterationType+"[;\\t]")
|| parts[1].matches(".+[;\\t]"+alterationType+"[;\\t]")
|| parts[1].matches(".+[;\\t]"+alterationType+"$")) {

Since the attacker controls the string and the regex pattern he may cause a ReDoS by regex catastrophic backtracking on the server side.

@inodb
Copy link
Member

inodb commented Jun 15, 2021

CC @adamabeshouse

@inodb inodb added the security label Jun 15, 2021
@inodb
Copy link
Member

inodb commented Jun 15, 2021

Thanks for reporting! I don't think this code is used anymore actually? Maybe we can delete it?

@jjgao jjgao closed this as completed Jun 22, 2021
@ghost
Copy link
Author

ghost commented Jun 28, 2021

Hi,
Do you plan to release a GitHub security advisory and/or request CVE number?

@ghost
Copy link
Author

ghost commented Jun 28, 2021

Oh, did you just close the issue without fixing the code? Even if the parameters are not used the code is still callable. Do I miss something?

@ghost
Copy link
Author

ghost commented Jul 13, 2021

I thought since the issue was so brutally closed without explanation maybe my code analysis is wrong and it is not expoitable. Thus I have followed the instructions from https://docs.cbioportal.org/2.1.1-deploy-with-docker-recommended/docker and ran a local instance of cbioportal in container. I have a proof of concept when just a single request makes server cpu to consume 100% indefinetely. Please create a security advisory where you could invite me and discuss it in private if you have any questions.

It makes me sad that such a noble project makes it hard to responsibly disclose a security issue that may potentially lead to Denial of Service. Please respond in 24 hours.

@jjgao
Copy link
Member

jjgao commented Jul 15, 2021

@edvraa Thanks for reporting this. The code is not being used in production anymore. Also, we planned to retire both core and portal modules once all dependencies are removed (cBioPortal/icebox#161), so at this moment, we will not invest time fixing issues in these two modules that will not be running in production.

@ghost
Copy link
Author

ghost commented Jul 15, 2021

@jjgao The question is not if it is used or not. Single request to http://cbioportal.org/ProteinArraySignificanceTest.json?heat_map=censored&gene=censored&alteration=censored will make the web server consume 100% CPU. Multiple requests like this may potentially take down the server. Since it is not used, commenting out the function or disabling the route sounds as easy fix, right?

@adamabeshouse
Copy link
Contributor

@edvraa thanks for reporting this! The endpoint has now been deleted in master.

@ghost
Copy link
Author

ghost commented Jul 20, 2021

A pity it didn't make it into https://github.com/cBioPortal/cbioportal/releases/tag/v3.6.21 by one hour.

@adamabeshouse
Copy link
Contributor

adamabeshouse commented Jul 20, 2021

We release frequently and it will be in the next one.

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

Successfully merging a pull request may close this issue.

4 participants