Fix the checks against reading beyond the image length when executing
an ELF image.
As with the equivalent commit 979c86f ("[nbi] Avoid harmless integer
overflows in image length checks"), this change has absolutely no
security impact: an ELF image will obtain control of the system in
ring 0 anyway, and so a "malicious" ELF image with malformed length
fields cannot do anything that it would not already be able to do
simply by being executed. However, fixing these harmless integer
overflows costs very little and reduces unwanted noise from security
reviewers.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The descriptor count for a zero-length stream transfer with no
explicit terminating zero-length packet is currently calculated
incorrectly as requiring zero descriptors. This will cause
uhci_enqueue() to attempt to allocate a zero-length block of transfer
descriptors, which will fail and return -ENOMEM.
There is no internal code path within iPXE that can ever submit a
zero-length stream transfer without an explicit terminating
zero-length packet. This condition is reachable only via the
EFI_USB_IO_PROTOCOL interface that we expose on UEFI platforms to
allow existing firmware drivers to reconnect after we take control of
the host controller.
Fix by ensuring that the descriptor count is set to one for a
zero-length stream transfer with no explicit terminating zero-length
packet, as is already done for EHCI and XHCI.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Left shifts into the sign bit are often reported as potential
undefined behaviour by automated tools, which distracts from real
issues.
Now that all offending constant left shifts have been eliminated from
the codebase, enable -Wshift-overflow=2 to ensure that such shifts
cannot be reintroduced in future.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Fix the technically undefined constant left shifts into the sign bit
in ancient, messy, and third-party code.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Fix the technically undefined constant left shifts into the sign bit
in code where there is some value in attempting to minimise the
aesthetic disruption from doing so.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Clean up the IPV4() macro used to construct literal IPv4 addresses in
test cases, and make it generally available to all code.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The log message length is calculated incorrectly, causing the first
byte after the I/O buffer data to be both read and written (with a
fixed zero value). A log message of precisely 4079 bytes will
therefore result in a zero byte being written outside the I/O buffer's
heap allocation.
Fix by using the correct length for the log message.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The unsigned 8-bit value from the keyUsage bit string is promoted to a
(signed) int before being shifted left by up to 24 bits, which is
technically undefined behaviour.
Explicitly cast the 8-bit value to an unsigned int before shifting, to
inhibit this class of false positive warning. There is no difference
to the resulting object code.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
iPXE is single-threaded by design, but automated tools still tend to
erroneously report the mutation of static state as being unsafe,
especially when that mutation happens within cryptographic code.
At the cost of six bytes in the 32-bit BIOS binary, allocate the
reference algorithm ASN.1 cursor on the stack to eliminate this class
of false positive warning.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The last byte within a non-empty ASN.1 bit string object always
exists, but automated tools tend to erroneously report the way in
which we access it as being out of bounds.
Move the assignment of the last byte pointer to be ahead of the
shrinking of the cursor, to eliminate this class of false positive
warning.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Automated reporting tools tend to pick up the right-shift by an
attacker-controllable shift amount as a potential defect, since a
right-shift by greater than the word size is technically undefined
behaviour.
The result of an undefined shift is already ignored by the following
range check on the shift amount, and the separate "unused_mask"
variable exists only to make the code clearer to read. Sacrifice this
very small improvement in legibility for the sake of reducing future
reporting noise.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The NFS protocol code was marked as forbidden for UEFI Secure Boot in
commit 3094898 ("[build] Mark existing files as explicitly forbidden
for Secure Boot"), but the file net/tcp/oncrpc.c was missed due to
being outside of the net/oncrpc directory.
Add the missing explicit FILE_SECBOOT() declaration.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The SCSI RDMA protocol (as implemented in iPXE) allows a remote entity
full write access to host memory, and so would provide an immediate
Secure Boot exploit.
The SCSI RDMA protocol is already implicitly forbidden for UEFI Secure
Boot (by not having any FILE_SECBOOT marker). Make this explicit.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Fix the checks against reading beyond the image length when executing
an NBI image.
This change has absolutely no security impact: an NBI image will
obtain control of the system in ring 0 anyway, and so a "malicious"
NBI image with malformed length fields cannot do anything that it
would not already be able to do simply by being executed. However,
fixing these harmless integer overflows costs very little and reduces
unwanted noise from security reviewers.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The peer cache entries are subject to the cache discarder, and could
therefore potentially be freed during calls to ib_resolve_path(),
eoib_duplicate(), or ib_post_send().
Create an on-stack copy of the destination address vector, instead of
passing around a pointer to the address vector within the peer cache
entry.
Since the LID within the peer cache entry will no longer be updated by
ib_resolve_path(), change the receive-side logic to update the peer
cache unconditionally.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The construction of the OCSP URI erroneously attempts to URI-encode
the terminating NUL of the Base64-encoded string, but does so using a
bounded write into a buffer that was sized precisely (i.e. without
space for the spurious encoded NUL), and so ends up constructing the
correct string anyway.
Reduce confusion by passing the same input value to both calls to
uri_encode(), and add assertions on the return values from both
base64_encode() and uri_encode().
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The existing overflow check for the allocated memory block size has a
logic gap: a size that is close to the maximum value with a suitable
offset can end up being rounded to heap->align rather than to zero.
This overflow is not reachable via malloc(). With the internal heap,
we have:
align = heap->ptr_align = sizeof ( void * )
offset = -offsetof ( struct autosized_block, data )
= -sizeof ( size_t )
= -sizeof ( void * )
= -align
and therefore
offset & ( align - 1 ) == 0
and so any integer overflow in actual_size will produce a zero result
and will be caught by the existing check.
The overflow is also not reachable via malloc_phys(), since these
allocations are made for DMA and I/O buffers, where the size cannot be
arbitrarily controlled by an attacker.
The overflow is reachable via umalloc() on the BIOS and RISC-V SBI
platforms where umalloc() is backed by the external user heap. The
overflow is not reachable via umalloc() on UEFI platforms where
umalloc() is instead backed by AllocatePages(), or on Linux platforms
where umalloc() is backed by mmap().
Fix by checking for overflow in the standard way, rather than relying
erroneously upon the assumption that overflow will always produce a
zero result in actual_size.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
There is no way for heap_alloc_block() to be called with a size of
zero or with an alignment that is not a power of two, and so asserting
these conditions is justifiable.
However, given the criticality of memory allocation to security, it is
worth converting these to runtime checks to guard against future code
changes that could, for example, allow for a variable alignment to be
passed in without being rounded up.
Convert the zero-size assertion and the power-of-two-alignment
assertion into runtime checks, and document the reasoning.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
A requested alignment of zero is logically unsatisfiable: the
resulting pointer can never be a multiple of zero. No existing caller
ever attempts to allocate memory with an alignment of zero.
Correct the relevant assertions, and drop the misleading handling of
zero as a special-cased value when masking the alignment offset.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The iob_unput() to strip any trailing padding is currently sign
reversed, causing the buffer to be extended rather than truncated.
This can result in uninitialised data within the receive I/O buffer
being passed to the LACP or marker receive handlers and subsequently
echoed back to the sender.
Fix by reversing the subtraction.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The length checks in rsa_pkcs1_encode() and rsa_pkcs1_encrypt()
subtract the 11-byte fixed encoding length from the modulus size,
which can underflow in the case of a malicious RSA key with an
absurdly small modulus.
Signature verification for validating X.509 certificates is already
gated behind the validation status of the issuer certificate. It is
therefore impossible to exploit this via X.509 without explicitly
trusting a malicious certificate (e.g. via the TRUST=... build-time
parameter).
However, commit 05e6256 ("[tls] Parse ServerKeyExchange record
immediately") changed the timing of the TLS protocol parsing such that
the verification of the ServerKeyExchange message is now performed
immediately upon receipt, rather than deferring this check until the
certificate has been validated. It is therefore possible to use a
malicious TLS server certificate to trigger this underflow before the
certificate is validated. This commit is less than two weeks old and
has never been included in a Secure Boot signed build.
Fix by performing the length checks using addition rather than
subtraction.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The DHCP options parsing code is approximately twenty years old and
dates back to a time when code size considerations were dominant. The
dhcp_option_len() function may currently read up to one byte beyond
the end of the options data. There is no impact from this (since the
immediately following range check will cause the loop to terminate),
but it is technically an out-of-bounds read.
Fix by passing the remaining length to dhcp_option_len() and treating
a malformed tag at the end of the options data as having a length of
one byte.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Writing to the raw (i.e. decompressed) data buffer is already strictly
bounded by its allocated length. However, reading from the raw data
buffer to construct the pixel buffer content is not. A maliciously
formed PNG file can therefore result in undefined external heap memory
being read, interpreted, and used to construct the picture shown on
screen to the user.
There is no way for this data to subsequently be obtained over the
network, though a particularly determined attacker could potentially
reconstruct the contents of other image files by capturing the
on-screen video output.
Fix by checking for overflow at each stage of constructing the raw
buffer length.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The iob_unput() to strip any trailing padding is currently sign
reversed, causing the buffer to be extended rather than truncated.
This can result in uninitialised data within the receive I/O buffer
being passed to the EAP request handler. This uninitialised data
would then erroneously be hashed as part of the MD5 or MSCHAPv2
challenge.
Fix by reversing the subtraction, and adjust the variable names so
that the correct order is more immediately obvious.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The header length field exists for all packet types. Validate this
length wihtin eap_rx() for all packet types, rather than performing
validation only for EAP requests in eap_rx_request().
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The optimisation to check for a trailing CRLF in http_rx_chunk_data()
could potentially underflow and look for the CR and LF bytes in the
I/O buffer data that immediately precedes the HTTP content (i.e. in
the TCP header).
Fix by avoiding the potential underflow. Update the code to use a
dedicated CRLF structure, to reduce the proliferation of magic numbers
within the function.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Reject underlength DHCP packets before calling dhcppkt_init(), which
takes a struct dhcphdr pointer and so may legitimately assume that the
structure is complete (i.e. that the length is at least large enough
to contain a struct dhcphdr).
Do not modify the dhcppkt_init() parameters to pass the options length
rather than the total length. This alternative approach would make it
impossible to pass an invalid length: the check in dhcp_deliver()
would then become a check for integer underflow, which would be more
obviously necessary. However, all callers of dhcppkt_init() have the
total length more readily available than the options length, and
callers such as cachedhcp_record() deal with fixed-size structures
such as EFI_PXE_BASE_CODE_PACKET and so do not have to worry about
potential underlength packets.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The IPv6 header length field contains the payload length (excluding
the length of the IPv6 header itself). The IPv6 packet parser
calculates the length of the received packet correctly, but wrongly
uses the payload length (rather than the full packet length) when
checking for truncated packets.
Fix by calculating the packet length exactly once and using it for
both purposes.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
POSIX specifies that the values of members of the broken-down time
structure are "not restricted to the ranges", and defines the way in
which out-of-range values are to be handled.
For most fields, the arithmetic is already purely linear and so
out-of-range values are handled automatically. Out-of-range months
are an exception: these are used as array indices and so must be
normalised before use.
Restructure mktime() to make it more immediately visible when values
are being read from and written back to the broken-down time
structure, add the required normalisation for the month number, and
add test cases to cover out-of-range months.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
For memory that may contain secrets, it is good practice to zero the
memory before returning it to the heap.
Add a zfree() function that can be used to zero and then free any
memory allocated using malloc(), and use it in place of free() for any
existing code that is obviously managing secrets held in dynamically
allocated memory.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Michael Brown