Skip to content
This repository has been archived by the owner on Dec 9, 2022. It is now read-only.

Potential error due to the unreleased lock #1037

Closed
anson-lo opened this issue Jul 18, 2021 · 28 comments
Closed

Potential error due to the unreleased lock #1037

anson-lo opened this issue Jul 18, 2021 · 28 comments
Assignees
Labels
bug Something isn't working needs-triage

Comments

@anson-lo
Copy link
Contributor

Dear developers:
Thank you for your checking. In the method mempool_destroy, the lock &pool->lock may be not released if the branch condition satisfies and the method returns.

pthread_mutex_lock(&pool->lock);

return S3_MEMPOOL_INVALID_ARG;

int mempool_destroy(MemoryPoolHandle *handle) {
  struct mempool *pool = NULL;
  struct memory_pool_element *pool_item;
  char *log_msg_fmt = "mempool(%p): free(%p) called for buffer size(%zu)";
  char log_msg[200];

  if (handle == NULL) {
    return S3_MEMPOOL_INVALID_ARG;
  }

  pool = (struct mempool *)*handle;
  if (pool == NULL) {
    return S3_MEMPOOL_INVALID_ARG;
  }

  if ((pool->flags & ENABLE_LOCKING) != 0) {
    pthread_mutex_lock(&pool->lock); // the lock
  }

  if (*handle == NULL) {
    return S3_MEMPOOL_INVALID_ARG;  //return without releasing
  }

  ...;
}

Best,

@welcome
Copy link

welcome bot commented Jul 18, 2021

Thanks for opening this issue. A contributor should be by to give feedback soon. In the meantime, please check out the contributing guidelines and explore other ways you can get involved.

@johnbent
Copy link
Member

Thanks @anson-lo !  Just curious: how did you find this by the way?  Just reading the code or you did some automated analysis?

@nileshgovande
Copy link
Contributor

Thanks @anson-lo . You're right, this looks like a bug. Would you like to submit the patch yourself? Let us know and that'd be awesome. Otherwise, we can fix it. Thanks for catching this and reporting

@anson-lo
Copy link
Contributor Author

@johnbent It is reported by a static code scanner.
@nileshgovande I am happy to pull a request. Would be done soon.

@nileshgovande
Copy link
Contributor

Thanks, @anson-lo Which static code scanner?

@mukul-seagate11 mukul-seagate11 added the bug Something isn't working label Jul 20, 2021
@mukul-seagate11
Copy link
Contributor

Thanks, @anson-lo Which static code scanner?

Nilesh, Its reported in Codacy

@anson-lo
Copy link
Contributor Author

@nileshgovande We use Pinpoint, a static analysis tool similar to Codacy.

@nileshgovande
Copy link
Contributor

nileshgovande commented Jul 21, 2021

No @mukul-seagate11 - its not
CC @johnbent @bkirunge7

We will check on Pinpoint @anson-lo

@nileshgovande
Copy link
Contributor

PR - #1041

@nileshgovande
Copy link
Contributor

Changes merged to main

@anson-lo
Copy link
Contributor Author

@nileshgovande Hi, would this bug cause any security issues like deadlock due to acquiring the same lock?

@johnbent
Copy link
Member

FYI @swanand-gadre . This is an interesting issue where a community member used a different static analysis tool and found an issue in our S3 code. Just something you might be interested in.

@swanand-gadre
Copy link
Contributor

Thanks @johnbent for looping me in.

Similar condition is described as a weakness here -> https://cwe.mitre.org/data/definitions/401.html

Since memory lock is not released in specific condition, It leads to not sufficiently tracking and releasing allocated memory after it has been used, which slowly consumes remaining memory.

It impacts availability (reduced performance, article also mention, potential DoS attack).

Hi @nileshgovande

I see that the specific condition is now fixed with pool->lock released with pthread_mutex_unlock(&pool->lock);
Also code to reset *handle and free items in the free list does clean-up.

However do we need to also add a similar code, in

if (handle == NULL) {
return S3_MEMPOOL_INVALID_ARG;
}

Or will it not have pool-lock if handle is NULL ?

Regards
Swanand

@anson-lo
Copy link
Contributor Author

