If errors occur in printbuf_memappend, then these errors should be
propagated through sprintbuf to indicate the error to the user.
Proof of Concept:
```
#include <err.h>
#include <limits.h>
#include <stdio.h>
#include "json.h"
int
main(void) {
struct printbuf *pb;
if ((pb = printbuf_new()) == NULL)
err(1, "printbuf_new");
if (printbuf_memset(pb, INT_MAX - 9, 'a', 1) < 0)
errx(1, "printbuf_memset");
printf("length: %d\n", printbuf_length(pb));
printf("sprintbuf: %d\n", sprintbuf(pb, "string too long"));
printf("length: %d\n", printbuf_length(pb));
printbuf_free(pb);
return 0;
}
```
You can see that sprintbuf does not return an error but length is still
the same, i.e. the string "string too long" has not been appended.
I would like to add this as a unit test but it really depends on the
operating system if printbuf_memset() would fail if not enough memory is
available or not.
It is possible to have a printbuf with "gaps", i.e. areas within the
print buffer which have not been initialized by using printbuf_memset.
Always clear memory in such cases.
Example:
```
struct printbuf *pb = printbuf_new();
printbuf_memset(pb, 10, 'a', 2);
```
In this case pb->buf[0] is '\0' but pb->buf[1] up to pb->buf[9] are
not set. The length would be 12 due to successful printbuf_memset.
Systems without vasprintf fall back to implementation in header file
vasprintf_compat.h. This version could run into heap overflow issues
with very long arguments or formats provoking a lot of output.
The vsnprintf function returns a negative value if more than INT_MAX
characters would be written since its int return type could not
handle this (and %n couldn't handle it either).
Before testing for a possible error value the additional char for
\0 is already added. A -1 error code would not be detected.
Increment only after implicitly casting to an unsigned value to avoid
signed integer overflow if INT_MAX has been returned.
Use va_copy to duplicate the original ap argument for multiple uses
on non-WIN32 systems. At least with glibc the test suite would fail
because the arguments are not reset after leaving the vsnprintf call.
Removed support for apparently very old glibc versions which do not
comply with vsnprintf standard descriptions. It breaks support for
modern ones which are not forced to return -1 in case of error. The
standard specifies merely "a negative value".
How to reproduce:
- Use a system without vasprintf
- Alternatively remove -D_GNU_SOURCE from CMakeLists.txt
- Compile and run:
#include "json.h"
int main(void) {
struct printbuf *pb = printbuf_new();
sprintbuf(pb, "prefix %2147483647s", "*");
printbuf_free(pb);
return 0;
}
SIZEOF_SIZE_T might be only defined in config.h.
Include config.h for these systems to pass tests which are only
supposed to be run on 32 bit systems.
Fixes issue #666.
The array_list_new2 function, which is externally reachable through
json_object_new_array_ext, does not check if specified initial size
actually fits into memory on 32 bit architectures.
It also allows negative values, which could lead to an overflow on these
architectures as well. I have added test cases for these situations.
While at it, also protect array_list_shrink against too large
empty_slots argument. No test added because it takes a huge length
value, therefore a lot of items within the array, to overflow the
calculation. In theory this affects 64 bit sytems as well, but since the
arraylist API is not supposed to be used by external applications
according to its header file, the call is protected due to int
limitation of json_object_array_shrink.
Casting time(2) return value to int and multiplying the result with
such a constant will definitely lead to a signed overflow by this day.
Since signed overflows are undefined behaviour in C, avoid this.
Casting to unsigned is more than enough since the upper bits of a
64 bit time_t value will be removed with the int conversion anyway.
The allocation of printbuf_new might fail. Return NULL to indicate tis
error to the caller. Otherwise later usage of the returned tokener would
lead to null pointer dereference.
Several issues occur if a string is longer than INT_MAX:
- The function json_object_get_string_len returns the length of a string
as int. If the string is longer than INT_MAX, the result would be
negative.
- That in turn would lead to possible out of boundary access when
comparing these strings with memcmp and the returned length as done in
json_object_equal.
- If json_escape_str is called with such strings, out of boundary
accesses can occur due to internal int handling (also fixed).
- The string cannot be printed out due to printbuffer limits at
INT_MAX (which is still true after this commit).
Such huge strings can only be inserted through API calls at this point
because input files are capped at INT_MAX anyway.
Due to huge amount of RAM needed to reproduce these issues I have not
added test cases.
The function _json_c_strerror does not properly format unknown errnos.
The int to ascii loop ignores the leading digit if the number can be
divided by 10 and if an errno has been formatted, shorter errnos would
not properly terminate the newly created string, showing the ending
numbers of the previous output.
A test case has been added to show these effects.
Since this function has been introduced for tests, the effect of this on
real life code is basically non-existing. First an environment variable
has to be set to activate this strerror code and second an unknown errno
would have to be encountered.
The json-c library is used in cryptsetup for LUKS2 header information.
Since cryptsetup can be called very early during boot, the developers
avoid getrandom() calls in their own code base for now. [1]
Introducing a blocking getrandom() call in json-c therefore introduces
this issue for cryptsetup as well. Even though cryptsetup issues do not
have to be json-c issues, here is my proposal:
Let's use a non-blocking call, falling back to other sources if the call
would block. Since getrandom() accesses urandom, it must mean that we
are in an early boot phase -- otherwise the call would not block
according to its manual page.
As stated in manual page of random(4), accessing /dev/urandom won't
block but return weak random numbers, therefore this fallback would work
for json-c.
While at it, fixed the debug message.
[1] https://gitlab.com/cryptsetup/cryptsetup/-/merge_requests/47
which references to https://lwn.net/Articles/800509/
A custom double formatter can lead to truncation of the rest of the
JSON document.
If a custom formatter completely fills the buffer used by snprintf
with a trailing dot or comma and the formatting option
JSON_C_TO_STRING_NOZERO has been specified, then an iterator moves
past the ending '\0' (off-by-one buffer overflow) to set an
additional '\0' and adds the first '\0' into the printbuf.
Since '\0' will eventually be considered the terminating character
of the complete printbuf result, all trailing characters are lost.
This leads to an incomplete JSON string as can be seen with the
test case.
The off-by-one can be noticed if compiled with address sanitizer.
Since this is a very special case and a malformed formatter could
do way more harm and is the responsibility of the user of this
library, this is just a protective measure to keep json-c code as
robust as possible.
The data structures linkhash and printbuf are limited to 2 GB in size
due to a signed integer being used to track their current size.
If too much data is added, then size variable can overflow, which is
an undefined behaviour in C programming language.
Assuming that a signed int overflow just leads to a negative value,
like it happens on many sytems (Linux i686/amd64 with gcc), then
printbuf is vulnerable to an out of boundary write on 64 bit systems.
If a linkhash with a size of zero is created, then modulo operations
are prone to division by zero operations.
Purely protective measure against bad usage.
If the assignment of stop overflows due to idx and count being
larger than SIZE_T_MAX in sum, out of boundary access could happen.
It takes invalid usage of this function for this to happen, but
I decided to add this check so array_list_del_idx is as safe against
bad usage as the other arraylist functions.
Tobias Stoeckmann