[settings] Fix limited out-of-bounds read in fetch_numeric_setting()

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>

diff --git a/src/core/settings.c b/src/core/settings.c
index 2e576033026154a7bf985c53c4c539a78dad37ff..221d8a1bd067e2fbc391c45b719770a11ea04031 100644 (file)
--- a/src/core/settings.c
+++ b/src/core/settings.c
@@ -998,19 +998,29 @@ static int numeric_setting_value ( int is_signed, const void *raw, size_t len,
 int fetch_numeric_setting ( struct settings *settings,
                            const struct setting *setting,
                            unsigned long *value, int is_signed ) {
-       unsigned long tmp;
+       void *raw;
+       int raw_len;
        int len;
 
        /* Avoid returning uninitialised data on error */
        *value = 0;
 
        /* Fetch raw (network-ordered, variable-length) setting */
-       len = fetch_raw_setting ( settings, setting, &tmp, sizeof ( tmp ) );
-       if ( len < 0 )
-               return len;
+       raw_len = fetch_raw_setting_copy ( settings, setting, &raw );
+       if ( raw_len < 0 ) {
+               len = raw_len;
+               goto err_fetch_copy;
+       }
 
        /* Extract numeric value */
-       return numeric_setting_value ( is_signed, &tmp, len, value );
+       len = numeric_setting_value ( is_signed, raw, raw_len, value );
+       if ( len < 0 )
+               goto err_value;
+
+ err_value:
+       free ( raw );
+ err_fetch_copy:
+       return len;
 }
 
 /**