Starting with 75e300aeec ("blobmsg: fix wrong payload len passed from
blobmsg_check_array") blobmsg_check_array_len() gets *blob* length
passed as argument. It cannot be used with __blobmsg_for_each_attr()
which expects *data* length.
Use blobmsg_for_each_attr() which calculates *data* length on its own.
The same bug was already reported in the past and there was fix attempt
in the commit cd75136b13 ("blobmsg: fix wrong payload len passed from
blobmsg_check_array"). That change made blobmsg_check_attr_len() calls
fail however.
This is hopefully the correct & complete fix:
1. blobmsg_check_array_len() gets *blob* length
2. It calls blobmsg_check_attr_len() which requires *blob* length
3. It uses blobmsg_for_each_attr() which gets *data* length
This fixes iterating over random memory treated as attrs. That was
resulting in check failing randomly for totally correct blobs. It's
critical e.g. for procd project with its instance_fill_array() failing
and procd not starting services.
Fixes: 75e300aeec ("blobmsg: fix wrong payload len passed from blobmsg_check_array")
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
Seems like the CI runners are slower and produce different test output:
- [0/1] finish 'sleep 1' (killer)
[1/1] start 'sleep 1' (sleeper)
+ [1/1] finish 'sleep 1' (killer)
+ [1/1] finish 'sleep 1' (killer)
[1/1] cancel 'sleep 1' (sleeper)
[0/1] finish 'sleep 1' (sleeper)
[1/1] start 'sleep 1' (sleeper)
Lets try to fix it by lowering the killing timeout.
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Fixes a use-after-free bug in runqueue_task_kill():
Invalid read of size 8
at runqueue_task_kill (runqueue.c:200)
by uloop_process_timeouts (uloop.c:505)
by uloop_run_timeout (uloop.c:542)
by uloop_run (uloop.h:111)
by main (tests/test-runqueue.c:126)
Address 0x5a4b058 is 24 bytes inside a block of size 208 free'd
at free
by runqueue_task_complete (runqueue.c:234)
by runqueue_task_kill (runqueue.c:199)
by uloop_process_timeouts (uloop.c:505)
by uloop_run_timeout (uloop.c:542)
by uloop_run (uloop.h:111)
by main (tests/test-runqueue.c:126)
Block was alloc'd at
at calloc
by add_sleeper (tests/test-runqueue.c:101)
by main (tests/test-runqueue.c:123)
Since commit 11e8afea (runqueue should call the complete handler from
more places) the call to the complete() callback has been moved to
runqueue_task_complete(). However in runqueue_task_kill()
runqueue_task_complete() is called before the kill() callback. This
will result in a use after free if the complete() callback frees the
task struct.
Furthermore runqueue_start_next() is already called at the end of
runqueue_task_complete(), so there is no need to call it again in
runqueue_task_kill().
The issue was that the _complete() callback frees the memory used by the
task struct, which is then read after the _complete() callback returns.
Ref: FS#3016
Signed-off-by: Alban Bedel <albeu@free.fr>
[initial test case, kill cb comment fix]
Signed-off-by: Chris Nisbet <nischris@gmail.com>
[testcase improvements and commit subject/description tweaks]
Signed-off-by: Petr Štetiar <ynezz@true.cz>
The comment relating to the runqueue task structure 'cancel' callback
indicated that the callback 'calls' runqueue_task_complete, which
isn't quite right. The callback _should_ call runqueue_task_complete.
Signed-off-by: Chris Nisbet <nischris@gmail.com>
* add a test for blobmsg_check_array() to test an array with a string in it
This test was added in conjunction with a change to blobmsg_check_array() to
get it to pass the length obtained from blob_len() rather than blobmsg_len().
Signed-off-by: Chris Nisbet <nischris@gmail.com>
Fix incorrect use of blobmsg_len() on passed blobmsg to
blobmsg_check_array_len() introduced in commit 379cd33d19
("fix wrong payload len passed from blobmsg_check_array") by using correct
blob_len().
By using blobmsg_len() a value too small was passed to blobmsg_check_array()
which could lead to this function returning an error when there is none.
Fixes: 379cd33d19 ("fix wrong payload len passed from blobmsg_check_array")
Signed-off-by: Chris Nisbet <nischris@gmail.com>
[add fixes tag, rewrap commit message]
Signed-off-by: Jo-Philipp Wich <jo@mein.io>
Fix out of bounds read in blobmsg_parse and blobmsg_check_name. The
out of bounds read happens because blob_attr and blobmsg_hdr have
flexible array members, whose size is 0 in the corresponding sizeofs.
For example the __blob_for_each_attr macro checks whether rem >=
sizeof(struct blob_attr). However, what LibFuzzer discovered was,
if the input data was only 4 bytes, the data would be casted to blob_attr,
and later on blob_data(attr) would be called even though attr->data was empty.
The same issue could appear with data larger than 4 bytes, where data
wasn't empty, but contained only the start of the blobmsg_hdr struct,
and blobmsg_hdr name was empty. The bugs were discovered by fuzzing
blobmsg_parse and blobmsg_array_parse with LibFuzzer.
CC: Luka Perkov <luka.perkov@sartura.hr>
Reviewed-by: Jo-Philipp Wich <jo@mein.io>
Signed-off-by: Juraj Vijtiuk <juraj.vijtiuk@sartura.hr>
[refactored some checks, added fuzz inputs, adjusted unit test results]
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Help detecting Valgrind OOB reads and other issues.
Conditional jump or move depends on uninitialised value(s)
at 0x5452886: blobmsg_parse (blobmsg.c:203)
by 0x400A8E: test_blobmsg (tests/test-blobmsg-parse.c:66)
by 0x400A8E: main (tests/test-blobmsg-parse.c:82)
Conditional jump or move depends on uninitialised value(s)
at 0x545247F: blobmsg_check_name (blobmsg.c:39)
by 0x545247F: blobmsg_check_attr_len (blobmsg.c:79)
by 0x5452710: blobmsg_parse_array (blobmsg.c:159)
by 0x400AB8: test_blobmsg (tests/test-blobmsg-parse.c:69)
by 0x400AB8: main (tests/test-blobmsg-parse.c:82)
Conditional jump or move depends on uninitialised value(s)
at 0x54524A0: blobmsg_check_name (blobmsg.c:42)
by 0x54524A0: blobmsg_check_attr_len (blobmsg.c:79)
by 0x5452710: blobmsg_parse_array (blobmsg.c:159)
by 0x400AB8: test_blobmsg (tests/test-blobmsg-parse.c:69)
by 0x400AB8: main (tests/test-blobmsg-parse.c:82)
Ref: http://lists.infradead.org/pipermail/openwrt-devel/2020-January/021204.html
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Better safe than sorry and while at it prefer use of PRId16 and PRId32
formatting constants as well.
Reviewed-by: Jo-Philipp Wich <jo@mein.io>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Better safe than sorry and while at it add handling of possible
*printf() failures.
Reviewed-by: Jo-Philipp Wich <jo@mein.io>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Add a new `ABIVERSION` define which allows to control the SOVERSION used
for the built shared library. This is needed for downstream packaging to
properly track breaking ABI changes when updating to newer versions of
the library.
Suggested-by: Jo-Philipp Wich <jo@mein.io>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
libjson-c json_type_int values are stored as int64_t. Use
json_object_get_int64() instead of json_object_get_int()
to avoid clamping to INT32_MAX.
Reviewed-by: Jo-Philipp Wich <jo@mein.io>
Signed-off-by: Dainis Jonitis <dainis.jonitis@ubnt.com>
[fixed author to match SoB, added unit test results]
Signed-off-by: Petr Štetiar <ynezz@true.cz>
int16 blobmsg type is currently being serialized as uint16_t due to
missing cast during JSON output.
Following blobmsg content:
bar-min: -32768 (i16)
bar-max: 32767 (i16)
Produces following JSON:
{ "bar-min":32768,"bar-max":32767 }
Whereas one would expect:
{ "bar-min":-32768,"bar-max":32767 }
Reviewed-by: Jo-Philipp Wich <jo@mein.io>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
* add missing test with sanitizers
* add test case for blobmsg_add_json_from_string
* add test cases for all numeric types
* print types for each variable
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Include shunit2 based tests into unit testing pipeline until
(eventually) it's converted to cram based unit tests.
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Fix incorrect use of blob_raw_len() on passed blobmsg to
blobmsg_check_array_len() introduced in commit b0e21553ae ("blobmsg:
add _len variants for all attribute checking methods") by using correct
blobmsg_len().
This wrong (higher) length was then for example causing issues in
procd's instance_config_parse_command() where blobmsg_check_attr_list()
was failing sanity checking of service command, thus resulting in the
startup failures of some services like collectd, nlbwmon and samba4.
Ref: http://lists.infradead.org/pipermail/openwrt-devel/2019-December/020840.html
Fixes: b0e21553ae ("blobmsg: add _len variants for all attribute checking methods")
Reported-by: Hannu Nyman <hannu.nyman@welho.com>
Tested-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Fixes following warning reported by GCC 10.0.0 20191203:
blobmsg.c:234:2: error: 'strcpy' offset 6 from the object at 'attr' is out of the bounds of referenced subobject 'name' with type 'uint8_t[0]' {aka 'unsigned char[0]'} at offset 6 [-Werror=array-bounds]
234 | strcpy((char *) hdr->name, (const char *)name);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from blobmsg.c:16:
blobmsg.h:42:10: note: subobject 'name' declared here
42 | uint8_t name[];
| ^~~~
Reported-by: Khem Raj <raj.khem@gmail.com>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Introduce _len variants of blobmsg attribute checking functions which
aims to provide safer implementation as those functions should limit all
memory accesses performed on the blob to the range [attr, attr + len]
(upper bound non inclusive) and thus should be suited for checking of
untrusted blob attributes.
While at it add some comments in order to make it clear.
Signed-off-by: Tobias Schramm <tobleminer@gmail.com>
[_safe -> _len, blobmsg_check_array_len fix, commit subject/desc facelift]
Signed-off-by: Petr Štetiar <ynezz@true.cz>
blobmsg_check_attr_len adds a length limit specifying the max offset
from attr that can be read safely.
Signed-off-by: Tobias Schramm <tobleminer@gmail.com>
[rebased and reworked, line wrapped commit message, _safe -> _len]
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Before there might have been as little as one single byte left which
would result in 3 bytes of blob_attr->id_len being out of bounds.
Acked-by: Yousong Zhou <yszhou4tech@gmail.com>
Signed-off-by: Tobias Schramm <tobleminer@gmail.com>
[line wrapped < 72 chars]
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Fixes following error found by the fuzzer:
==29774==ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 1 at 0x6020004f1c56 thread T0
#0 strcmp sanitizer_common_interceptors.inc:442:3
#1 blobmsg_parse blobmsg.c:168:8
Signed-off-by: Petr Štetiar <ynezz@true.cz>
==31775==ERROR: AddressSanitizer: SEGV on unknown address 0x604000a7c715
==31775==The signal is caused by a READ memory access.
#0 blobmsg_check_attr blobmsg.c:48:6
#1 blobmsg_parse_array blobmsg.c:118:8
#2 fuzz_blobmsg_parse test-blobmsg-parse-fuzzer.c:35:2
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Found by fuzzer:
ERROR: AddressSanitizer: SEGV on unknown address 0x602100000455
The signal is caused by a READ memory access.
#0 in blob_check_type blob.c:214:43
#1 in blob_parse_attr blob.c:234:9
#2 in blob_parse_untrusted blob.c:272:12
#3 in fuzz_blob_parse tests/fuzzer/test-blob-parse-fuzzer.c:34:2
#4 in LLVMFuzzerTestOneInput tests/fuzzer/test-blob-parse-fuzzer.c:39:2
Caused by following line:
if (type == BLOB_ATTR_STRING && data[len - 1] != 0)
where len was pointing outside of the data buffer.
Signed-off-by: Petr Štetiar <ynezz@true.cz>
blob_parse can be only used on trusted input as it has no possibility to
check the length of the provided input buffer, which might lead to
undefined behaviour and/or crashes when supplied with malformed,
corrupted or otherwise specially crafted input.
So this introduces blob_parse_untrusted variant which expects additional
input buffer length argument and thus should be able to process also
inputs from untrusted sources.
Signed-off-by: Petr Štetiar <ynezz@true.cz>
==5872==ERROR: AddressSanitizer: SEGV on unknown address 0x6020004100b4
==5872==The signal is caused by a READ memory access.
#0 blob_data blob.h
#1 blob_parse blob.c:228:2
Signed-off-by: Petr Štetiar <ynezz@true.cz>
LibFuzzer is in-process, coverage-guided, evolutionary fuzzing engine.
LibFuzzer is linked with the library under test, and feeds fuzzed inputs
to the library via a specific fuzzing entrypoint (aka "target
function"); the fuzzer then tracks which areas of the code are reached,
and generates mutations on the corpus of input data in order to maximize
the code coverage.
Lets use libFuzzer to fuzz blob and blobmsg parsing for the start.
Ref: https://llvm.org/docs/LibFuzzer.html
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Currently we run all tests via Valgrind. This patch adds 2nd batch of
tests which are compiled with Clang AddressSanitizer[1],
LeakSanitizer[2] and UndefinedBehaviorSanitizer[3] in order to catch
more issues during QA on CI.
AddressSanitizer is a fast memory error detector. The tool can detect
the following types of bugs:
* Out-of-bounds accesses to heap, stack and globals
* Use-after-free, use-after-return, use-after-scope
* Double-free, invalid free
LeakSanitizer is a run-time memory leak detector. It can be combined
with AddressSanitizer to get both memory error and leak detection, or
used in a stand-alone mode.
UndefinedBehaviorSanitizer (UBSan) is a fast undefined behavior
detector. UBSan modifies the program at compile-time to catch various
kinds of undefined behavior during program execution, for example:
* Using misaligned or null pointer
* Signed integer overflow
* Conversion to, from, or between floating-point types which would
overflow the destination
1. http://clang.llvm.org/docs/AddressSanitizer.html
2. http://http://clang.llvm.org/docs/LeakSanitizer.html
3. http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Fixes following compiler warnings:
blobmsg.c:242:39: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
blobmsg.c:248:23: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
ulog.c💯18: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
ulog.c:112:16: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
ulog.c:117:20: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
Signed-off-by: Petr Štetiar <ynezz@true.cz>
gcc version 4.8.4 (Ubuntu 14.04) and -Wextra produces following:
json_script.c:124:3: error: missing initializer for field 'name' of 'struct blobmsg_policy' [-Werror=missing-field-initializers]
Reported-by: Jonas Gorski <jonas.gorski@gmail.com>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Add missing usage hints for -p and -o arguments.
Fixes: e16fa068a5 ("jshn: add support for namespaces")
Fixes: eb30a03048 ("libubox, jshn: add option to write output to a file")
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Fixes following error:
Invalid read of size 1
at 0x4C32D04: strlen
by 0x5043367: json_tokener_parse_ex
by 0x5045316: json_tokener_parse_verbose
by 0x504537D: json_tokener_parse
by 0x401AB1: jshn_parse (jshn.c:179)
by 0x40190D: jshn_parse_file (jshn.c:370)
by 0x40190D: main (jshn.c:434)
Address 0x5848c4c is 0 bytes after a block of size 1,036 alloc'd
at 0x4C2FB0F: malloc
by 0x4018E2: jshn_parse_file (jshn.c:357)
by 0x4018E2: main (jshn.c:434)
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Fixes following leaks of memory:
352 (72 direct, 280 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 3
at 0x4C31B25: calloc
by 0x5042E1F: json_object_new_array
by 0x5044B02: json_tokener_parse_ex
by 0x5045316: json_tokener_parse_verbose
by 0x504537D: json_tokener_parse
by 0x401AA9: jshn_parse (jshn.c:179)
by 0x401977: main (jshn.c:378)
752 (72 direct, 680 indirect) bytes in 1 blocks are definitely lost in loss record 6 of 6
at 0x4C31B25: calloc
by 0x50424CF: json_object_new_object
by 0x5044B38: json_tokener_parse_ex
by 0x5045316: json_tokener_parse_verbose
by 0x504537D: json_tokener_parse
by 0x401AA9: jshn_parse (jshn.c:179)
by 0x401977: main (jshn.c:380)
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Fixes following leak of memory:
6,016 bytes in 1 blocks are possibly lost in loss record 1 of 1
at 0x4C31B25: calloc
by 0x1098F8: main (jshn.c:353)
Signed-off-by: Petr Štetiar <ynezz@true.cz>
Turn longer switch cases into separate functions in order to make it
easier to follow. Don't return from the cases as it makes future
cleaning up harder.
Signed-off-by: Petr Štetiar <ynezz@true.cz>
clang-10 analyzer reports following:
avl.c:671:25: warning: Access to field 'parent' results in a dereference of a null pointer (loaded from field 'right')
node->right->parent = parent;
~~~~~ ^
Which seems to be impossible to trigger via exported AVL public API, but
it could be probably trigerred by fiddling with the AVL tree node struct
members manually as they are exposed.
Signed-off-by: Petr Štetiar <ynezz@true.cz>
clang-10 analyzer reports following:
blobmsg_json.c:285:2: warning: The expression is an uninitialized value. The computed value will also be garbage
s->indent_level++;
^~~~~~~~~~~~~~~~~
Signed-off-by: Petr Štetiar <ynezz@true.cz>
clang-10 analyzer reports following:
base64.c:325:20: warning: Array access (from variable 'target') results in a null pointer dereference
target[tarindex] = 0;
~~~~~~ ^
and prepared test case confirms it:
Invalid write of size 1
at 0x4E4463F: b64_decode (base64.c:325)
by 0x40088C: test_invalid_inputs (tests/test-base64.c:26)
by 0x40088C: main (tests/test-base64.c:32)
Address 0x1 is not stack'd, malloc'd or (recently) free'd
Process terminating with default action of signal 11 (SIGSEGV)
Access not within mapped region at address 0x1
at 0x4E4463F: b64_decode (base64.c:325)
by 0x40088C: test_invalid_inputs (tests/test-base64.c:26)
by 0x40088C: main (tests/test-base64.c:32)
Signed-off-by: Petr Štetiar <ynezz@true.cz>
In order to allow seamless assert() usage in release builds without the
need for fiddling with CMake C flags as CMake adds -DNDEBUG switch in
release builds which disable assert().
Signed-off-by: Petr Štetiar <ynezz@true.cz>
For improved QA etc. For the start with initial test cases for avl,
base64, jshn and list components. Moved runqueue and blobmsg from
examples to tests. Converted just a few first test cases from
json-script example into the new cram based unit test, more to come.
Signed-off-by: Petr Štetiar <ynezz@true.cz>
