| Message ID | 20220902170310.959922-1-sbabic@denx.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | parser: allow both string and number for "offset" | expand |
Stefano Babic wrote on Fri, Sep 02, 2022 at 07:03:10PM +0200: > The offset attribute can be misleading. It is defined as string and then > converted by the parser, but it is usually set as number in decimal or > hexadecimal format. > > In case a number is put in sw-description, the offset is set to 0. This > can cause to write at a wrong offset and no warning or error are raised. > > Change this and let to set the offset in both formats. These example > will generate the same offset for SWUpdate: > > as number like: > > offset = 1024; > or > offset = 0x400; > > or as a string: > offset = "1024"; > or > offset = "1K"; > > Signed-off-by: Stefano Babic <sbabic@denx.de> > --- > parser/parser.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/parser/parser.c b/parser/parser.c > index cf2b324..5607031 100644 > --- a/parser/parser.c > +++ b/parser/parser.c > @@ -385,6 +385,7 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im > { > char seek_str[MAX_SEEK_STRING_SIZE]; > const char* compressed; > + unsigned long offset = 0; > > /* > * GET_FIELD_STRING does not touch the passed string if it is not > @@ -401,15 +402,24 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im > GET_FIELD_STRING(p, elem, "mtdname", image->mtdname); > GET_FIELD_STRING(p, elem, "filesystem", image->filesystem); > GET_FIELD_STRING(p, elem, "type", image->type); > + get_field(p, elem, "offset", &offset); hmm, that's a bit unsafe for json isn't it? get_field -> get_field_json -> get_value_json will just strcpy to whatever we pointed at, so it'll merrily overflow the 4 or 8 bytes of our long if we're parsing a string (which is likely) for libconfig, get_value_libconfig merrily just copes the string pointer into a local variable and never lets it out so it... just does nothing... which isn't really a problem as GET_FIELD_STRING uses a different function that behaves correctly, but at least strings are safe enough. We could get an overflow though if we run on an arch where long long != long (32 bit platforms do iirc?), in which case it'll write too much as well. It's a bit of work but we probably ought to add get_field_TYPE for types other that string that do the right thing and only accept proper elements. (note in practice the config is signed, so this isn't an attack vector, just another way people can shoot themselves in the foot. In particular, string + json should be valid and must not overflow in my opinion) Thanks,
Hi Dominique, On 06.09.22 10:41, Dominique MARTINET wrote: > Stefano Babic wrote on Fri, Sep 02, 2022 at 07:03:10PM +0200: >> The offset attribute can be misleading. It is defined as string and then >> converted by the parser, but it is usually set as number in decimal or >> hexadecimal format. >> >> In case a number is put in sw-description, the offset is set to 0. This >> can cause to write at a wrong offset and no warning or error are raised. >> >> Change this and let to set the offset in both formats. These example >> will generate the same offset for SWUpdate: >> >> as number like: >> >> offset = 1024; >> or >> offset = 0x400; >> >> or as a string: >> offset = "1024"; >> or >> offset = "1K"; >> >> Signed-off-by: Stefano Babic <sbabic@denx.de> >> --- >> parser/parser.c | 20 +++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/parser/parser.c b/parser/parser.c >> index cf2b324..5607031 100644 >> --- a/parser/parser.c >> +++ b/parser/parser.c >> @@ -385,6 +385,7 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im >> { >> char seek_str[MAX_SEEK_STRING_SIZE]; >> const char* compressed; >> + unsigned long offset = 0; >> >> /* >> * GET_FIELD_STRING does not touch the passed string if it is not >> @@ -401,15 +402,24 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im >> GET_FIELD_STRING(p, elem, "mtdname", image->mtdname); >> GET_FIELD_STRING(p, elem, "filesystem", image->filesystem); >> GET_FIELD_STRING(p, elem, "type", image->type); >> + get_field(p, elem, "offset", &offset); > > hmm, that's a bit unsafe for json isn't it? > > get_field -> get_field_json -> get_value_json will just strcpy to > whatever we pointed at, so it'll merrily overflow the 4 or 8 bytes of > our long if we're parsing a string (which is likely) > > for libconfig, get_value_libconfig merrily just copes the string pointer > into a local variable and never lets it out so it... I confess I have only tested with libconfig. So you get a good point. I have thought to just add support for both (number and string), but this is maybe overkilling. I was debugging an issue from a customer of mine, and the bootloader was update, with something like this: filename = "u-boot"; offset = 0x400; type =" flash"; Because offset is not recognized, SWUpdate goes further and offset is (default value) 0. No error and then device is bricked (of course). I have to tethinhk this, of course. One way could be just to recognize if a number is set as offset, and raise an error. It is clear that the error is due to a wrong sw-description, but anyway it should be raised as soon as possible. > just does > nothing... which isn't really a problem as GET_FIELD_STRING uses a > different function that behaves correctly, but at least strings are safe > enough. Right, this is also the reason that STRINGS are generally preferred here. > We could get an overflow though if we run on an arch where long long != > long (32 bit platforms do iirc?), in which case it'll write too much as > well. But this is due to the variable "offset" I declared on the stack, right ? > > > It's a bit of work but we probably ought to add get_field_TYPE for types > other that string that do the right thing and only accept proper > elements. This could be done, but I will also happy if I can simply reject the case above and raise an error. > > > (note in practice the config is signed, so this isn't an attack vector, No, it is not > just another way people can shoot themselves in the foot. Exactly. > In particular, > string + json should be valid and must not overflow in my opinion) I agree with you. Regards, Stefano > > > Thanks,
Stefano Babic wrote on Wed, Sep 07, 2022 at 03:32:56PM +0200: > > > diff --git a/parser/parser.c b/parser/parser.c > > > index cf2b324..5607031 100644 > > > --- a/parser/parser.c > > > +++ b/parser/parser.c > > > @@ -385,6 +385,7 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im > > > { > > > char seek_str[MAX_SEEK_STRING_SIZE]; > > > const char* compressed; > > > + unsigned long offset = 0; > > > /* > > > * GET_FIELD_STRING does not touch the passed string if it is not > > > @@ -401,15 +402,24 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im > > > GET_FIELD_STRING(p, elem, "mtdname", image->mtdname); > > > GET_FIELD_STRING(p, elem, "filesystem", image->filesystem); > > > GET_FIELD_STRING(p, elem, "type", image->type); > > > + get_field(p, elem, "offset", &offset); > > > > hmm, that's a bit unsafe for json isn't it? > > > > get_field -> get_field_json -> get_value_json will just strcpy to > > whatever we pointed at, so it'll merrily overflow the 4 or 8 bytes of > > our long if we're parsing a string (which is likely) > > > > for libconfig, get_value_libconfig merrily just copes the string pointer > > into a local variable and never lets it out so it... > > I confess I have only tested with libconfig. So you get a good point. > > I have thought to just add support for both (number and string), but this is > maybe overkilling. I was debugging an issue from a customer of mine, and the > bootloader was update, with something like this: > > filename = "u-boot"; > offset = 0x400; > type =" flash"; > > Because offset is not recognized, SWUpdate goes further and offset is > (default value) 0. No error and then device is bricked (of course). > > I have to tethinhk this, of course. One way could be just to recognize if a > number is set as offset, and raise an error. It is clear that the error is > due to a wrong sw-description, but anyway it should be raised as soon as > possible. Yes, I definitely see the use-case -- either handling it simimlarly to this patch, or straight out error if we expect a string and get something else at parser level sounds good. We probably want to error in other places as well if the type we got was not expected, so I think this is a good idea to add such a check. > > We could get an overflow though if we run on an arch where long long != > > long (32 bit platforms do iirc?), in which case it'll write too much as > > well. > > But this is due to the variable "offset" I declared on the stack, right ? yes, declaring it as long long would fix this particular problem, but it's not exactly pretty and doesn't help with json. > > It's a bit of work but we probably ought to add get_field_TYPE for types > > other that string that do the right thing and only accept proper > > elements. > > This could be done, but I will also happy if I can simply reject the case > above and raise an error. I'm fine either way, but we have similar problems if someone uses get_field elsewhere, so it might be worth doing regardless? I think we should eventually aim to get rid of get_field and always specify the type we want... But that's not a new problem so it can be put on some backlog and be done when someone has time; as said below this isn't a security problem so there is no rush. Thanks,
diff --git a/parser/parser.c b/parser/parser.c index cf2b324..5607031 100644 --- a/parser/parser.c +++ b/parser/parser.c @@ -385,6 +385,7 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im { char seek_str[MAX_SEEK_STRING_SIZE]; const char* compressed; + unsigned long offset = 0; /* * GET_FIELD_STRING does not touch the passed string if it is not @@ -401,15 +402,24 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im GET_FIELD_STRING(p, elem, "mtdname", image->mtdname); GET_FIELD_STRING(p, elem, "filesystem", image->filesystem); GET_FIELD_STRING(p, elem, "type", image->type); + get_field(p, elem, "offset", &offset); GET_FIELD_STRING(p, elem, "offset", seek_str); GET_FIELD_STRING(p, elem, "data", image->type_data); get_hash_value(p, elem, image->sha256); - /* convert the offset handling multiplicative suffixes */ - image->seek = ustrtoull(seek_str, NULL, 0); - if (errno){ - ERROR("offset argument: ustrtoull failed"); - return -1; + /* + * offset can be set as number or string. As string, + * multiplier suffixes are allowed + */ + if (offset) + image->seek = offset; + else { + /* convert the offset handling multiplicative suffixes */ + image->seek = ustrtoull(seek_str, NULL, 0); + if (errno){ + ERROR("offset argument: ustrtoull failed"); + return -1; + } } if ((compressed = get_field_string(p, elem, "compressed")) != NULL) {
The offset attribute can be misleading. It is defined as string and then converted by the parser, but it is usually set as number in decimal or hexadecimal format. In case a number is put in sw-description, the offset is set to 0. This can cause to write at a wrong offset and no warning or error are raised. Change this and let to set the offset in both formats. These example will generate the same offset for SWUpdate: as number like: offset = 1024; or offset = 0x400; or as a string: offset = "1024"; or offset = "1K"; Signed-off-by: Stefano Babic <sbabic@denx.de> --- parser/parser.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)