-
Notifications
You must be signed in to change notification settings - Fork 106
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
ReDoS vulnerability in svnurl.py #287
Comments
Related: #256 |
I doubt there's anyone using this code, so I don't think it would warrant any sort of security notification that users should bother with, but if you prepare a PR for this I'll merge & release it. |
As @skialpine mentions, the advisory now triggers pip-audit and thus can fail CI runs (like this one). Is there a fix on the horizon? |
The issue is not considered critical, im not aware of anyone working towards a fix |
Well, congratulations to whoever it is that decided that the right path of action here is getting a CVE for...
Given that it will still take some time for pytest to get fully rid of it's
Sorry if I sound frustrated. But requesting a CVE without understanding the context of the project/issue you're reporting, and then generating false reports for hundreds of thousands of pytest users is doing a major disservice to both pytest/pylib maintainers and all those users. |
I'd go as far as to label this cve report a supply chain attack Pytest should keep dropping pylib as it does The cve should be added as common false positive |
I would not, but I'd certainly add it to my "examples of behavior causing open source maintainer burnout" list, given that we're now getting the first pytest issue about this, and it almost certainly won't be the last one. |
I should have known better that this was going to happen when I replied as I did...
If someone can get the notification retracted that'd be best.
But I guess since they've already put out a security notification to the entire world and their grandmother through GitHub, the path of least resistence would be to issue a release to fix this "security issue". I'll try to do it today.
|
@bluetech @The-Compiler i woudnt mind to cut a release that drops the svn wc stuff alltogether |
@RonnyPfannschmidt agreed, and perhaps a bit more even - I opened #288 with some overview on that. |
FWIW, I've also proposed adding a note to the GitHub advisory (github/advisory-database#761) and tweeted a PSA. |
GitHub have now reacted and amended the advisory:
and apparently also added that information for Dependabot alerts:
|
I've filed a CVE rejection request with MITRE, since they're the CVE CNA in this case. If they successfully reject it, getting it removed from GHSA should also be easy. |
Actually, looks like I'll be able to propose a GHSA withdrawal even if the CVE itself hasn't been retracted. I'll open a PR for that in a moment. |
I've filed github/advisory-database#762 to mark the GHSA report as withdrawn. |
@The-Compiler agreeable and funny #287 (comment) 😁 @woodruffw thanks for following up on the advisory! That means that |
Once that's merged and the OSV entry is marked as withdrawn, yeah. |
@The-Compiler @bluetech @woodruffw & other maintainers: don't sweat it! Even though 18 years ago, someone wrote a regex, not thinking of 2022-levels of security paranoia, your efforts are still very much appreciated and your contributions are incredibly productive. The CVE did trigger some alarms in builds of valuable systems used by financial institutions. But that's good. It doesn't necessarily mean that pytest is suddenly unusable. It just forces downstream developers in highly secured places to stop and think. Alerts can be ignored, assessed, categorized, accepted,... Please look at CVEs as mostly just a catalyst. Having the CVE removed is a harsh reaction, but the right one if we're absolutely sure that that particular regex is not exploitable. But try to think of it from the point of view of a security officer, with full paranoia goggles on (and no intimate knowledge of the module). The regex is present in the code. It could be executed. A hacker could penetrate, set up a SVN repo, craft some commands, take advantage of other existing vulnerabilities on the system,... You have to think worst case and then some. In the end, I think a correctly worded CVE with a low severity score which clearly includes all the IFs, would have been the best option here. Thanks! |
Taking the freedom to quote what I wrote over at github/advisory-database#762 (comment):
|
Just to make sure there's no confusion: I'm not a maintainer of this project or of With that being said: I disagree that it's good that a CVE was filed for this behavior. The fact that one was filed and published seemingly without any review represents a series of communication and authority breakdowns; the fact that the project's own maintainers have done more investigation into exploitability potential than the original reporter seems to have is an indicator of this. As for why that matters: not everybody is a bank, with roles dedicated to reviewing a constant deluge of security reports. In the context of more limited resources, managing security fatigue is far more important than reporting weakness classes like ReDoS, which don't manifest as exploitable vulnerabilities in the overwhelming majority of cases. When users get tired of useless reports, they disable their security tools entirely. |
First of all, I did fill for a CVE, but I didn't publish it. Someone else did it. I find illogical it was you, but the GH advisory says "Credits - @The-Compiler". |
Is the POC in the description actually working for anyone?
For me it executes everything in a fraction of a second. |
It's thinkable that recent python versions have complexity enhancements |
See pytest-dev/py#287 (comment) for more info.
Update Python Package Version - find issue: pytest-dev/py#287
Ignore Safety ID 51457 dues to a false-positive reported here: pytest-dev/py#287
Upgrade plugin dependencies to cover security risks Ignore Safety ID 51457 dues to a false-positive reported here: pytest-dev/py#287 Ignore bandit B113 request timeout issue
Resolves CVE: pytest-dev/py#287 (comment)
Dependabot flagged 'py' as being vulnerable to a DoS. It likely doesn't affect us, but the recommendation is that 'pytest' removed dependency on 'py' in 7.2.0 so let's do that. This is discussed in a thread found here: pytest-dev/py#287 (comment)
* Update pytest version to remove py dependency Dependabot flagged 'py' as being vulnerable to a DoS. It likely doesn't affect us, but the recommendation is that 'pytest' removed dependency on 'py' in 7.2.0 so let's do that. This is discussed in a thread found here: pytest-dev/py#287 (comment) * Remove 'py' py is a dependency of pytest so can be removed as of pytest version==7.2.0.
The py library through 1.11.0 for Python allows remote attackers to conduct a ReDoS (Regular expression Denial of Service) attack via a Subversion repository with crafted info data, because the InfoSvnCommand argument is mishandled. The particular codepath in question is the regular expression at py._path.svnurl.InfoSvnCommand.lspattern and is only relevant when dealing with subversion (svn) projects. Notably the codepath is not used in the popular pytest project. The developers of the pytest package have released version 7.2.0 which removes their dependency on py. Users of pytest seeing alerts relating to this advisory may update to version 7.2.0 of pytest to resolve this issue. See pytest-dev/py#287 (comment) for additional context.
…#4880) * Update pytest (which removes their dependency on py) The py library through 1.11.0 for Python allows remote attackers to conduct a ReDoS (Regular expression Denial of Service) attack via a Subversion repository with crafted info data, because the InfoSvnCommand argument is mishandled. The particular codepath in question is the regular expression at py._path.svnurl.InfoSvnCommand.lspattern and is only relevant when dealing with subversion (svn) projects. Notably the codepath is not used in the popular pytest project. The developers of the pytest package have released version 7.2.0 which removes their dependency on py. Users of pytest seeing alerts relating to this advisory may update to version 7.2.0 of pytest to resolve this issue. See pytest-dev/py#287 (comment) for additional context. * Added conditions so that we keep using 7.0.0 on python 3.6
fix security issues pytest upgrade to 7.2.0
### Generic implementation of a processing graph Remove explicit mentions to /splits or /first-rows from code, and move them to the "processing graph": ```json { "/splits": {"input_type": "dataset", "required_by_dataset_viewer": true}, "/first-rows": {"input_type": "split", "requires": "/splits", "required_by_dataset_viewer": true}, } ``` This JSON (see libcommon.config) defines the *processing steps* (here /splits and /first-rows) and their dependency relationship (here /first-rows depends on /splits). It also defines if a processing step is required by the Hub dataset viewer (used to fill /valid and /is-valid). A processing step is defined by the endpoint (/splits, /first-rows), where the result of the processing step can be downloaded. The endpoint value is also used as the cache key and the job type. After this change, adding a new processing step should consist in: - creating a new worker in the `workers/` directory - update the processing graph - update the CI, tests, docs and deployment (docker-compose files, e2e tests, docs, openapi, helm chart) This also means that the services (API, admin) don't contain any code mentioning directly splits or first-rows. And the splits worker does not contain direct reference to first-rows. ### Other changes - code: the libcache and libqueue libraries have been merged into libcommon - the code to check if a dataset is supported (exists, is not private, access can be programmatically obtained if gated) has been factorized and is now used before every processing step and before even accepting to create a new job (through the webhook or through the /admin/force-refresh endpoint). - add a new endpoint: /admin/cancel-jobs, which replaces the last admin scripts. It's easier to send a POST request than to call a remote script. - simplify the code of the workers by factorizing some code into libcommon: - the code to test if a job should be skipped, based on the versions of the git repository and the worker - the logic to catch errors and to write to the cache This way, the code for every worker now only contains what is specific to that worker. ### Breaking changes - env vars `QUEUE_MAX_LOAD_PCT`, `QUEUE_MAX_MEMORY_PCT` and `QUEUE_SLEEP_SECONDS` are renamed as `WORKER_MAX_LOAD_PCT`, `WORKER_MAX_MEMORY_PCT` and `WORKER_SLEEP_SECONDS`. --- * feat: 🎸 add /cache-reports/parquet endpoint and parquet reports * feat: 🎸 add the /parquet endpoint * feat: 🎸 add parquet worker Note that it will not pass the CI because - the CI token is not allowed to push to refs/convert/parquet (should be in the "datasets-maintainers" org) - the refs/convert/parquet does not exist and cannot be created for now * ci: 🎡 add CI for the worker * feat: 🎸 remove the hffs dependency we don't use it, and it's private for now * feat: 🎸 change the response format associate each parquet file with a split and a config (based on path parsing) * fix: 🐛 handle the fact that "SSSSS-of-NNNNN" is "optional" thanks @lhoestq * fix: 🐛 fill two fields to known versions in case of error * feat: 🎸 upgrade datasets to 2.7.0 * ci: 🎡 fix action * feat: 🎸 create ref/convert/parquet if it does not exist * feat: 🎸 update pytest See pytest-dev/py#287 (comment) * feat: 🎸 unlock access to the gated datasets Gated datasets with extra fields are not supported. Note also that only one token is used now. * feat: 🎸 check if the dataset is supported only for existing one * fix: 🐛 fix config * fix: 🐛 fix the branch argument + fix case where ref is created * fix: 🐛 fix logic of the worker, to ensure we get the git sha Also fix the tests, and disable gated+private for now * fix: 🐛 fix gated datasets and update the tests * test: 💍 assert that gated with extra fields are not supported * fix: 🐛 add controls on the dataset_git_revision * feat: 🎸 upgrade datasets * feat: 🎸 add script to refresh parquet response * fix: 🐛 fix the condition to test if the split exists in list also: rename functions to be more accurate * refactor: 💡 use exceptions to make the flow clearer * feat: 🎸 add processing_steps * fix: 🐛 fix signature * chore: 🤖 adapt to poetry 1.2, use pip-audit * feat: 🎸 use ProcessingStep in api service * feat: 🎸 use ProcessingStep in admin service and replace the last scripts with the /cancel-jobs/xxx endpoints. * style: 💄 fix style * feat: 🎸 update libcommon (use processing_step) * refactor: 💡 merge libcache and libqueue into libcommon * feat: 🎸 upgrade to libcommon 0.4 * feat: 🎸 upgrade to libcommon 0.4 * fix: 🐛 upgrade poetry * feat: 🎸 use processing_step in workers * feat: 🎸 implement should_skip_job and process in generic Worker this will make the code of workers simpler * feat: 🎸 handle CustomError from the workers, with specific code * feat: 🎸 simplify compute method * refactor: 💡 fix typing * fix: 🐛 remove erroneous control * feat: 🎸 update libcommon to 0.4.2 * feat: 🎸 update to libcommon 0.4.2 * ci: 🎡 fix ci * docs: ✏️ fix docstring * feat: 🎸 update to libcommon 0.4.2 * refactor: 💡 use Mapping instead of Dict * feat: 🎸 update to libcommon 0.4.2 also: replace Dict with Mapping * fix: 🐛 use Dict because it must be mutable * fix: 🐛 missing import * feat: 🎸 remplace dependency with previous_step and next_steps * feat: 🎸 define the processing graph in the configuration * feat: 🎸 upgrade to libcommon 0.5 * feat: 🎸 upgrade to libcommon 0.5 * feat: 🎸 upgrade to libcommon 0.5 * feat: 🎸 upgrade to libcommon 0.5.0 * feat: 🎸 upgrade to libcommon 0.5 * feat: 🎸 upgrade to libcommon 0.5 * refactor: 💡 add logic methods to simplify services and workers * feat: 🎸 upgrade to libcommon 0.5.1 some tests have been moved (commented yet) to e2e, since it becomes hard to simulate all the Hub endpoints -> better to test the scenari against the real Hub instead * feat: 🎸 upgrade to libcommon 0.5.1 * feat: 🎸 remove parquet processing step since it's not the scope of this PR * style: 💄 fix stykle * ci: 🎡 remove parquet ci * feat: 🎸 upgrade docker images * test: 💍 add some tests for the webhook * test: 💍 update e2e tests (and error messages in openapi) * style: 💄 fix style * feat: 🎸 remove parquet code
I've removed this change.
hi there, I am getting this CVE coming up in my project, which is using pybind with cmake, Any guidance on how I should update to remove the violation warning? Do I need to contact pybind to update their version of pytest? |
please ask those downstream, most of the team here didnt work with pybind |
im closing this issue now - this was a incorrectly filed issue right from the start, it showing up in tool reports simply indicates low quality of said tools |
Good night!
I found that this regex is vulnerable to Regular Expression Denial of Service.
PoC:
Attack vector:
An user accessing a (possibly remote) subversion repository that provides malicious "info" data.
Or an attacker injecting 'svn ls http://...' output (less realistic).
Fix:
Use a pattern with non-overlapping groups. I can help in finding a better regex and testing if needed.
The text was updated successfully, but these errors were encountered: