ssl: first pass limit when allocating buffer for certificates #7139
Conversation
With this check, on the first packet of a certificate presenting a length of 16Mbytes, we only allocate up to 65Kb When we get to the point where need more than 65Kb, we realloc to the true size. With this check, it makes it more expensive for an attacket to use this allocation as a way to trigger ressource exhaustion...
|
Information: QA ran without warnings. Pipeline 6572 |
Codecov Report
@@ Coverage Diff @@
## master #7139 +/- ##
==========================================
+ Coverage 78.06% 78.12% +0.05%
==========================================
Files 628 628
Lines 185266 185268 +2
==========================================
+ Hits 144635 144738 +103
+ Misses 40631 40530 -101
Flags with carried forward coverage won't be shown. Click here to find out more. |
| @@ -1431,6 +1437,10 @@ static int EnsureRecordSpace(SSLStateConnp *curr_connp, const uint8_t * const in | |||
| SCLogDebug("cert_len unknown still, create small buffer to start"); | |||
| certs_len = 256; | |||
| } | |||
| // Limit in a first time allocation for very large certificates | |||
There was a problem hiding this comment.
I'm struggling a bit with understanding how this will work for actual large records. Are we getting called again in the future? A casual read of the code suggests it actually expects to be called with the full record, incl the starting bytes. If we'd have a record size that is larger than the initial input size, which will almost always be true if it is more than say 4k, do we still handle things correctly? I guess I'd love to see some test cases for this condition.
There was a problem hiding this comment.
btw my lack of understanding is more about the original code, but its relevant to judging this PR
There was a problem hiding this comment.
btw my lack of understanding is more about the original code
Oh, I was indeed assuming you were familiar with it...
One of my goals for this PR is to have minimal changes (until we get a rust tls parser)
Are we getting called again in the future?
Yes ! this function EnsureRecordSpace gets called for every packet which contains bytes of the certificate even if only the 3 first bytes are useful... 🙄
A casual read of the code suggests it actually expects to be called with the full record
I do not see this... Where do you see this ?
There was a problem hiding this comment.
It seems EnsureRecordSpace needs the first 3 bytes of the record to get the length, but I don't see it store it in a way to use it when more data comes in later? So if more data comes in, we still need the original data (start of record), right?
There was a problem hiding this comment.
I think I see my error. On consecutive calls it does use the buffered data to get the length.
There was a problem hiding this comment.
On consecutive calls it does use the buffered data to get the length.
Indeed...
So, do you expect something from me ?
|
Merged in #7187, thanks! |
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/5188
Describe changes:
Modifies #7131 with using a macro for the Magic number