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]: duplicate with #1180 #1216

Closed
songxpu opened this issue May 1, 2023 · 8 comments
Closed

[Security]: duplicate with #1180 #1216

songxpu opened this issue May 1, 2023 · 8 comments
Labels
bug security vulnerability

Comments

@songxpu
Copy link

songxpu commented May 1, 2023

Describe the bug

Hi!
Similar to previous #1165, this should be one UAF due to condition contention in the latest branch (both for NanoMQ && NanoNNG).

In fact, this vulnerability was committed to #1165#issuecomment and all of them have been fixed. But it seems that the problem is still present in the latest branch, either because of a situation that bypasses the patch or because they are caused by something more specific, so I opened this issue.

The following information is provided to facilitate reproduction.

To Reproduce
If possible include actual reproduction test code here.
Minimal C test cases are perferred.

Environment Details

  • NanoMQ version #5a8f908c09f23c54d8d82ade607d7cffda80358e
  • NanoNNG #2fc2c28d74ad2bc31c73fdf067d0e49c0c163485

image

UAF_nmq_mqtt_c_404.zip

@JaylinYu JaylinYu added the security vulnerability label May 2, 2023
@JaylinYu JaylinYu changed the title UAF in nmq_mqtt.c:404 && data tracing [Security]: Vulnerability awaits to be verified May 2, 2023
@JaylinYu
Copy link
Member

JaylinYu commented May 14, 2023

I am pretty sure this is not same issue as #1165. and I am not able to reproduce your case. Also seems like our code base is different.
I could tell this should be treated as a data racing problem, just don't sure what's your config and env. The testing raw data is sending retain msg frequently and we have figured out a way to avoid data racing in this case.

@JaylinYu JaylinYu added the Investigation quiestion remains ambiguous label May 14, 2023
@JaylinYu
Copy link
Member

However I found memleak case when testing your raw data. Thank you bro!!

@songxpu
Copy link
Author

songxpu commented May 14, 2023

I am pretty sure this is not same issue as #1165. and I am not able to reproduce your case. Also seems like our code base is different. I could tell this should be treated as a data racing problem, just don't sure what's your config and env. The testing raw data is sending retain msg frequently and we have figured out a way to avoid data racing in this case.

I test and invesigate it on the newest banch, and here's what happened.
It takes a certain randomness, even luck, to find and trigger it, as you mentioned it is a data contention problem.

If necessary, I can provide a dockerfile to make sure the environment and code are the same.

image

However I found memleak case when testing your raw data. Thank you bro!!

Glad to hear that.

Is the following the same as what you found? If so, please ignore this message.

image

@JaylinYu
Copy link
Member

This seems like a similar issue with #1180\

@JaylinYu JaylinYu changed the title [Security]: Vulnerability awaits to be verified [Security]: duplicate with #1180 May 18, 2023
@JaylinYu JaylinYu added bug and removed Investigation quiestion remains ambiguous labels May 18, 2023
@JaylinYu
Copy link
Member

JaylinYu commented May 18, 2023

Is the following the same as what you found? If so, please ignore this message.

image

yes, this is the only issue that remains here. any idea?

@songxpu
Copy link
Author

songxpu commented May 18, 2023

Is the following the same as what you found? If so, please ignore this message.
image
yes, this is the only issue that remains here. any idea?

Of course not, NanoMQ is very strong now.

@JaylinYu
Copy link
Member

JaylinYu commented May 18, 2023

I mean, any your advice on how to debug this memleak would be appreciated.
No No No, Got plenty more to do. There are more security issues somewhere. At least I need fix this memleak

@JaylinYu
Copy link
Member

All bugs spotted by this issue have been fixed.
Thank you @songxpu

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

No branches or pull requests

2 participants