If the AMD microcode equivalence table is malformed and is not an
exact multiple of the entry size, then we may read up to two bytes
beyond the end of the allocated image.
The small out-of-bounds read is harmless since the immediately
following code will reject any image with fewer than eight bytes
remaining after the equivalence table (or will harmlessly return
immediately if the out-of-bounds read value was 0x00000000 and no
previous equivalence table entries were present).
Fix by adjusting the loop condition to ignore partial equivalence
table entries.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Add the instructions that Claude developed for itself over the course
of a very interactive week-long security audit of the iPXE codebase.
These instructions are to be used to guide any future use of AI agents
to search for security issues in iPXE.
Agents that follow these instructions are expected to surface only
relevant information, write up suitably minimalistic reports (unlike
the typical unguided AI slop that resulted in iPXE's current "(Ab)use
of AI" policy), and guide submission through the appropriate channels
that have been set up and documented in the security policy. Any
AI-authored reports are directed towards the "ipxe/aipxe" sandbox
repository, which exists to provide a clear separation between
human-generated and AI-generated content.
Given that repeated passes with Claude Opus 4.8 (and a cross-check
with Claude Fable) have converged to a clean state, it is expected
that publishing these instructions will lead to at most a trickle of
submissions, and that any such submissions should end up being
genuinely useful.
These instructions were written by Claude (with many hours of guidance
and refinement) and have not been modified, on the basis that an AI
agent knows best about what documentation it will itself find useful.
Unnecessary duplication has been avoided by documenting the key points
(e.g. bounds contracts) within the code's own Doxygen comments for
reference by both humans and agents, and ensuring that Claude's own
instructions refer and defer to this authoritative documentation.
Claude has not authored any code that was committed as part of this
week-long project. The AI agent instructions added by this commit
remain the only AI-authored content present in the tree. I have set
myself as the commit author (with an appropriate Authored-by credit
for Claude), written this commit message myself, and added my own
signoff, to confirm that I am the human owner taking long-term
responsibility for this contribution, regardless of its origin.
Authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Michael Brown <mcb30@ipxe.org>
With suitable guidance, AI agents such as Claude Code are capable of
scanning effectively for potential vulnerabilities, and reporting them
in a concise and actionable format.
These tools are now widely available to malicious actors, and so any
vulnerabilities that they are capable of finding must be fixed now
before they are inevitably found and potentially exploited.
The recent batch of commits over the past week closes all potential
vulnerabilities that were detectable by either Opus 4.8 or Fable in
multiple passes over the code. No serious security impact was found,
and there is nothing that would merit a UEFI Secure Boot revocation.
A concrete threat model is now documented, along with the explicit
bounds contracts for several internal APIs (such as ASN.1 parsing and
I/O buffer pointer manipulation). Some entire classes of nominal
defect (e.g. technically undefined behaviour arising from constant
left shifts into the sign bit) have been eliminated. False positives
that were raised several times and that could not be silenced through
reporting guidelines were fixed in the code, even when the code change
had no real-world impact. It is now possible to ask an appropriately
instructed AI agent to search for vulnerabilities in the iPXE codebase
and to be reasonably confident that anything that it reports is worth
investigating further.
Add a security policy to formally document the expectations upon both
humans and AI agents in terms of reporting potential vulnerabilities,
and update the contribution guidelines to grant a limited exception to
the blanket ban on AI-generated text.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Add a workflow that dispatches the synchronisation workflow in a
repository-defined list of downstream forks.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The code in fetch_numeric_setting() reads the setting value into a
local fixed-size buffer but then passes the full setting length to
numeric_setting_value(). If the setting length exceeds the size of
the fixed-size buffer, then numeric_setting_value() will continue to
read bytes from the stack.
The number of bytes read is constrained: numeric_setting_value() will
exit with -ERANGE as soon as the value being constructed exceeds the
range of an unsigned long. The existence of a return address on the
stack thus provides an upper bound on how far numeric_setting_value()
can read before terminating with an error.
Creating a setting with a length of more than an unsigned long is
trivial, for example:
set thing:hexraw 00000000000000000000000000000000
However, the out-of-bounds read can be reached only via calls to the
fetch_[u]int[z]_setting() family of internal helper functions.
Reading the setting in a script via e.g. ${thing:uint32} goes via a
different code path that does not use a fixed-length buffer.
The fetch_[u]int[z]_setting() functions are called from only a few
places. Most uses are for boolean flags or bit masks. A few are
genuinely used as numeric values: the settings mechanism itself reads
and uses the "priority" setting, the network core reads the "mtu"
setting, and the SAN boot mechanism reads the drive number and retry
count.
An extremely determined attacker could potentially obtain up to eight
bytes of information from the stack (in a 64-bit build) by, for
example, creating two sibling settings blocks where one has an
overlength "priority" setting value, and then repeatedly manipulating
the priority in the other settings block and testing to see which
block ends up with the higher priority. The information that could be
obtained in this way is limited to the temporary values stored on the
stack by fetch_numeric_setting() itself, along with its own return
address. None of this information is security-sensitive, and so any
information leakage is a mere curiosity.
Fix by allocating a temporary copy within fetch_numeric_setting()
instead of using a fixed-size buffer. This has the downside of
introducing an otherwise unnecessary memory allocation (which could
potentially itself fail), but guarantees consistency with other
numeric interpretations of setting values. (The alternative approach
of rejecting overlength setting values would introduce a potential
inconsistency between the value returned by fetch_numeric_setting()
and the value obtained by formatting a setting using a numeric setting
type, or by numerating the setting.)
Signed-off-by: Michael Brown <mcb30@ipxe.org>
A DHCP static route option is capable of encoding an invalid subnet
mask width of greater than 32 bits. This leads to a technically
undefined left shift when calculating the 32-bit subnet mask.
There is no security impact of this undefined shift: the only possible
outcome is that the subnet mask for the improperly defined static
route ends up holding an invalid value.
Fix by checking the range before performing the shift, to eliminate
future reporting noise.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
Add an assortment of missing length checks that can currently result
in reads of uninitialised data from within the Ethernet frame padding
region of a received I/O buffer.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
The EFI command line is not necessarily terminated with a wNUL
character. We currently use snprintf() with an output buffer size to
constrain the write to the correct size and ensure that a NUL
terminator exists (as required for the image data), but nothing
prevents snprintf() from continuing to pointlessly read beyond the end
of the wide-character command line until it happens to encounter a
wNUL somewhere.
Fix by creating a temporary wNUL-terminated copy of the EFI command
line and then converting that (in situ) to ASCII.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
A caller that places an invalid value in the IpCnt field would cause
iPXE to read beyond the end of the IpList array.
This has no meaningful security impact: there is no out-of-bounds
write, and a caller with the ability to place an invalid value in the
IpCnt field would already have to be a Secure Boot signed binary (if
Secure Boot is enabled).
Fix by limiting the traversal of IpList to the lower of IpCnt or the
array size, to reduce unwanted noise from security reviewers.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
EFI device paths generally have no externally defined length: the only
way to calculate the length is to scan the device path itself (and
therefore to implicitly assume that the path is valid).
There is no way to guard against a malformed device path (absent the
atypical existence of an external length), but we can at least prevent
infinite loops from a device path component that encodes a zero
length.
Treat any device path component with a length too short to contain the
device path header as ending the device path. This does not prevent
invalid device paths from being accepted, but it does at least guard
against a silent system hang from an infinite loop, and ensures that
callers may safely subtract the length of the device path header from
the length of the path component without underflowing.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
EFI device paths generally have no externally defined length: the only
way to calculate the length is to scan the device path itself (and
therefore to implicitly assume that the path is valid).
The EFI load option structure does have an externally defined length
field, and we currently attempt to validate against this. The
validation logic is missing a crucial step which renders it
ineffective: the overall effect is essentially equivalent to trusting
that the system's configured load option structures are well-formed.
(This is a reasonable assumption: the length check exists primarily as
a defence against external bugs, and an attacker with the ability to
change the system load options has already compromised the system.)
Fix by updating the remaining length correctly as we traverse the
device path.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
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>
Michael Brown