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

[BUG] string length not enforced in client or server #326

Closed
taldinj opened this issue Nov 24, 2022 · 5 comments · Fixed by #328
Closed

[BUG] string length not enforced in client or server #326

taldinj opened this issue Nov 24, 2022 · 5 comments · Fixed by #328
Assignees
Labels
Milestone

Comments

@taldinj
Copy link

taldinj commented Nov 24, 2022

Hi,
First off, trying out eRPC and impressed by what I see so far. Great work.

I have a command with a maximum of string length associated with the parameter. The max length (@max_length) is not properly enforced in either client nor server:

  • On client side the generated code simply writes the string with writeString(strlen(), (const char*)str, (const char*)), no validation is done
  • On server side it actually looks worse:
  1. codec->readString() is used, populating size and pointer (uint32_t str_len, char * str_local)
  2. Allocation is done with the @max_length+1
  3. If the read string is not NULL and allocation was successful memcpy() is used with the length of the read string (possible buffer overflow if string sent exceeds @max_length)

The issues I see:

  1. Client side can and should avoid sending content that violets the @max_length
  2. No validation of size, possible buffer overflow(?)

Thank you!

@taldinj taldinj added the bug label Nov 24, 2022
@github-actions
Copy link

Hi eRPC user. Thank you for your interest and welcome. We hope you will enjoy this framework well.

@Hadatko
Copy link
Member

Hadatko commented Nov 24, 2022

HI @taldinj, you may be correct that there is missing validation. Thank you for your input. Once i will have time i will improve this behavior.

@amgross
Copy link
Contributor

amgross commented Dec 1, 2022

PR #328 is for solving this issue

@amgross
Copy link
Contributor

amgross commented Dec 1, 2022

As @taldinj mentioned, we have here security issue od buffer overflow. so I will be happy to push this change to 1.10.0

@Hadatko
Copy link
Member

Hadatko commented Dec 5, 2022

@amgross Thank you for your effort in our project ;)

@Hadatko Hadatko added this to the 1.11.0 milestone Dec 5, 2022
@Hadatko Hadatko linked a pull request Dec 14, 2022 that will close this issue
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants