-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
transform/base64: check for 0-sized buffer #11869
Conversation
So as to avoid undefined behavior with a 0-sized variable length array Ticket: OISF#7296
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #11869 +/- ##
==========================================
- Coverage 82.60% 82.57% -0.03%
==========================================
Files 912 912
Lines 249351 249356 +5
==========================================
- Hits 205965 205914 -51
- Misses 43386 43442 +56
Flags with carried forward coverage won't be shown. Click here to find out more. |
Information: QA ran without warnings. Pipeline 22972 |
@@ -141,6 +141,9 @@ static void TransformFromBase64Decode(InspectionBuffer *buffer, void *options) | |||
} | |||
decode_length = nbytes; | |||
} | |||
if (decode_length == 0) { | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would leave the original buffer as-is, right? Is that the desired behavior, or should the 0 decoded bytes be the buffer so bsize:0 would match?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decoding behavior below is also pass-through in case of error, so perhaps it makes sense this way
@jlucovsky any thoughts on how it should behave?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This transform only updates the buffer when bytes are successfully decoded.
That fact might be important for some rules (no buffer if the input buffer isn't b64-encoded) and bsize: 0
would be one way to tell.
Suggestion: we always update the buffer with the number of decoded bytes -- either 0 or the actual value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, should we create another ticket for it ?
I guess other transforms should be updated to like pcrexform
Or we should even return a NULL buffer when the transform "fails"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged in #11905, thanks! |
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/7296
Describe changes:
As done already by other transforms
SV_BRANCH=OISF/suricata-verify#2075
#11866 with SV test and better check : wait after computation of offset...