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

ValidateSchema::evaluate is not thread safe #2639

Closed
saiskee opened this issue Nov 10, 2021 · 2 comments
Closed

ValidateSchema::evaluate is not thread safe #2639

saiskee opened this issue Nov 10, 2021 · 2 comments

Comments

@saiskee
Copy link

saiskee commented Nov 10, 2021

Describe the bug

ValidateSchema::evaluate is not thread safe. It is setting member variables of ValidateSchema, in this case m_parserCtx when this variable is used only once in the function. This causes a use-after-free error in the following scenario:

  1. T1 runs [xmlSchemaNewParserCtxt](https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/operators/validate_schema.cc#L46)
  2. T2 runs [xmlSchemaNewParserCtxt](https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/operators/validate_schema.cc#L46)
  3. T1 runs [freeXmlSchemaParser](https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/operators/validate_schema.cc#L130)
  4. T2 runs [freeXmlSchemaParser](https://github.com/SpiderLabs/ModSecurity/blob/4127c1bf52d2b30a5c2c3e641b8085fd9a720f43/src/operators/validate_schema.cc#L130), causing a use-after-free on m_parserCtx

Expected behavior/fix
m_parserCtx is no longer needed as a member, so we use a local variable instead.

@martinhsv
Copy link
Contributor

Hi @saiskee ,

The problems with ValidateSchema were first identified (initially as a memory leak) way back with #2469 and addressed in a 3.1 branch: https://github.com/SpiderLabs/ModSecurity/tree/v3/dev/3.1-experimental

It hadn't gotten moved to v3/master so far -- partly simply due to low priority since this operator does not appear to be widely used. I will proceed to do so, however, just to clean it up if nothing else.

Although this item is, in essence, a duplicate, I'll leave it open for now as reminder since the original item was closed with the merge to the other branch.

@martinhsv
Copy link
Contributor

Closing this duplicate now, since the fix that was originally applied to a dev branch has been merged to v3/master via #3005

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

No branches or pull requests

2 participants