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>
The Content-Transfer-Encoding header is optional: if not present then
the default "7bit" encoding should be assumed. iPXE already includes
logic to set a default encoding name, but the default encoding name
then fails to match against any entries in the known encodings list
since it is terminated with a NUL (rather than with the semicolon or
whitespace character that would terminate the encoding name found
within a Content-Transfer-Encoding header).
Fix by removing the default encoding name and instead treating a NULL
encoding name as indicating that the default encoding should be used,
and add a test case that omits the Content-Transfer-Encoding header.
Reported-by: Huzaifa Ali Zar <zar@amazon.com>
Signed-off-by: Michael Brown <mcb30@ipxe.org>
When specifying the content of a test image (e.g. a MIME archive file)
using C string literals, there is no easy way to indicate that the
terminating NUL should be excluded from the byte array.
Provide BINFILE(), TEXTFILE(), and FILE_ARRAY() helper macros that can
be used to simplify the initialisation of a byte array passed as
either a raw byte value list or a string literal. For example:
#define TEST_CASE( name, file ) do { \
static uint8_t name ## _bytes FILE_ARRAY ( file ); \
... \
} while ( 0 )
TEST_CASE ( test1, BINFILE ( 0x68, 0x65, 0x6c, 0x6c, 0x6f ) );
TEST_CASE ( test2, TEXTFILE ( "hello" ) );
Both of the above TEST_CASE() lines will end up producing a five-byte
array:
static uint8_t test1_bytes[] = { 0x68, 0x65, 0x6c, 0x6c, 0x6f };
static uint8_t test2_bytes[5] = "hello";
This allows us to remove the stray NUL that otherwise appears at the
end of any test images that are specified using string literals.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Commit 3662065 ("[dns] Use all configured DNS servers") changed the
logic from opening a single defined nameserver address to opening an
unspecified peer socket address and then specifying the full peer
address for each transmitted packet.
The peer socket address was left unspecified by passing a null pointer
to xfer_open_socket(). This is supported by the UDP socket opener,
but technically violates the internal API (which allows the local
socket address to be a null pointer, but not the peer socket address).
In particular, in a debug build using DEBUG=open, the debug code will
itself dereference the peer address pointer.
Fix by embedding the name server socket address within the DNS request
structure, and passing this to xfer_open_socket().
Signed-off-by: Michael Brown <mcb30@ipxe.org>
With no current working URI, even a fully resolved URI may not have a
scheme. Attempting to open such a URI will currently result in
xfer_uri_opener() calling strcasecmp() with a null pointer. On a
system that guards against null pointer dereferences, this will result
in a segfault (or the equivalent, such as a Synchronous Exception on
arm64 UEFI).
Fix by checking that the URI is absolute (i.e. has a scheme) before
calling xfer_uri_opener(), as is already done elsewhere.
Reported-by: Matt Fleming <matt@readmodwrite.com>
Signed-off-by: Michael Brown <mcb30@ipxe.org>
As of commit 433a8f5 ("[tls] Retain a reference in the key schedule to
the bound identity"), the act of binding the server identity is
logically separated from the act of validating the server identity.
We may therefore bind the server identity (by verifying the signature
over the Diffie-Hellman parameters) and agree the ephemeral shared
secret immediately upon receiving the ServerKeyExchange record, rather
than deferring the verification until we have a validated identity.
This provides a closer match to the flow required for TLS version 1.3,
where the ephemeral shared secret is used for all messages after
ServerHello, and so must always be agreed prior to validation.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Add a "--quiet" option to each image-acquiring command that currently
accepts a "--timeout" option, to allow the displaying of the download
URI and the progress dots to be inhibited.
This is particularly useful with "data:" URIs to inhibit the echoing
of the full data URI contents:
iPXE> imgfetch -n hw data:,hello%20world
data:,hello%20world... ok
iPXE>
vs.
iPXE> imgfetch -q -n hw data:,hello%20world
iPXE>
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Add a trivial ring buffer console that can be used to extract the most
recent 8kB of (non-UI) console output as the ${dmesg} setting.
This allows previous console output to be displayed after the screen
has been cleared, such as when a background picture has been loaded.
For example:
#!ipxe
console -p http://boot.ipxe.org/ipxe.png
show -q dmesg
It also allows console output to be captured and sent as part of an
HTTP POST, to allow for remote diagnostics. For example:
#!ipxe
params
param dmesg ${dmesg:base64}
imgfetch http://192.168.0.1/api/diags##params
The recorded console output may be cleared if necessary by clearing
the setting:
clear builtin/dmesg
The name ${dmesg} is chosen as being unlikely to collide with any
existing variables used in end-user scripts. A separate "dmesg"
command is not provided, but could easily be added if useful.
Note that iPXE supports recursive variable expansion in shell
commands. Typing an interactive command such as "echo ${dmesg}" or
"param dmesg ${dmesg}" is therefore a great way to exercise the memory
allocator to the point of exhaustion. Use "show -q dmesg" to show the
ring buffer contents.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Within application/x-www-form-urlencoded values, a "+" character needs
to be escaped to avoid its being interpreted as a space.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Allow the "show -q" command to be used to display a setting's value
without also showing its origin and type metadata.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Add support for "data:" URIs as defined in RFC 2397. These can be
used to construct image content under control of an iPXE script. For
example:
# Inject the message "Hello from iPXE" as /etc/motd
initrd -n motd data:,Hello%20from%20iPXE%0A /etc/motd
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Within the iPXE data transfer interface model, openers are fully
asynchronous and may not deliver any data until after the opener has
returned.
Provide a trivial openable data blob object (as a generalisation of
the "hello world" data transfer interface example code) that will
simply deliver a single fixed blob of data to its parent interface and
then close itself.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The design of IMDSv2 within Alibaba Cloud is identical to AWS IMDSv2,
with the header names changed from "X-aws-ec2-*" to "X-aliyun-ecs-*".
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Use an HTTP PUT request to fetch a session token, and pass this token
value as a header when fetching the user-data script.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
A commonly requested feature is to allow a setting to be populated
with the contents of an HTTP response. This currently requires a
somewhat ugly workaround of having the HTTP endpoint generate an iPXE
executable script fragment that includes the "#!ipxe" shebang and the
relevant "set" command.
For HTTP endpoints that are under the end user's control, this
workaround is viable (though still ugly). For HTTP endpoints that are
outside the user's control (such as the AWS metadata endpoints), this
workaround cannot be used.
Add an "imgset" command that can be used to store downloaded content
directly into a setting.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The design of IMDSv2 within both AWS and Alibaba Cloud requires the
client to obtain a temporary token via an HTTP PUT request. There is
no authentication on this request and there is no associated request
body: the requirement to use PUT exists solely to reduce the attack
surface for SSRF attacks (since vulnerable servers are much more
likely to be able to be tricked into issuing a GET request than a PUT
request).
iPXE can currently issue requests using HTTP GET (if the request body
is empty) or HTTP POST (if the request body includes form parameters).
There is no support for issuing a PUT request, or for allowing a
script to explicitly specify the HTTP method.
Add a "--method" option to the "params" command to allow an arbitrary
request method name to be specified, and use this as the HTTP request
method.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
If a named parameter block is created and then a URI is parsed that
attempts to use a nonexistent unnamed parameter block (or vice versa),
then the code in find_parameters() will currently call strcmp() with a
NULL argument, resulting in a read-only access to undefined memory.
Fix by calling strcmp() only for non-NULL names.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Some public clouds (such as AWS and Alibaba Cloud) allow for only a
single user metadata blob. The official iPXE cloud images will
attempt to download and boot from this user metadata, expecting it to
contain an iPXE script.
This works, but causes conflicts when another consumer (such as
cloud-init) also wants to use the same metadata blob. There are
workarounds (such as publishing the cloud-init script at an
alternative URI outside of the instance metadata service, and using
the iPXE script to direct cloud-init to use the alternative URI via
kernel command-line arguments), but these are cumbersome and may
weaken security since the alternative URI cannot provide the same
level of guaranteed access restrictions.
There is support within cloud-init for parsing a multipart MIME
archive, which may contain additional shell scripts, JSON data, etc,
alongside the cloud-init configuration itself. This is the standard
and documented method that cloud-init has chosen to solve the issue of
obtaining multiple data sources from a single user metadata blob.
Add support for multipart MIME as an archive image format from which
iPXE will extract the first body part that has the "text/x-ipxe" MIME
type. This allows the iPXE boot script to be placed alongside
cloud-init configuration within a single user metadata blob.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Michael Brown
