| Message ID | 1481298289-13546-2-git-send-email-ian.jackson@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote: > This will allow us to keep track of the total amount of work we are > doing. When it becomes excessive, we mark the ELF broken, and stop > processing. > > This is a more robust way of preventing DoS problems by bad images > than attempting to prove, for each of the (sometimes rather deeply > nested) loops, that the total work is "reasonable". We bound the > notional work by 4x the image size (plus 1M). The 4x has been selected arbitrarily, I suppose? > --- a/xen/common/libelf/libelf-loader.c > +++ b/xen/common/libelf/libelf-loader.c > @@ -38,6 +38,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char > *image_input, size_t > ELF_HANDLE_DECL(elf_shdr) shdr; > unsigned i, count, section, link; > uint64_t offset; > + const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR; This will need adjustment for 32-bit tool stack builds - did you perhaps mean UINT64_C(1), considering the type of the variable? Also please add blanks around the / . > @@ -546,6 +551,15 @@ uint64_t elf_lookup_addr(struct elf_binary * elf, const char *symbol) > return value; > } > > +bool elf_iter_ok_counted(struct elf_binary *elf, uint64_t maxcopysz) { > + if (maxcopysz > elf->iteration_deaccumulator) > + elf_mark_broken(elf, "excessive iteration - too much work to parse"); > + if (elf->broken) > + return false; > + elf->iteration_deaccumulator -= maxcopysz; > + return true; > +} Hypervisor coding style here please (missing lots of blanks, and the opening brace needs to go on its own line). I think there are more such style problems further down. And then - would it perhaps make sense to have most of this function's body in #ifndef __XEN__, as there's nothing to guard against when loading Dom0? Jan
>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote: > +static inline bool elf_iter_ok(struct elf_binary *elf) > + { return elf_iter_ok_counted(elf,1); } Having seen first uses of this - why is this 1 instead of 0? Once it is, calling elf_iter_ok_counted() here would be rather pointless, and checking just elf_broken() here would allow the parameter to become const. Jan
Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe"):
> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> > This is a more robust way of preventing DoS problems by bad images
> > than attempting to prove, for each of the (sometimes rather deeply
> > nested) loops, that the total work is "reasonable". We bound the
> > notional work by 4x the image size (plus 1M).
>
> The 4x has been selected arbitrarily, I suppose?
Yes.
> > uint64_t offset;
> > + const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;
>
> This will need adjustment for 32-bit tool stack builds - did you
> perhaps mean UINT64_C(1), considering the type of the variable?
Oops, yes. Although it has to be (uint64_t)1 since the tools do not
have UINT64_C.
> Also please add blanks around the / .
Done.
> Hypervisor coding style here please (missing lots of blanks, and the
> opening brace needs to go on its own line). I think there are more
> such style problems further down.
Sorry, I will (try to) fix the style in all the patches.
> And then - would it perhaps make sense to have most of this
> function's body in #ifndef __XEN__, as there's nothing to guard
> against when loading Dom0?
This is a good idea. If it's OK with you I'll leave the variable
initialisation etc., and simply stub out body of elf_iter_ok_counted.
If you want a more comprehensive approach I can add more #ifdefs.
Thanks,
Ian.
>>> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote: > --- a/xen/common/libelf/libelf-loader.c > +++ b/xen/common/libelf/libelf-loader.c > @@ -38,6 +38,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t > ELF_HANDLE_DECL(elf_shdr) shdr; > unsigned i, count, section, link; > uint64_t offset; > + const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR; > > if ( !elf_is_elfbinary(image_input, size) ) > { > @@ -52,6 +53,10 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t > elf->class = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_CLASS]); > elf->data = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_DATA]); > > + elf->iteration_deaccumulator = 1024*1024 + > + (size > max_size_for_deacc ? max_size_for_deacc : size) > + * ELF_MAX_ITERATION_FACTOR; One more question here: Is this useful at all? You're allowing for approximately 2**63 accounted operations - how big does an image need to be to actually break this limit? XSA-25 already limited image sizes to 1Gb (but I do understand that the guarding here is also against e.g. redundant loading of the same bits through multiple program header table entries). And how long will it take you to reach that limit (and to cause elf->broken to be set)? With 1ns per accounted operation, that'll be on the order of 270 years. Am I missing something here? Jan
Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe"):
> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote:
> > + const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR;
...
> > + elf->iteration_deaccumulator = 1024*1024 +
> > + (size > max_size_for_deacc ? max_size_for_deacc : size)
> > + * ELF_MAX_ITERATION_FACTOR;
>
> One more question here: Is this useful at all? You're allowing
> for approximately 2**63 accounted operations - how big does
> an image need to be to actually break this limit? XSA-25 already
> limited image sizes to 1Gb (but I do understand that the
> guarding here is also against e.g. redundant loading of the
> same bits through multiple program header table entries).
>
> And how long will it take you to reach that limit (and to cause
> elf->broken to be set)? With 1ns per accounted operation,
> that'll be on the order of 270 years. Am I missing something
> here?
I'm not sure of your point.
Mostly I allow for 4x the size of the image, plus a fixed constant of
1M operations.
The max_size_for_deacc part is necessary because otherwise the
calculation "size * ELF_MAX_ITERATION_FACTOR" might overflow. It
seems unreasonable to simply allow that overflow to occur. But if it
is causing confusion we could do that. The result would be a low
value for iteration_deaccumulator.
In practice the 1Gby image size limit will prevent this limit from
being reached, but it is distant from this check.
Ian.
>>> On 12.12.16 at 17:00, <ian.jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce > elf_iter_ok and elf_strcmp_safe"): >> On 09.12.16 at 16:44, <ian.jackson@eu.citrix.com> wrote: >> > + const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR; > ... >> > + elf->iteration_deaccumulator = 1024*1024 + >> > + (size > max_size_for_deacc ? max_size_for_deacc : size) >> > + * ELF_MAX_ITERATION_FACTOR; >> >> One more question here: Is this useful at all? You're allowing >> for approximately 2**63 accounted operations - how big does >> an image need to be to actually break this limit? XSA-25 already >> limited image sizes to 1Gb (but I do understand that the >> guarding here is also against e.g. redundant loading of the >> same bits through multiple program header table entries). >> >> And how long will it take you to reach that limit (and to cause >> elf->broken to be set)? With 1ns per accounted operation, >> that'll be on the order of 270 years. Am I missing something >> here? > > I'm not sure of your point. > > Mostly I allow for 4x the size of the image, plus a fixed constant of > 1M operations. Well, I have to confess that I've read the above as max() when in fact it is min(). > The max_size_for_deacc part is necessary because otherwise the > calculation "size * ELF_MAX_ITERATION_FACTOR" might overflow. It > seems unreasonable to simply allow that overflow to occur. But if it > is causing confusion we could do that. The result would be a low > value for iteration_deaccumulator. Considering that overflow here will actually result in a comparably smaller upper limit, I think this may help overall readability. But with the above I won't insist on this in any way. Jan
Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe"):
> Well, I have to confess that I've read the above as max() when
> in fact it is min().
Sadly we can't use min() and max() here it seems.
> On 12.12.16 at 17:00, <ian.jackson@eu.citrix.com> wrote:
> > The max_size_for_deacc part is necessary because otherwise the
> > calculation "size * ELF_MAX_ITERATION_FACTOR" might overflow. It
> > seems unreasonable to simply allow that overflow to occur. But if it
> > is causing confusion we could do that. The result would be a low
> > value for iteration_deaccumulator.
>
> Considering that overflow here will actually result in a comparably
> smaller upper limit, I think this may help overall readability. But with
> the above I won't insist on this in any way.
I have replaced the limit with a comment. Now I have:
elf->iteration_deaccumulator =
1024*1024 + size * ELF_MAX_ITERATION_FACTOR;
/* overflow (from very big size, probably rejected earlier)
* would just lead to small limit, which is safe */
Ian.
>>> On 12.12.16 at 17:56, <ian.jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce > elf_iter_ok and elf_strcmp_safe"): >> Well, I have to confess that I've read the above as max() when >> in fact it is min(). > > Sadly we can't use min() and max() here it seems. Sure, I understand that. >> On 12.12.16 at 17:00, <ian.jackson@eu.citrix.com> wrote: >> > The max_size_for_deacc part is necessary because otherwise the >> > calculation "size * ELF_MAX_ITERATION_FACTOR" might overflow. It >> > seems unreasonable to simply allow that overflow to occur. But if it >> > is causing confusion we could do that. The result would be a low >> > value for iteration_deaccumulator. >> >> Considering that overflow here will actually result in a comparably >> smaller upper limit, I think this may help overall readability. But with >> the above I won't insist on this in any way. > > I have replaced the limit with a comment. Now I have: > > elf->iteration_deaccumulator = > 1024*1024 + size * ELF_MAX_ITERATION_FACTOR; > /* overflow (from very big size, probably rejected earlier) > * would just lead to small limit, which is safe */ Thanks. May I ask that you then also use proper hypervisor style for that comment? Jan
Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce elf_iter_ok and elf_strcmp_safe"):
> On 12.12.16 at 17:56, <ian.jackson@eu.citrix.com> wrote:
> > I have replaced the limit with a comment. Now I have:
> >
> > elf->iteration_deaccumulator =
> > 1024*1024 + size * ELF_MAX_ITERATION_FACTOR;
> > /* overflow (from very big size, probably rejected earlier)
> > * would just lead to small limit, which is safe */
>
> Thanks. May I ask that you then also use proper hypervisor
> style for that comment?
You mean like this ?
elf->iteration_deaccumulator =
1024*1024 + size * ELF_MAX_ITERATION_FACTOR;
/*
* overflow (from very big size, probably rejected earlier)
* would just lead to small limit, which is safe
*/
Sure, whatever. Done.
Ian.
>>> On 13.12.16 at 17:04, <ian.jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: [PATCH 1/8] libelf: loop safety: Introduce > elf_iter_ok and elf_strcmp_safe"): >> On 12.12.16 at 17:56, <ian.jackson@eu.citrix.com> wrote: >> > I have replaced the limit with a comment. Now I have: >> > >> > elf->iteration_deaccumulator = >> > 1024*1024 + size * ELF_MAX_ITERATION_FACTOR; >> > /* overflow (from very big size, probably rejected earlier) >> > * would just lead to small limit, which is safe */ >> >> Thanks. May I ask that you then also use proper hypervisor >> style for that comment? > > You mean like this ? > > elf->iteration_deaccumulator = > 1024*1024 + size * ELF_MAX_ITERATION_FACTOR; > /* > * overflow (from very big size, probably rejected earlier) > * would just lead to small limit, which is safe > */ > > Sure, whatever. Done. Almost: Comments are also supposed to start with an upper case letter. But I'm fine either way. Jan
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index a72cd8a..00479af 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -38,6 +38,7 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t ELF_HANDLE_DECL(elf_shdr) shdr; unsigned i, count, section, link; uint64_t offset; + const uint64_t max_size_for_deacc = (1UL << 63)/ELF_MAX_ITERATION_FACTOR; if ( !elf_is_elfbinary(image_input, size) ) { @@ -52,6 +53,10 @@ elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t elf->class = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_CLASS]); elf->data = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_DATA]); + elf->iteration_deaccumulator = 1024*1024 + + (size > max_size_for_deacc ? max_size_for_deacc : size) + * ELF_MAX_ITERATION_FACTOR; + /* Sanity check phdr. */ offset = elf_uval(elf, elf->ehdr, e_phoff) + elf_uval(elf, elf->ehdr, e_phentsize) * elf_phdr_count(elf); @@ -546,6 +551,15 @@ uint64_t elf_lookup_addr(struct elf_binary * elf, const char *symbol) return value; } +bool elf_iter_ok_counted(struct elf_binary *elf, uint64_t maxcopysz) { + if (maxcopysz > elf->iteration_deaccumulator) + elf_mark_broken(elf, "excessive iteration - too much work to parse"); + if (elf->broken) + return false; + elf->iteration_deaccumulator -= maxcopysz; + return true; +} + /* * Local variables: * mode: C diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index 1b763f3..294231a 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -56,6 +56,8 @@ typedef void elf_log_callback(struct elf_binary*, void *caller_data, #define ELF_MAX_STRING_LENGTH 4096 #define ELF_MAX_TOTAL_NOTE_COUNT 65536 +#define ELF_MAX_ITERATION_FACTOR 4 + /* ------------------------------------------------------------------------ */ /* Macros for accessing the input image and output area. */ @@ -201,6 +203,9 @@ struct elf_binary { uint64_t bsd_symtab_pstart; uint64_t bsd_symtab_pend; + /* private */ + uint64_t iteration_deaccumulator; + /* * caller's other acceptable destination. * Set by elf_set_xdest. Do not set these directly. @@ -264,6 +269,14 @@ uint64_t elf_access_unsigned(struct elf_binary *elf, elf_ptrval ptr, uint64_t elf_round_up(struct elf_binary *elf, uint64_t addr); +bool elf_iter_ok_counted(struct elf_binary *elf, uint64_t count); + /* It is OK for count to be out by a smallish constant factor. + * It is OK for count to be 0, as we clamp it to 1, so we + * can use lengths or sizes from the image. */ + +static inline bool elf_iter_ok(struct elf_binary *elf) + { return elf_iter_ok_counted(elf,1); } + const char *elf_strval(struct elf_binary *elf, elf_ptrval start); /* may return NULL if the string is out of range etc. */ @@ -463,6 +476,14 @@ static inline void *elf_memset_unchecked(void *s, int c, size_t n) * memcpy, memset and memmove to undefined MISTAKE things. */ +static inline int elf_strcmp_safe(struct elf_binary *elf, + const char *a, const char *b) { + elf_iter_ok_counted(elf, strlen(b)); + return strcmp(a,b); +} + /* Unlike other *_safe functions, elf_strcmp_safe is called on + * values already extracted from the image (eg by elf_strval), + * and fixed constant strings (typically, the latter is "b"). */ /* Advances past amount bytes of the current destination area. */ static inline void ELF_ADVANCE_DEST(struct elf_binary *elf, uint64_t amount)
This will allow us to keep track of the total amount of work we are doing. When it becomes excessive, we mark the ELF broken, and stop processing. This is a more robust way of preventing DoS problems by bad images than attempting to prove, for each of the (sometimes rather deeply nested) loops, that the total work is "reasonable". We bound the notional work by 4x the image size (plus 1M). Also introduce elf_strcmp_safe, which unconditionally does the work, but increments the count so any outer loop may be aborted if necessary. Currently there are no callers, so no functional change. Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com> --- xen/common/libelf/libelf-loader.c | 14 ++++++++++++++ xen/include/xen/libelf.h | 21 +++++++++++++++++++++ 2 files changed, 35 insertions(+)