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

fix bug in SSLParser #8306

Closed
wants to merge 1 commit into from
Closed

fix bug in SSLParser #8306

wants to merge 1 commit into from

Conversation

focksor
Copy link

@focksor focksor commented Dec 20, 2022

Make sure these boxes are signed before submitting your Pull Request -- thank you.

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/5767

Describe changes:

  • When pass a JA3Buffer *ja3_elliptic_curves into function TLSDecodeHSHelloExtensionEllipticCurves, the buffer may be freed by Ja3BufferAddValue and set to NULL. But the raw pointer of the buffer will not change in this case.
  • The same thing also happened in function TLSDecodeHSHelloExtensionEllipticCurvePF

#suricata-verify-pr:
#suricata-verify-repo:
#suricata-verify-branch:
#suricata-update-pr:
#suricata-update-repo:
#suricata-update-branch:
#libhtp-pr:
#libhtp-repo:
#libhtp-branch:

@jufajardini
Copy link
Contributor

Hello, thanks for this contribution :)
Is there a redmine ticket for this bug? If not, could you create one, please?

@jufajardini jufajardini added the needs ticket Needs (link to) redmine ticket label Dec 20, 2022
@victorjulien victorjulien requested review from catenacyber and removed request for victorjulien December 21, 2022 10:24
@focksor
Copy link
Author

focksor commented Dec 25, 2022

added ticket

@catenacyber
Copy link
Contributor

Thanks @focksor

This looks legit... Quick remarks :
Would you have a test case for this ? Like a suricata-verify test ?
Could you get the CI green ?
Let me know if you have questions about this...
See https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing

@focksor focksor closed this Dec 26, 2022
@catenacyber
Copy link
Contributor

Why did you close this ? Are you opening a new PR ?

@focksor
Copy link
Author

focksor commented Dec 26, 2022

sorry, i'm not a professional developer of suricate and i don't know how to create a test case. i just read the code and happened to find out there is a bug. i think maybe i'm wasting your time because i cannot give you more information about this bug. that's the reason why i closed this pull request.

@catenacyber
Copy link
Contributor

sorry, i'm not a professional developer of suricate and i don't know how to create a test case.

You can take a look at https://github.com/OISF/suricata-verify

i just read the code and happened to find out there is a bug. i think maybe i'm wasting your time because i cannot give you more information about this bug. that's the reason why i closed this pull request.

How does the bug manifest ?
Is there a set fault ? a miscalculated ja3 ?

@focksor
Copy link
Author

focksor commented Dec 26, 2022

it obviously will cause a segment fault because of the dangling pointer

@catenacyber
Copy link
Contributor

Ok, I see the bug. It happens if realloc fails...
Could the fix rather be to have Ja3BufferAddValue use a JA3Buffer *buffer as argument and not call Ja3BufferFree in case of failure ?..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs ticket Needs (link to) redmine ticket
3 participants