CVE-2021-43429: Potential error due to the unreleased lock · Issue #1037 · Seagate/cortx-s3server

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,

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

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

Copy link

Contributor Author

anson-lo commented Jul 20, 2021

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

Thanks, @anson-lo Which static code scanner?

Nilesh, Its reported in Codacy

Copy link

Contributor Author

anson-lo commented Jul 20, 2021

Copy link

Contributor Author

anson-lo commented Oct 15, 2021

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

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.

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

Copy link

Contributor Author

anson-lo commented Oct 16, 2021

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

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?

Copy link

Contributor Author

anson-lo commented Oct 22, 2021

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.

@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!

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

Copy link

Contributor Author

anson-lo commented Oct 28, 2021

@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.

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?

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

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.

Hi @anson-lo

Any further updates ?

Regards
Swanand

Copy link

Contributor Author

anson-lo commented Nov 10, 2021

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

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

Copy link

Contributor Author

anson-lo commented Nov 17, 2021

OK, thanks for your time and help