Optimization #7966
openRelevance of QA UBSan article
Description
I fuzz suricata 7.0.10 and recently I decided to enable some of undefined behavior sanitizers which led to a bunch of errors (mostly integer overflow). So I was looking for similar exsting issues and found QA UBSan article.
This excludes 3 tests from being fatal:
- vptr, because clang errors out otherwise
- unsigned-integer-overflow, because we rely on this in the TCP sequence number tracking.
- unaligned, because this is a minor issue on the platforms we are most used on and there are some open issues
- I don't have any problems with enabling vptr.
- This doesn't look good to me. Any undefined behavior must be properly handled. If wraping value around integer range is desired there's better way to do it.
- UBSan report also doesn't look good to me and so I guess for my regulators too.
This article seems to be very old, so basicly I want to know if this changed or fixed in never versions of suricata or if there a serious reason why it won't be fixed.
VJ Updated by Victor Julien 9 months ago
- Assignee changed from OISF Dev to Philippe Antoine
- Git IDs updated (diff)
PA Updated by Philippe Antoine 9 months ago
- Status changed from New to Assigned
PA Updated by Philippe Antoine 3 months ago
If wraping value around integer range is desired there's better way to do it.
What would be the way to do this ?
unaligned
Seems we want this indeed cf https://github.com/OISF/suricata/pull/15207
PA Updated by Philippe Antoine 3 months ago
- Tracker changed from Security to Optimization
- Severity deleted (
MODERATE) - Affected Versions deleted (
7.0.10)
SZ Updated by Sergey Zhidkih 9 days ago
Philippe Antoine wrote in #note-3:
If wraping value around integer range is desired there's better way to do it.
What would be the way to do this ?
Add explanatory comments to the code, create a documented UBSan blacklist, or add wrappers that are enabled only in UBSan builds, similar to the existing Valgrind wrappers or use explicit integer truncation like:
/* Macro's for comparing Sequence numbers * Page 810 from TCP/IP Illustrated, Volume 2. */ #define SEQ_EQ(a,b) ((int32_t)((int64_t)(a) - (int64_t)(b)) == 0) #define SEQ_LT(a,b) ((int32_t)((int64_t)(a) - (int64_t)(b)) < 0) #define SEQ_LEQ(a,b) ((int32_t)((int64_t)(a) - (int64_t)(b)) <= 0) #define SEQ_GT(a,b) ((int32_t)((int64_t)(a) - (int64_t)(b)) > 0) #define SEQ_GEQ(a,b) ((int32_t)((int64_t)(a) - (int64_t)(b)) >= 0) #define SEQ_MIN(a, b) (SEQ_LT((a), (b)) ? (a) : (b)) #define SEQ_MAX(a, b) (SEQ_GT((a), (b)) ? (a) : (b))
Which is free operation, but doesn't cause any problems with ubsan.
unaligned
Seems we want this indeed cf https://github.com/OISF/suricata/pull/15207
If I will have free time I will report all findings. But if suricata is distributed only for platforms supporting unaligned access it should be explicitly stated in documentation otherwise it's a vulnerability.