Fix Issue 2765 - rule option: migrate to GeoIP2 database and libmaxminddb API. #3610
Conversation
|
Apparently there is no libmaxminddb library package available for Cygwin, thus the test build on appveyor is failing because it says it cannot find the library. I've searched the web and did not find a binary package for Cygwin I could reference. I did find source code for maybe creating a binary package, though, using cygport. So does this mean "geoip" support in Suricata is going to die with the end-of-life of the legacy GeoIP database, or is there another alternative for including the GeoIP2 format supported by libmaxminddb in Suricata? |
| /* If ISO country code was found, then return it */ | ||
| if (entry_data.has_data) { | ||
| if (entry_data.type == MMDB_DATA_TYPE_UTF8_STRING) { | ||
| char *country_code = strndup((char *)entry_data.utf8_string, |
There was a problem hiding this comment.
Why is the string copied here? It seems unnecessary. If it is needed, please use SCStrdup instead.
There was a problem hiding this comment.
The library uses memory-mapped file I/O, so there is a remote chance that a string pointer returned may be voided later, especially if the database is closed. The MaxMind sample code makes a copy of the string, so I felt that was the safer route here. I will switch to the SCStrdup() function.
There was a problem hiding this comment.
After another more careful look I realized the function needed here is strndup() (a string copy with specified length) and not strdup(). This is because the MaxMind DB entries are not null-terminated strings. The data length is given as part of the returned data structure. I did not find an equivalent Suricata function for strndup(). I see SCStrdup() but I do not see SCStrndup().
There was a problem hiding this comment.
Ok fair enough, but then I would prefer a proper SCStrndup implementation to be introduced. It should be quite simple to add that.
|
Our Windows support efforts are now focused on MinGW, which does have a libmaxminddb package. In any case I would like to get this work merged, even if not all platforms support it. |
|
On 14-01-19 14:25, Bill Meeks wrote:
Good idea. The default should generally be
"DATADIR/GeoIP/GeoLite2-Country.mmdb". I will make the change.
Some tools/libs, like libmagic, will fall back to a built-in default
that is correct for most (all?) platforms if passed a NULL. Might be
worth having a look if that is true here as well.
|
|
I believe all review comments have been resolved and this request is ready for acceptance and merging. |
In case of bad IPv4, TCP or UDP, the per packet ip4vars/tcpvars/udpvar
structures would not be cleaned up because the cleanup depends on the
'header' pointer being set, but the error handling would unset that.
This could mean these structures were already filled with values before
the error was detected. As packets were recycled, the next packet decoding
would use this unclean structure.
To make things worse these structures are part of unions. IPv4/IPv6 and
TCP/ICMPv4/ICMPv6 share the same memory location.
LibFuzzer+UBSAN found this both locally and in Oss-Fuzz:
decode-ipv6.c:654:9: runtime error: load of value 6, which is not a valid value for type 'bool'
#0 0x6146f0 in DecodeIPV6 /src/suricata/src/decode-ipv6.c:654:9
#1 0x617e96 in DecodeNull /src/suricata/src/decode-null.c:70:13
#2 0x9dd8a4 in DecodePcapFile /src/suricata/src/source-pcap-file.c:412:9
#3 0x4c8ed2 in LLVMFuzzerTestOneInput /src/suricata/src/tests/fuzz/fuzz_sigpcap.c:158:25
#4 0x457e51 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:556:15
#5 0x457575 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:470:3
#6 0x459917 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:698:19
#7 0x45a6a5 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:830:5
#8 0x448728 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:824:6
#9 0x472552 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:19:10
#10 0x7ff0d097b82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
#11 0x41bde8 in _start (/out/fuzz_sigpcap+0x41bde8)
Bug: OISF#3610
In case of bad IPv4, TCP or UDP, the per packet ip4vars/tcpvars/udpvar
structures would not be cleaned up because the cleanup depends on the
'header' pointer being set, but the error handling would unset that.
This could mean these structures were already filled with values before
the error was detected. As packets were recycled, the next packet decoding
would use this unclean structure.
To make things worse these structures are part of unions. IPv4/IPv6 and
TCP/ICMPv4/ICMPv6 share the same memory location.
LibFuzzer+UBSAN found this both locally and in Oss-Fuzz:
decode-ipv6.c:654:9: runtime error: load of value 6, which is not a valid value for type 'bool'
#0 0x6146f0 in DecodeIPV6 /src/suricata/src/decode-ipv6.c:654:9
#1 0x617e96 in DecodeNull /src/suricata/src/decode-null.c:70:13
#2 0x9dd8a4 in DecodePcapFile /src/suricata/src/source-pcap-file.c:412:9
#3 0x4c8ed2 in LLVMFuzzerTestOneInput /src/suricata/src/tests/fuzz/fuzz_sigpcap.c:158:25
#4 0x457e51 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:556:15
#5 0x457575 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:470:3
#6 0x459917 in fuzzer::Fuzzer::MutateAndTestOne() /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:698:19
#7 0x45a6a5 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:830:5
#8 0x448728 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:824:6
#9 0x472552 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:19:10
#10 0x7ff0d097b82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
#11 0x41bde8 in _start (/out/fuzz_sigpcap+0x41bde8)
Bug: OISF#3610
Make sure these boxes are signed before submitting your Pull Request -- thank you.
Link to redmine ticket: #2765
Describe changes:
PRScript output (if applicable):