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

Add check that string read is not more than max length #328

Merged
merged 7 commits into from
Jan 25, 2023

Conversation

amgross
Copy link
Contributor

@amgross amgross commented Nov 29, 2022

Pull request

Choose Correct

  • bug
  • feature

Describe the pull request

Add fix to #326 , when passing string with max length set, ensure the real length not exceeds max length
To Reproduce

Run erpcgen on:
example.erpc.txt

Expected behavior

Screenshots

Desktop (please complete the following information):

  • OS: Windows
  • eRPC Version:1.9.1

Steps you didn't forgot to do

  • I checked if other PR isn't solving this issue.
  • I read Contribution details and did appropriate actions.
  • PR code is tested.
  • PR code is formatted.
  • Allow edits from maintainers pull request option is set (recommended).

Additional context

I made just partially fix, need help for continue. What missing:

  1. {% if info.stringAllocSize != info.stringLocalName + "_len" %} is not working, don't know how to make it working
  2. Don't know how to fix client side

@amgross
Copy link
Contributor Author

amgross commented Nov 29, 2022

@Hadatko as you can see, the compilation failed due to point 1 I mentioned in Additional context, can you help me and fix my commit? I don't know how to do it

@amgross
Copy link
Contributor Author

amgross commented Nov 29, 2022

BTW, the fix is based on the decodeBinaryType template

@Hadatko
Copy link
Member

Hadatko commented Nov 29, 2022

Ok thank you. I will take a look into this.

@amgross
Copy link
Contributor Author

amgross commented Nov 30, 2022

I added fix to client side, and added the checks also in the read/write binary (was exist just in server write binary).
In my view need just to see how to fix the {% if info.stringAllocSize != info.stringLocalName + "_len" %}

@amgross amgross changed the title Add check that string theat read is not more than max length Add check that string read is not more than max length Dec 1, 2022
@amgross amgross force-pushed the readstring_len_check branch 2 times, most recently from 25022d7 to fb67a4f Compare December 1, 2022 07:14
@amgross
Copy link
Contributor Author

amgross commented Dec 1, 2022

Hi @Hadatko ,
The fix is fully ready for code review.
The failures in the tests are: FAILED to find pattern 'codec\->writeString\(strlen\(\(const\s*char\*\)data\->a\),\s*\(const\s*char\*\)data\->a\)
Because I changed the strlen to be outside the writeString so I can reuse the return value to compare it against the max value.

@Hadatko
Copy link
Member

Hadatko commented Dec 1, 2022

Can you fix the test too? Remove line, add line.

@amgross
Copy link
Contributor Author

amgross commented Dec 1, 2022

@Hadatko can you point me in which tests the YML failed?

@amgross
Copy link
Contributor Author

amgross commented Dec 1, 2022

@Hadatko I done (hopefully)

@amgross
Copy link
Contributor Author

amgross commented Dec 4, 2022

@Hadatko @MichalPrincNXP can you prioritize this PR to version 1.10.0?
It suppose to prevent buffer overflow that the client may cause on the server (and maybe also vise versa)

@Hadatko
Copy link
Member

Hadatko commented Dec 5, 2022

@amgross I put note here. Likely we do not manage to get it into 1.10.0 but into 1.11.0 it shouldn't be an issue. On develop branch it will be present once @MichalPrincNXP will have time, but we need to make decision on note i added.

@Hadatko Hadatko added this to the 1.11.0 milestone Dec 5, 2022
@amgross
Copy link
Contributor Author

amgross commented Dec 6, 2022

but we need to make decision on note i added

@Hadatko what is the note you are referring to?

@Hadatko
Copy link
Member

Hadatko commented Dec 6, 2022

image

@amgross
Copy link
Contributor Author

amgross commented Dec 7, 2022

@Hadatko You probably didn't submitted your review, see at: github documentation that "Before you submit your review, your line comments are pending and only visible to you".
image

@amgross
Copy link
Contributor Author

amgross commented Dec 7, 2022

About the asserts:
two reasons why I don't want assert in this case:

  1. Asserts are used for debug generally, in lots of systems asserts compiles out in operational mode, so because this flow may happen (and may even be attack of the client on the server) I think it shouldn't be assert.
  2. Asserts (in most systems) causes system halt/reset, so there shouldn't be flow that is not unknown bug where assert is used.

So when we should use assert:

  1. in cases that we can't think on flow it should happen and if they happen it means we have bug in current core.

Good example for it is the assert that currently exist, if erpc allocated the memory buffer and afterward got bigger length from the function that used this pointer, probably this function smashed the heap and have a bug so we should halt/reset the system.

@Hadatko
Copy link
Member

Hadatko commented Dec 9, 2022

From my point of view asserts are good to replace conditions in release target (if you would write if condition it is present in DEBUG and in RELEASE target. Using assert is only in DEBUG target and halts system).
Reason to set assert for write cases is that you as an owner are trying to writting value which can be out of range.
Reason to use updateStatus+condition for read cases is that you can receive wrong data from communication channel.

erpcgen/src/templates/c_coders.template Outdated Show resolved Hide resolved
erpcgen/src/templates/c_coders.template Show resolved Hide resolved
@Hadatko
Copy link
Member

Hadatko commented Dec 14, 2022

@amgross Just few minor changes and it is ready for merge.

@Hadatko Hadatko assigned Hadatko and amgross and unassigned Hadatko Dec 14, 2022
@Hadatko Hadatko linked an issue Dec 14, 2022 that may be closed by this pull request
@amgross
Copy link
Contributor Author

amgross commented Dec 15, 2022

@Hadatko , I think I did now what you asked

@amgross
Copy link
Contributor Author

amgross commented Dec 19, 2022

@MichalPrincNXP any comments?
@Hadatko finished reviewing.

@Hadatko
Copy link
Member

Hadatko commented Dec 19, 2022

@amgross i think @MichalPrincNXP will be available in beginning of new year. This is not critical bug so it will be likely added to next release as current one is in feature freeze state.

@amgross
Copy link
Contributor Author

amgross commented Dec 20, 2022

OK, I will wait,
In my point of view it is critical issue because the bug here may lead to buffer overflow which one attacker side can cause the other, but I understand it is not yours point of view.

@Hadatko
Copy link
Member

Hadatko commented Dec 20, 2022

@amgross Thank you for your patience

@MichalPrincNXP MichalPrincNXP merged commit 6d7dfc7 into EmbeddedRPC:develop Jan 25, 2023
@MichalPrincNXP
Copy link
Member

Thank you, @amgross , @Hadatko for the effort and the patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

[BUG] string length not enforced in client or server
3 participants