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
Content length verifier negative check #361
Conversation
Can one of the admins verify this patch? |
8 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
dbd8c37
to
d734101
Compare
Motivation: We discovered that this code path can result in an arithmetic overflow in the case where the content-length header has INT_MIN value. Since negative content-lengths don't have any meaning anyway we may as well throw if we encounter one so the decoding can be aborted. Modifications: * ContentLengthVerifier init throws a `ContentLengthHeaderNegative` error when the decoded value is < 0. * ContentLengthVerifier init throws a `ContentLengthHeaderMalformedValue` error when the value cannot be decoded, e.g. it is out of bounds for Int or is an invalid string. Result: We will no longer panic for INT_MIN content-lengths and will fail decoding if one of various invalid values is encountered.
d734101
to
ea77144
Compare
self.expectedContentLength = Int(first, radix: 10) | ||
|
||
guard let expectedLength = self.expectedContentLength else { | ||
throw NIOHTTP2Errors.contentLengthHeaderMalformedValue() | ||
} | ||
if expectedLength < 0 { | ||
throw NIOHTTP2Errors.contentLengthHeaderNegative() | ||
} |
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.
Note: more of a felling then an actual rule I follow
I usually only assign after the value is fully validated. It doesn't really matter but it signals to me at least that once it is assigned I'm done with it.
self.expectedContentLength = Int(first, radix: 10) | |
guard let expectedLength = self.expectedContentLength else { | |
throw NIOHTTP2Errors.contentLengthHeaderMalformedValue() | |
} | |
if expectedLength < 0 { | |
throw NIOHTTP2Errors.contentLengthHeaderNegative() | |
} | |
guard let expectedLength = Int(first, radix: 10) else { | |
throw NIOHTTP2Errors.contentLengthHeaderMalformedValue() | |
} | |
if expectedLength < 0 { | |
throw NIOHTTP2Errors.contentLengthHeaderNegative() | |
} | |
self.expectedContentLength = expectedLength |
ContentLengthVerifier
init
throws on negative valuesMotivation:
We discovered that this code path can result in an arithmetic overflow in the case where the content-length header has
INT_MIN
value. Since negative content-lengths don't have any meaning anyway we may as well throw if we encounter one so the decoding can be aborted.Modifications:
ContentLengthHeaderNegative
error when the decoded value is < 0.ContentLengthHeaderMalformedValue
error when the value cannot be decoded, e.g. it is out of bounds for Int or is an invalid string.Result:
We will no longer panic for
INT_MIN
content-lengths and will fail decoding if one of various invalid values is encountered.