The yes_no() function was leaking memory when returning
early from the loop, as the 'answer' buffer allocated by
getline() was not freed before the return statements.
Restructure the function to use a single exit point, ensuring
free(answer) is always called before returning.
Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Volkan Unal <vunal@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
sort_bls_entries() already checks for the keyword "title"
duplication. Add also checks for keywords "linux", "initrd", and
"options" duplication (Rule #5).
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Eduard Shishkin <edward6@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Check decimal prefixes, representing site values in on-disk
environment block. Reject any values different from {0, ..., 9}
as invalid ones.
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Eduard Shishkin <edward6@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Before extracting a file trailer, check that the file is big enough;
Check that signature length stored in the extracted trailer doesn't
exceed the size of the file without the trailer. Treat the case of
the failed checks as unsigned file.
Without the checks, memcmp() may read the area before the allocated
@buffer, dumping heap into bootmap or crashing.
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Eduard Shishkin <edward6@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
When parsing environment block, check each its line for the
compliance with the 'foo=bar\n' pattern. In case of missing '=',
or '\n' abort the parsing procedure with a warning message.
Without this check, stage3 may write a zero byte to the absolute
address 0 (robustness defect)
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Eduard Shishkin <edward6@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
When parsing environment block, limit the number of the hash table
entries, so that total size of all records doesn't exceed PAGE_SIZE
bytes allocated for that hash table.
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Eduard Shishkin <edward6@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
By design, before replacement of '${FOO}' with its value, the
procedure checks that the resulted command line doesn't exceed the
maximum one.
The old check used the never updated length of the original command
line, which is incorrect. Instead, use its current length resulting
from the replacement happened at the previous iteration.
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Eduard Shishkin <edward6@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
mem_init_flex() allocates the page header index array sized for
ADDR_TO_IDX(mem_end) + 1 entries, but never checked that each
page record's address falls within [0, mem_end).
A crafted LKCD page record with addr >= mem_end writes 8 bytes
beyond the allocation.
Fix by rejecting any page record whose address is > mem_end - PAGE_SIZE
with ERR_EXIT(), immediately after the end-of-dump marker check.
Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
Reviewed-by: Alexander Egorenkov <egorenar@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
read_page_buf() uses the file-controlled pg_hdr->size directly as the
byte count for zg_read() into fixed stack buffers of PAGE_SIZE.
A crafted LKCD dump with pg_hdr->size > PAGE_SIZE overflows the buffer
and smashes the stack frame. A size of 0 for a raw page silently
produces uninitialised data.
Enforce page header size constraints following crash-utility's logic:
- Compressed or Raw pages with size > PAGE_SIZE are invalid;
exit with an error.
- Raw pages with size 0 contain no data in the file; fill the
output buffer with zeros without reading.
- Compressed pages with size 0 are caught by the uncompress() return
code check.
- Raw pages with size != PAGE_SIZE are invalid; exit with an error.
Check the return code of uncompress() and exit with an error if
decompression fails, rather than silently proceeding with an incomplete
output buffer.
Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
Reviewed-by: Alexander Egorenkov <egorenar@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Declare binary data symbols as arrays to correctly represent
linker-provided symbols of unknown size. This resolves compiler warnings
about reading 22-24 bytes from a 1-byte region when including stage*
loaders.
DEBUG: In function ‘memcpy’,
DEBUG: inlined from ‘boot_init_eckd_ldl_stage0’ at boot.c:181:2,
DEBUG: inlined from ‘install_svdump_eckd_ldl’ at install.c:891:2,
DEBUG: inlined from ‘install_dump’ at install.c:1221:9,
DEBUG: inlined from ‘main’ at zipl.c:185:9:
DEBUG: /usr/include/bits/string_fortified.h:29:10: warning: ‘__builtin_memcpy’ reading 22 bytes from a region of size 1 [-Wstringop-overread]
DEBUG: 29 | return __builtin___memcpy_chk (__dest, __src, __len,
DEBUG: | ^
Signed-off-by: Marc Hartmayer <marc@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Ensure that the path to a dump image specified in the NGDump meta file
of a dump device points to a valid location within the dump device.
Especially, disallow escaping from a dump device with a dump image path
using references to '..' or symbolic links pointing outside of the dump
device.
Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Replace the popen() call to the external tool sha256sum with OpenSSL's
SHA256 digest algorithm to compute the checksum of a dump image
to improve security of NGDump.
The call to popen() is inherently insecure because a malicious user
controlling a dump device could manipulate the latter and let the user
applying zgetdump to the dump device execute arbitrary shell code.
Furthermore, getting rid of process and shell spawning performed by
popen() shall improve performance of calculation of dump image
checksums.
Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
Reviewed-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Use the file descriptor opened in zg_open() before to obtain information
about the file. Theoretically, there is a tiny window for a race
condition between opening a file and obtaining its information with
stat(). A malicious user could use it to entice incorrect behavior
from zgetdump. Using the same file descriptor for both operations
prevents such situations.
Suggested-by: Peter Jin <pjin@linux.ibm.com>
Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
Reviewed-by: Marc Hartmayer <marc@linux.ibm.com>
Reviewed-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
The name of a private NGDump function does not require to have a unique
prefix to avoid naming conflicts with public functions of other C modules.
Furthermore, be consistent with private NGDump functions not having
the prefix 'ngdump_'.
Signed-off-by: Alexander Egorenkov <egorenar@linux.ibm.com>
Reviewed-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
dfo_elf_init() allocates the output ELF header buffer based on
HDR_BASE_SIZE, per-CPU note sizes, and per-memory-chunk overhead,
but does not account for the vmcoreinfo note. notes_init() then
writes the full vmcoreinfo string into the buffer via nt_vmcoreinfo(),
overflowing the heap allocation for any dump with a vmcoreinfo blob
larger than what fits in HDR_BASE_SIZE. The following ABORT check
fires after the heap was already corrupted.
Fix by extracting the allocation size calculation into a new
hdr_alloc_size() function that explicitly accounts for all items
written into the header buffer.
Remove the opaque HDR_BASE_SIZE and HDR_PER_MEMC_SIZE macros.
Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
Reviewed-by: Alexander Egorenkov <egorenar@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
vmcoreinfo_item() copies a key's value string from the vmcoreinfo heap
blob into the caller's buffer using memcpy() without checking the source
length. The length is derived from the distance between the '=' separator
and the next '\n' (or '\0') in the blob, which is bounded only by the
total vmcoreinfo size. A crafted dump with a vmcoreinfo value >= 1024
bytes would overflow the buffer.
Additionally, the len parameter of vmcoreinfo_item() was declared UNUSED
and never checked.
Fix by computing val_len before the copy and returning -1 if val_len >= len.
Change len type from int to size_t, which is natural for a buffer size,
drops the need for a negativity guard, and makes the call site passing
sizeof(str) type-consistent.
Write directly into the caller's buf, instead of going through the
intermediate str[].
Replace two strchr() calls with a single strchrnul().
Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
Reviewed-by: Alexander Egorenkov <egorenar@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
The 16-bit sec5_len field from the ADSR record was used directly as
the byte count for zg_read() and ebc_2_asc() into a pair of 1024-byte
stack buffers. A crafted VMDUMP file with sec5_len > 1024 would
overflow both buffers and write past the stack frame.
The overflow is only reachable when zgetdump is invoked with the -V
(verbose) flag, but that is a common diagnostic usage.
Section 5 contains a human-readable dump symptom string used only for
display purposes. Truncate sec5_len to sizeof(buf) - 1 so that
oversized values are silently clamped rather than causing an abort,
and the symptom string is still printed up to the buffer limit.
Signed-off-by: Mikhail Zaslonko <zaslonko@linux.ibm.com>
Reviewed-by: Alexander Egorenkov <egorenar@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Building with ASAN=1 produces the following warning:
utils.c: In function 'sysfs_get_mkvps':
utils.c:416:9: warning: writing 8 bytes into a region of size 0
[-Wstringop-overflow=]
416 | memcpy(mk_reg->mkvp, &mkvp, sizeof(mkvp));
| ^
In file included from utils.c:32:
utils.h:51:17: note: at offset 4 into destination object 'mk_state'
of size 4
51 | int mk_state;
This is a false positive, field mk_reg->mkvp is defined as an array of
u8 with a length of 16 elements, thus it is 16 bytes in size, and thus
more than long enough to copy 8 bytes into it.
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
When the BER encoded BITRSTRING field has a zero length, then skipping
the unused-bits byte that a BITRSTING normally contains will cause a
underflow and thus may cause an out of bounds read by the caller of
sk_ep11_parse_spki().
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Check for the size of struct cca_rsa_pub_key_section, instead of
struct cca_ec_pub_key_section.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
When EVP_PKEY_get0_EC_KEY(), EC_KEY_set_ex_data(), EVP_PKEY_get0_RSA()
or RSA_set_ex_data() fails, the function returns -EIO without freeing
the data struct or the data->key_blob that were allocated before.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Field spki_size is defined as size_t, so it can never be negative.
Before the check, sk_ep11_valid_ep11_blob() ensures that hdr->len is
never greater than key_token_length, so the subtraction can never
overflow the result, but it still can be zero.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
For long-length field encoding, check if the number of length bytes
exceed the data size.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
A malformed CCA key token with a zero size section can cause an endless
loop in sk_cca_get_pka_section().
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Array element key_value_structure.public_exponent[0] is written twice,
but key_value_structure.public_exponent[1] is not set, leading to a
wrong public exponent value (i.e. 0x01) in the key value structure.
CCA key generation will fail is this case.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Functions SK_CCA_generate_ec_key_pair(), SK_CCA_generate_ec_key_pair(),
SK_EP11_generate_ec_key_pair() and SK_EP11_generate_rsa_key_pair() are
supposed to allow parameter key_token to be NULL. In this case the
required buffer size is returned in key_token_length only.
This size query does not work because the parameter checking rejects
the call with a NULL key_token returning -EINVAL.
Currently the size query is not used by any known caller, but lets
correct it anyway.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
The party info contained in the response might be of an arbitrary size.
Check for an overflow before allocating the party info buffer for the
combined request and response party info data. With a very large
response party info size, the combined size might overflow (i.e. wrap
around), causing a too small buffer being allocated and the following
memcpy's would cause an out of bounds write.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
On large data sizes the calculation may cause an overflow, if done with
int type. Do the calculation with size_t instead to avoid an overflow.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Fix the error checking to check the correct variables using the right
conditions.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Don't attempt to free the local function parameter 'signature', but the
memory where *signature points to.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Function strtoull() silently accepts a leading '-' and returns a large
positive number (two's complement wrap). Reject this by checking for
the minus character.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Function gmtime() might return NULL which would then be passed to
strftime(). Return an error in case gmtime() return NULL.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
A deeply nested or pathologically large KMIP node tree crafted by a
malicious server can wrap the length calculation around to a small
value. Detect this and return an error in this case.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Functions kmip_node_get_structure_element_count() and
kmip_node_get_structure_element_by_tag_count() return an unsigned int,
but the error case returns -1. This leads to a wrap around actually
returning 0xffffffff which the caller might interpret as a very large
number of elements.
Return 0 in case of an error instead.
Also fix some callers to not unconditionally subtract -1 from the
returned value, but return an error if the returned value is zero.
These callers check the preconditions upfront, so the error case
won't be hit anyway.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Limit the value_len to not be larger than INT_MAX, because later on
BIO_read() is called with value_len and it uses the int type for
length parameter and return value.
This check also prevents the 'value_len + 1' from overflow, because
value_len is a size_t and this accepts larger values than int (even
on 32 bit architectures).
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
When decoding a value that is exactly at the boundary of the available size
(i.e., *size == value_len), and the value length is not a multiple of the
TTLV block length, then *size wraps to a huge value when the pad_len is
subtracted.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
If length is >= 0x80000000 (2 GB), length * 2 wraps around to a small
value, calloc allocates a too small buffer, then the loop writes
length * 2 bytes into it causing a heap buffer overflow.
Fix this by using a size_t for size calculation, and also checking
the length before multiplication (needed on 32 bit platforms).
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
In C, operands of '<<' are subject to integer promotion. So the unsigned
char array elements are promoted to signed int and then shifted producing
signed overflow — undefined behaviour in C.
Fix this by first casting to uint32_t and then shift.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
KMIP STRUCTURE elements can be nested, which causes a recursion of
functions kmip_decode_ttlv(), kmip_decode_xml(), and kmip_decode_json().
A malformed KMIP response may thus cause stack exhaustion.
Limit the KMIP STRUCTURE nesting level to 32 levels. This is more than
enough for currently defined KMIP responses. The practically used
nesting level is 8 or 9, dependent on the type of KMIP response.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
When creating or writing files, make sure that the file is not a
sysmlink. Such files created by libkmipclient are typically stored inside
the zkey repository and the owner and mode of them are changed to.
allow read/write for the owner user and the 'zkeyadm' group. It
would allow a symlink-following attack if the file being created are
symlinks. Make sure to open such files with the 'O_NOFOLLOW' flag.
Assisted-by: IBM Bob:2.0.0
Signed-off-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Finn Callies <fcallies@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