Could we apply CVE ID for this issue? (Similar to CVE-2014-3657, CVE-2015-8340,CVE-2020-12771). Thanks so much.

@johnbent
Copy link
Member

Hey @swanand-gadre , do you have an answer for @anson-lo ?

@swanand-gadre
Copy link
Contributor

Hi @anson-lo

Thank you for your interest,

Is there any POC or end user scenario available, where this condition is hit and shows evidence of impact?

@anson-lo
Copy link
Contributor Author

Unfortunately, I don't have PoC or find it via concretely triggering this bug.
I just manually examined the security impacts of this bug. I see many CVE IDs assigned without POC.
This's fine if you think it's not worth it.

@johnbent
Copy link
Member

@anson-lo , those other CVE IDs that you see are automatically created by the Whitesource bot. It would be really cool if your tool is finding vulnerabilities that Whitesource is missing!

@swanand-gadre
Copy link
Contributor

Hi @anson-lo

To further investigate, I went thru following linkes to check the details required to create CVE

https://www.kb.cert.org/vuls/report/

Along with some other details, it asks for

How does an attacker exploit this vulnerability? (Explain access or other conditions necessary to attack.)
What does an attacker gain by exploiting this vulnerability? (i.e. what is the impact?)

Also I wanted to check about the tool you mentioned you are using

Perhaps we can quickly connect so that I can understand the work you are doing?

Let me know.

Regards
Swanand

@anson-lo
Copy link
Contributor Author

@swanand-gadre Thanks so much!

How does an attacker exploit this vulnerability?
This is my understanding after eyeballing the code for a while. In the mempool_destroy, the lock pool->lock is not released correctly when *handle == NULL and the function mempool_destroy returns S3_MEMPOOL_INVALID_ARG. When mempool_create_with_shared_mem that invokes mempool_destroy is called multiple times, it could lead to reacquiring the same lock pool->lock.
Apologies for being unfamiliar with the code, this is my initial analysis, which could be refined further.

What does an attacker gain by exploiting this vulnerability?
This could lead to deadlock, wreaking a denial-of-the-service due to the reacquiring the same lock.

The tool I am working
This is a (academic purpose) static analysis tool that consists of pointer analysis, lock analysis, reachability analysis, etc. The tool is similar to SVF.

@swanand-gadre
Copy link
Contributor

Hi @anson-lo

Does the same fix applies to

if (handle == NULL) {
return S3_MEMPOOL_INVALID_ARG;
}

Do you want to try raising it with https://cve.mitre.org/cve/request_id.html

Regards
Swanand

@anson-lo
Copy link
Contributor Author

anson-lo commented Nov 1, 2021

@swanand-gadre Hi, it seems the individuals are more difficult to request this without assistance from developers. I tried before but got no reply. Could you please raise it?

@swanand-gadre
Copy link
Contributor

Hi @anson-lo

In my opinion, since your tool uncovered it, it is appropriate you raise request for CVE ID.
Typically, committee would reach out to the products, in case of doubts/queries.

However requesting you to take initial step.

Also I wanted to know your feedback, if , the same fix applies to?

if (handle == NULL) {
return S3_MEMPOOL_INVALID_ARG;
}

Regards
Swanand

@anson-lo
Copy link
Contributor Author

anson-lo commented Nov 2, 2021

@swanand-gadre Thanks, I would take an initial step.

if (handle == NULL) {
return S3_MEMPOOL_INVALID_ARG;
}
pool = (struct mempool *)*handle;
if (pool == NULL) {
return S3_MEMPOOL_INVALID_ARG;
}
if ((pool->flags & ENABLE_LOCKING) != 0) {
pthread_mutex_lock(&pool->lock);
}

From my view, applying to handle==NULL is not necessary, since the lock pool->lock is acquired later.

@swanand-gadre
Copy link
Contributor

Hi @anson-lo

Any further updates ?

Regards
Swanand

@anson-lo
Copy link
Contributor Author

@swanand-gadre Hi, I have submitted my CVE application form but haven't received any notifications so far.

@johnbent
Copy link
Member

@anson-lo , by the way, this has been fixed now in the code. So should we still file for the CVE?

@anson-lo
Copy link
Contributor Author

OK, thanks for your time and help

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working needs-triage
Projects
None yet
Development

No branches or pull requests

7 participants