| Message ID | 20241030053747.3905143-1-dominique.martinet@atmark-techno.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | core/parser: add 'size' attribute to sw-description | expand |
Hi Dominique, > This allows specifying the size of each cpio file in the sw-description > > The motivation behind this change is that files are only verified as > they are copied to temporary directory or streamed, but without the size > information a file could be streamed forever and easily fill in the tmp > directory by replacing the files of a valid (signed) SWU. I like the idea of protecting the temporary folder against flooding by buggy or malicious SWU files. In your patch I can see upfront, exact file size checks using the != operator. But shouldn't there also be somewhere a "larger than" check which prevents exactly the mentioned "streamed forever" scenario? > This will be useful even if a chunked checksum is implemented, because > while the chunked checksum implicitly also validates the files length it > is not useful to include chunked checksums for files that have an > intermediate copy stored, and it is more efficient and simpler to only > have this size information. > > Implementation note: SEARCH_FILE (updated in swupdate.h) is only used in > cpio_scan, itself never used anywhere in the current code, so this part > of the patch has no way of being tested. > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > This is another point I had brought up in the discussions this summer. > > It is much simpler both conceptually and in the implementation than > chunked checksums, so we can probably get this in more easily -- please > let me know if you want me to fix something. > > core/cpio_utils.c | 6 ++++++ > core/installer.c | 6 ++++++ > doc/source/sw-description.rst | 5 +++++ > include/swupdate.h | 1 - > parser/parser.c | 1 + > 5 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/core/cpio_utils.c b/core/cpio_utils.c > index 382b22c36434..7f642685b0d4 100644 > --- a/core/cpio_utils.c > +++ b/core/cpio_utils.c > @@ -799,6 +799,12 @@ int cpio_scan(int fd, struct swupdate_cfg *cfg, off_t start) > fdh.size, > file_listed ? "REQUIRED" : "not required"); > > + if (img->size && img->size != fdh.size) { > + ERROR("%s: size mismatch %llu / %lu", > + img->fname, img->size, fdh.size); > + return -1; > + } > + img->size = fdh.size; > /* > * use copyfile for checksum and hash verification, as we skip file > * we do not have to provide fdout > diff --git a/core/installer.c b/core/installer.c > index 0cb06b2ca419..e3abfd30ddf6 100644 > --- a/core/installer.c > +++ b/core/installer.c > @@ -58,6 +58,12 @@ swupdate_file_t check_if_required(struct imglist *list, struct filehdr *pfdh, > if (strcmp(pfdh->filename, img->fname) == 0) { > skip = COPY_FILE; > img->provided = 1; > + if (img->size && img->size != (unsigned int)pfdh->size) { > + ERROR("Size in sw-description %llu does not match size in cpio %u", > + img->size, (unsigned int)pfdh->size); > + return -EINVAL; > + > + } > img->size = (unsigned int)pfdh->size; > > if (snprintf(img->extract_file, > diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst > index d4cb8971b4ed..7d13e1e65d18 100644 > --- a/doc/source/sw-description.rst > +++ b/doc/source/sw-description.rst > @@ -1491,3 +1491,8 @@ There are 4 main sections inside sw-description: > | | | | the mtd to update, instead of | > | | | | specifying the devicenode | > +-------------+----------+------------+---------------------------------------+ > + | size | int64 | images | size of the file as it is expected | > + | | | files | in the SWU. If set and the cpio size | > + | | | scripts | does not match for some reason the | > + | | | | update will fail with an error. | > + +-------------+----------+------------+---------------------------------------+ > diff --git a/include/swupdate.h b/include/swupdate.h > index 7cf421047187..a68c0950dd3f 100644 > --- a/include/swupdate.h > +++ b/include/swupdate.h > @@ -96,7 +96,6 @@ struct swupdate_cfg { > found = 1; \ > img->offset = offs; \ > img->provided = 1; \ > - img->size = fdh.size; \ > } \ > } \ > if (!found) \ > diff --git a/parser/parser.c b/parser/parser.c > index f5113f94841b..52bee8cbf11d 100644 > --- a/parser/parser.c > +++ b/parser/parser.c > @@ -420,6 +420,7 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im > GET_FIELD_STRING(p, elem, "filesystem", image->filesystem); > GET_FIELD_STRING(p, elem, "type", image->type); > GET_FIELD_STRING(p, elem, "data", image->type_data); > + GET_FIELD_INT64(p, elem, "size", &image->size); > get_hash_value(p, elem, image->sha256); > > /* > -- > 2.39.5 Cheers, Mark
Hi Mark, Mark Jonas wrote on Wed, Oct 30, 2024 at 09:09:43AM +0100: > > This allows specifying the size of each cpio file in the sw-description > > > > The motivation behind this change is that files are only verified as > > they are copied to temporary directory or streamed, but without the size > > information a file could be streamed forever and easily fill in the tmp > > directory by replacing the files of a valid (signed) SWU. > > I like the idea of protecting the temporary folder against flooding by > buggy or malicious SWU files. In your patch I can see upfront, exact > file size checks using the != operator. But shouldn't there also be > somewhere a "larger than" check which prevents exactly the mentioned > "streamed forever" scenario? I'm not sure I understand the question; this patch checks that if the size is specified in sw-description, and the size from the cpio header didn't match then we error out immediately instead of streaming/storing the file; so it doesn't need any "larger than" as anything different will fail. Is it about "if the size isn't specified in sw-description then we should still have a upper limit"? Technically cpio limits each file to 4GB so "forever" isn't quite possible, but that's still quite a nuisance, so I'll just be setting a size attribute to all my files in sw-description. Thanks,
Hi Dominique, On 30.10.24 06:37, Dominique Martinet wrote: > This allows specifying the size of each cpio file in the sw-description > > The motivation behind this change is that files are only verified as > they are copied to temporary directory or streamed, but without the size > information a file could be streamed forever and easily fill in the tmp > directory by replacing the files of a valid (signed) SWU. > > This will be useful even if a chunked checksum is implemented, because > while the chunked checksum implicitly also validates the files length it > is not useful to include chunked checksums for files that have an > intermediate copy stored, and it is more efficient and simpler to only > have this size information. > > Implementation note: SEARCH_FILE (updated in swupdate.h) is only used in > cpio_scan, Right. > itself never used anywhere in the current code, so this part > of the patch has no way of being tested. > I implemented cpio_scan() in the past when I had two different paths for local update (update from file, -i option) and network (streaming). After merging the two paths, cpio_scan is dangling - thanks for note this. IMHO it can be safely removed. I send a patch for remove it. > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > This is another point I had brought up in the discussions this summer. > > It is much simpler both conceptually and in the implementation than > chunked checksums, so we can probably get this in more easily -- please > let me know if you want me to fix something. Agree - this is not a big change, the only concern is we must maintain compatibility with the past. After merging, older SWUs can still be installed. A new "size" attribute is ignored by older SWUpdate, so there shouln't be issue, but it should be tested before merging. > > core/cpio_utils.c | 6 ++++++ > core/installer.c | 6 ++++++ > doc/source/sw-description.rst | 5 +++++ > include/swupdate.h | 1 - > parser/parser.c | 1 + > 5 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/core/cpio_utils.c b/core/cpio_utils.c > index 382b22c36434..7f642685b0d4 100644 > --- a/core/cpio_utils.c > +++ b/core/cpio_utils.c > @@ -799,6 +799,12 @@ int cpio_scan(int fd, struct swupdate_cfg *cfg, off_t start) > fdh.size, > file_listed ? "REQUIRED" : "not required"); > > + if (img->size && img->size != fdh.size) { > + ERROR("%s: size mismatch %llu / %lu", > + img->fname, img->size, fdh.size); > + return -1; > + } It is ok that img-> size is checked, so if sw-description does not contain the size, it wouldn't hurt. > + img->size = fdh.size; > /* > * use copyfile for checksum and hash verification, as we skip file > * we do not have to provide fdout > diff --git a/core/installer.c b/core/installer.c > index 0cb06b2ca419..e3abfd30ddf6 100644 > --- a/core/installer.c > +++ b/core/installer.c > @@ -58,6 +58,12 @@ swupdate_file_t check_if_required(struct imglist *list, struct filehdr *pfdh, > if (strcmp(pfdh->filename, img->fname) == 0) { > skip = COPY_FILE; > img->provided = 1; > + if (img->size && img->size != (unsigned int)pfdh->size) { > + ERROR("Size in sw-description %llu does not match size in cpio %u", > + img->size, (unsigned int)pfdh->size); > + return -EINVAL; > + > + } > img->size = (unsigned int)pfdh->size; > > if (snprintf(img->extract_file, > diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst > index d4cb8971b4ed..7d13e1e65d18 100644 > --- a/doc/source/sw-description.rst > +++ b/doc/source/sw-description.rst > @@ -1491,3 +1491,8 @@ There are 4 main sections inside sw-description: > | | | | the mtd to update, instead of | > | | | | specifying the devicenode | > +-------------+----------+------------+---------------------------------------+ > + | size | int64 | images | size of the file as it is expected | > + | | | files | in the SWU. If set and the cpio size | > + | | | scripts | does not match for some reason the | > + | | | | update will fail with an error. | > + +-------------+----------+------------+---------------------------------------+ > diff --git a/include/swupdate.h b/include/swupdate.h > index 7cf421047187..a68c0950dd3f 100644 > --- a/include/swupdate.h > +++ b/include/swupdate.h > @@ -96,7 +96,6 @@ struct swupdate_cfg { > found = 1; \ > img->offset = offs; \ > img->provided = 1; \ > - img->size = fdh.size; \ > } \ > } \ > if (!found) \ > diff --git a/parser/parser.c b/parser/parser.c > index f5113f94841b..52bee8cbf11d 100644 > --- a/parser/parser.c > +++ b/parser/parser.c > @@ -420,6 +420,7 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im > GET_FIELD_STRING(p, elem, "filesystem", image->filesystem); > GET_FIELD_STRING(p, elem, "type", image->type); > GET_FIELD_STRING(p, elem, "data", image->type_data); > + GET_FIELD_INT64(p, elem, "size", &image->size); > get_hash_value(p, elem, image->sha256); > > /* Reviewed-by: Stefano Babic <stefano.babic@swupdate.org> Best regards, Stefano
On 30.10.24 09:29, Dominique Martinet wrote: > Hi Mark, > > Mark Jonas wrote on Wed, Oct 30, 2024 at 09:09:43AM +0100: >>> This allows specifying the size of each cpio file in the sw-description >>> >>> The motivation behind this change is that files are only verified as >>> they are copied to temporary directory or streamed, but without the size >>> information a file could be streamed forever and easily fill in the tmp >>> directory by replacing the files of a valid (signed) SWU. >> >> I like the idea of protecting the temporary folder against flooding by >> buggy or malicious SWU files. In your patch I can see upfront, exact >> file size checks using the != operator. But shouldn't there also be >> somewhere a "larger than" check which prevents exactly the mentioned >> "streamed forever" scenario? > > I'm not sure I understand the question; this patch checks that if the > size is specified in sw-description, and the size from the cpio header > didn't match then we error out immediately instead of streaming/storing > the file; so it doesn't need any "larger than" as anything different > will fail. I tried to imagine the scenario. CPIO's size should always be equal to the "size", the exception is the 4-bytes padding for the CPIO's header. But this is managed in another way, see copyfile(), and the padding does not change the value of the size inside CPIO's header. @Mark: which is the case when the size in the header could be different than the size of the artifact ? > > Is it about "if the size isn't specified in sw-description then we > should still have a upper limit"? IMHO this is a fallback to current status. We haven't yet this check, so it is fine for me that behavior is not changed. If I have read correctly your review, the check is inactive if "size" is not set in sw-description, and this does not break the compatibility with past. > Technically cpio limits each file to 4GB so "forever" isn't quite > possible, but that's still quite a nuisance, so I'll just be setting a > size attribute to all my files in sw-description. The "size" attribute must be then automatically managed by both SWUGenerator and meta-swupdate for sure, and user can forget about it. Best regards, Stefano
Hi Stefano and Dominique, > > Mark Jonas wrote on Wed, Oct 30, 2024 at 09:09:43AM +0100: > >>> This allows specifying the size of each cpio file in the sw-description > >>> > >>> The motivation behind this change is that files are only verified as > >>> they are copied to temporary directory or streamed, but without the size > >>> information a file could be streamed forever and easily fill in the tmp > >>> directory by replacing the files of a valid (signed) SWU. > >> > >> I like the idea of protecting the temporary folder against flooding by > >> buggy or malicious SWU files. In your patch I can see upfront, exact > >> file size checks using the != operator. But shouldn't there also be > >> somewhere a "larger than" check which prevents exactly the mentioned > >> "streamed forever" scenario? > > > > I'm not sure I understand the question; this patch checks that if the > > size is specified in sw-description, and the size from the cpio header > > didn't match then we error out immediately instead of streaming/storing > > the file; so it doesn't need any "larger than" as anything different > > will fail. > > I tried to imagine the scenario. CPIO's size should always be equal to > the "size", the exception is the 4-bytes padding for the CPIO's header. > But this is managed in another way, see copyfile(), and the padding does > not change the value of the size inside CPIO's header. > > @Mark: which is the case when the size in the header could be different > than the size of the artifact ? Maybe I misunderstood the intention of the patch. I was under the impression that the feature should also prevent problems from broken / malicious SWU files where the actual archive is larger than what the CPIO header says. So the CPIO header could say "5 MiB" which would fit nicely into the temp dir. But the file would actually stream maybe 1 GiB and thus eventually fill up the temp directory and potentially create a DoS situation. > > Is it about "if the size isn't specified in sw-description then we > > should still have a upper limit"? In my interpretation of the problem to be solved, yes. This would prevent accepting the SWU file even if header and size attribute match but the file is bigger than what can be stored in the temp dir. > IMHO this is a fallback to current status. We haven't yet this check, so > it is fine for me that behavior is not changed. If I have read correctly > your review, the check is inactive if "size" is not set in > sw-description, and this does not break the compatibility with past. > > > Technically cpio limits each file to 4GB so "forever" isn't quite > > possible, but that's still quite a nuisance, so I'll just be setting a > > size attribute to all my files in sw-description. > > The "size" attribute must be then automatically managed by both > SWUGenerator and meta-swupdate for sure, and user can forget about it. I do not understand how it helps to add the size attribute if it is anyhow expected to be identical with what the CPIO header says. What is exactly the use case where the size attribute would not match the correctly given size of the CPIO archive? Right now I could only imagine a check against broken tooling. Because if it was malicious the CPIO archive header could also give a different size than what the size of the CPIO actually is. Cheers, Mark
Hi Mark, On 30.10.24 18:51, Mark Jonas wrote: > Hi Stefano and Dominique, > >>> Mark Jonas wrote on Wed, Oct 30, 2024 at 09:09:43AM +0100: >>>>> This allows specifying the size of each cpio file in the sw-description >>>>> >>>>> The motivation behind this change is that files are only verified as >>>>> they are copied to temporary directory or streamed, but without the size >>>>> information a file could be streamed forever and easily fill in the tmp >>>>> directory by replacing the files of a valid (signed) SWU. >>>> >>>> I like the idea of protecting the temporary folder against flooding by >>>> buggy or malicious SWU files. In your patch I can see upfront, exact >>>> file size checks using the != operator. But shouldn't there also be >>>> somewhere a "larger than" check which prevents exactly the mentioned >>>> "streamed forever" scenario? >>> >>> I'm not sure I understand the question; this patch checks that if the >>> size is specified in sw-description, and the size from the cpio header >>> didn't match then we error out immediately instead of streaming/storing >>> the file; so it doesn't need any "larger than" as anything different >>> will fail. >> >> I tried to imagine the scenario. CPIO's size should always be equal to >> the "size", the exception is the 4-bytes padding for the CPIO's header. >> But this is managed in another way, see copyfile(), and the padding does >> not change the value of the size inside CPIO's header. >> >> @Mark: which is the case when the size in the header could be different >> than the size of the artifact ? > > Maybe I misunderstood the intention of the patch. I was under the > impression that the feature should also prevent problems from broken / > malicious SWU files where the actual archive is larger than what the > CPIO header says. So the CPIO header could say "5 MiB" which would fit > nicely into the temp dir. But the file would actually stream maybe 1 > GiB and thus eventually fill up the temp directory and potentially > create a DoS situation. This cannot happen. The master here is the size in cpio header. SWUpdate extracts then in tmp so many bytes as read by the CPIO header, and it expects to find another CPIO header after havin doing this. If the artifact in your case is simply replaced with a GB blkob, SWUpdate will still stop after 5MB, and it will parse then an expected CPIO header for the new artifact, causing then an error. An attacker could add then a new CPIO header exactly after 5MB, but then SWUpdate will parse it and if the new artifact was not listed in sw-description, it is simply skipped and nothing is streamed or stored in tmpfs. > >>> Is it about "if the size isn't specified in sw-description then we >>> should still have a upper limit"? > > In my interpretation of the problem to be solved, yes. This would > prevent accepting the SWU file even if header and size attribute match > but the file is bigger than what can be stored in the temp dir. But well, this is job of the integrator. We could then add a global "max-size" for artifacts, but it is dangerous and with side-effects. If later we need to update with larger image, the check will reject a new update. On the other side, signed image allows to trust the "size" value in sw-description. I see that with this patch in place, an attacker cannot replace an artifact by appending a large amount of data and adjusting the size in CPIO header, while the check will avoid this. Currently, SWUpdate will go on to read the stream until there will be at the end an error due to hash mismatch. > >> IMHO this is a fallback to current status. We haven't yet this check, so >> it is fine for me that behavior is not changed. If I have read correctly >> your review, the check is inactive if "size" is not set in >> sw-description, and this does not break the compatibility with past. >> >>> Technically cpio limits each file to 4GB so "forever" isn't quite >>> possible, but that's still quite a nuisance, so I'll just be setting a >>> size attribute to all my files in sw-description. >> >> The "size" attribute must be then automatically managed by both >> SWUGenerator and meta-swupdate for sure, and user can forget about it. > > I do not understand how it helps to add the size attribute if it is > anyhow expected to be identical with what the CPIO header says. > > What is exactly the use case where the size attribute would not match > the correctly given size of the CPIO archive? Right now I could only > imagine a check against broken tooling. Because if it was malicious > the CPIO archive header could also give a different size than what the > size of the CPIO actually is. Let's see if I can explain. Forget errors in tooling. We have a well formed SWU, everything is fine. And we have signed images, so it is not possible to install (if design is correct) a manipulated artifact. The patch is coming from previous discussions, and Dominik's use case is not well designed. So an attacker can take the SWU, split apart, append something to an artifact or replace an artifact (in Dominik's case, this is a tarball, that could be replace with another one). In well designed cases, SWUpdate will note this when the hash is computed and it will raise the error. The update will be considered invalid and not activated, but again some data will be installed and a large blob can substitute our artifact causing DOS. This is mostly due because the CPIO header is outside the trust of chain, and SWUpdate will authenticate it when the handler has installed the whole stream. The introduced check let us to validate the size in cpio header, and exactly the size of artifact can be streamed, nothing more. Best regards, Stefano
Stefano Babic wrote on Wed, Oct 30, 2024 at 07:13:00PM +0100: > > Maybe I misunderstood the intention of the patch. I was under the > > impression that the feature should also prevent problems from broken / > > malicious SWU files where the actual archive is larger than what the > > CPIO header says. So the CPIO header could say "5 MiB" which would fit > > nicely into the temp dir. But the file would actually stream maybe 1 > > GiB and thus eventually fill up the temp directory and potentially > > create a DoS situation. > > This cannot happen. The master here is the size in cpio header. SWUpdate > extracts then in tmp so many bytes as read by the CPIO header, and it > expects to find another CPIO header after havin doing this. If the > artifact in your case is simply replaced with a GB blkob, SWUpdate will > still stop after 5MB, and it will parse then an expected CPIO header for > the new artifact, causing then an error. > > An attacker could add then a new CPIO header exactly after 5MB, but then > SWUpdate will parse it and if the new artifact was not listed in > sw-description, it is simply skipped and nothing is streamed or stored > in tmpfs. Right, the source of truth that we compare to is the content of sw-description, and the second value is the cpio header -- there is no other size taken into account, there cannot be more data than what the cpio header said. Unfortunately this isn't strictly enough: even with this patch, we're not checking the size of sw-description and its .sig (first two files in the cpio archive) before extracting them; so just a dummy cpio with a very large file upfront will still fill $TMPDIR I'd like to cap both of these as well (and technically it's as easy as adding a check in core/stream_interface.c, save_stream, checking fdh.size in the "Copy first two cpio blocks" loop), but I'm honestly not sure what would be considered good values there; I think it'll have to be configurable to make sense and that's why I didn't send a patch yet. Something like 1MB will probably be enough for most case while being small enough to avoid any DoS, but in theory someone could have a sw-description as large as they want by having many artifacts, and one of the encryption rework patch I'm waiting for would have the sw-description be encrypted through cms against as many certificates as there are devices to be installed on so that can also inflate the sw-description size... If it's a setting at least it'll always be possible to split the update in two to update the setting first if it ever becomes possible, but if there's no setting I'm afraid I'll come to regret it. Anyway, with this extra addition, and size attributes properly set (in swupdate SWUGenerator or similar), then I believe it won't be possible to DoS a device's tmp directory anymore. I'll send that second patch either later this week or next week. > > In my interpretation of the problem to be solved, yes. This would > > prevent accepting the SWU file even if header and size attribute match > > but the file is bigger than what can be stored in the temp dir. > > But well, this is job of the integrator. We could then add a global > "max-size" for artifacts, but it is dangerous and with side-effects. If > later we need to update with larger image, the check will reject a new > update. Yes, I'd rather avoid such setting; it could be interpreted as 'max-size-unless-specified-in-sw-description' but I don't see any benefit. Thanks,
Hi Dominique and Stefano, Thank you very much for your explanation. It now makes sense to me. Cheers, Mark On Thu, Oct 31, 2024 at 5:03 AM Dominique Martinet <dominique.martinet@atmark-techno.com> wrote: > > Stefano Babic wrote on Wed, Oct 30, 2024 at 07:13:00PM +0100: > > > Maybe I misunderstood the intention of the patch. I was under the > > > impression that the feature should also prevent problems from broken / > > > malicious SWU files where the actual archive is larger than what the > > > CPIO header says. So the CPIO header could say "5 MiB" which would fit > > > nicely into the temp dir. But the file would actually stream maybe 1 > > > GiB and thus eventually fill up the temp directory and potentially > > > create a DoS situation. > > > > This cannot happen. The master here is the size in cpio header. SWUpdate > > extracts then in tmp so many bytes as read by the CPIO header, and it > > expects to find another CPIO header after havin doing this. If the > > artifact in your case is simply replaced with a GB blkob, SWUpdate will > > still stop after 5MB, and it will parse then an expected CPIO header for > > the new artifact, causing then an error. > > > > An attacker could add then a new CPIO header exactly after 5MB, but then > > SWUpdate will parse it and if the new artifact was not listed in > > sw-description, it is simply skipped and nothing is streamed or stored > > in tmpfs. > > Right, the source of truth that we compare to is the content of > sw-description, and the second value is the cpio header -- there is no > other size taken into account, there cannot be more data than what the > cpio header said. > > Unfortunately this isn't strictly enough: even with this patch, we're > not checking the size of sw-description and its .sig (first two files in > the cpio archive) before extracting them; so just a dummy cpio with a > very large file upfront will still fill $TMPDIR > > I'd like to cap both of these as well (and technically it's as easy as > adding a check in core/stream_interface.c, save_stream, checking > fdh.size in the "Copy first two cpio blocks" loop), but I'm honestly not > sure what would be considered good values there; I think it'll have to > be configurable to make sense and that's why I didn't send a patch yet. > > Something like 1MB will probably be enough for most case while being > small enough to avoid any DoS, but in theory someone could have a > sw-description as large as they want by having many artifacts, and one > of the encryption rework patch I'm waiting for would have the > sw-description be encrypted through cms against as many certificates as > there are devices to be installed on so that can also inflate the > sw-description size... > If it's a setting at least it'll always be possible to split the update > in two to update the setting first if it ever becomes possible, but if > there's no setting I'm afraid I'll come to regret it. > > Anyway, with this extra addition, and size attributes properly set (in > swupdate SWUGenerator or similar), then I believe it won't be possible > to DoS a device's tmp directory anymore. > > I'll send that second patch either later this week or next week. > > > > In my interpretation of the problem to be solved, yes. This would > > > prevent accepting the SWU file even if header and size attribute match > > > but the file is bigger than what can be stored in the temp dir. > > > > But well, this is job of the integrator. We could then add a global > > "max-size" for artifacts, but it is dangerous and with side-effects. If > > later we need to update with larger image, the check will reject a new > > update. > > Yes, I'd rather avoid such setting; it could be interpreted as > 'max-size-unless-specified-in-sw-description' but I don't see any > benefit. > > Thanks, > -- > Dominique
diff --git a/core/cpio_utils.c b/core/cpio_utils.c index 382b22c36434..7f642685b0d4 100644 --- a/core/cpio_utils.c +++ b/core/cpio_utils.c @@ -799,6 +799,12 @@ int cpio_scan(int fd, struct swupdate_cfg *cfg, off_t start) fdh.size, file_listed ? "REQUIRED" : "not required"); + if (img->size && img->size != fdh.size) { + ERROR("%s: size mismatch %llu / %lu", + img->fname, img->size, fdh.size); + return -1; + } + img->size = fdh.size; /* * use copyfile for checksum and hash verification, as we skip file * we do not have to provide fdout diff --git a/core/installer.c b/core/installer.c index 0cb06b2ca419..e3abfd30ddf6 100644 --- a/core/installer.c +++ b/core/installer.c @@ -58,6 +58,12 @@ swupdate_file_t check_if_required(struct imglist *list, struct filehdr *pfdh, if (strcmp(pfdh->filename, img->fname) == 0) { skip = COPY_FILE; img->provided = 1; + if (img->size && img->size != (unsigned int)pfdh->size) { + ERROR("Size in sw-description %llu does not match size in cpio %u", + img->size, (unsigned int)pfdh->size); + return -EINVAL; + + } img->size = (unsigned int)pfdh->size; if (snprintf(img->extract_file, diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst index d4cb8971b4ed..7d13e1e65d18 100644 --- a/doc/source/sw-description.rst +++ b/doc/source/sw-description.rst @@ -1491,3 +1491,8 @@ There are 4 main sections inside sw-description: | | | | the mtd to update, instead of | | | | | specifying the devicenode | +-------------+----------+------------+---------------------------------------+ + | size | int64 | images | size of the file as it is expected | + | | | files | in the SWU. If set and the cpio size | + | | | scripts | does not match for some reason the | + | | | | update will fail with an error. | + +-------------+----------+------------+---------------------------------------+ diff --git a/include/swupdate.h b/include/swupdate.h index 7cf421047187..a68c0950dd3f 100644 --- a/include/swupdate.h +++ b/include/swupdate.h @@ -96,7 +96,6 @@ struct swupdate_cfg { found = 1; \ img->offset = offs; \ img->provided = 1; \ - img->size = fdh.size; \ } \ } \ if (!found) \ diff --git a/parser/parser.c b/parser/parser.c index f5113f94841b..52bee8cbf11d 100644 --- a/parser/parser.c +++ b/parser/parser.c @@ -420,6 +420,7 @@ static int parse_common_attributes(parsertype p, void *elem, struct img_type *im GET_FIELD_STRING(p, elem, "filesystem", image->filesystem); GET_FIELD_STRING(p, elem, "type", image->type); GET_FIELD_STRING(p, elem, "data", image->type_data); + GET_FIELD_INT64(p, elem, "size", &image->size); get_hash_value(p, elem, image->sha256); /*
This allows specifying the size of each cpio file in the sw-description The motivation behind this change is that files are only verified as they are copied to temporary directory or streamed, but without the size information a file could be streamed forever and easily fill in the tmp directory by replacing the files of a valid (signed) SWU. This will be useful even if a chunked checksum is implemented, because while the chunked checksum implicitly also validates the files length it is not useful to include chunked checksums for files that have an intermediate copy stored, and it is more efficient and simpler to only have this size information. Implementation note: SEARCH_FILE (updated in swupdate.h) is only used in cpio_scan, itself never used anywhere in the current code, so this part of the patch has no way of being tested. Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- This is another point I had brought up in the discussions this summer. It is much simpler both conceptually and in the implementation than chunked checksums, so we can probably get this in more easily -- please let me know if you want me to fix something. core/cpio_utils.c | 6 ++++++ core/installer.c | 6 ++++++ doc/source/sw-description.rst | 5 +++++ include/swupdate.h | 1 - parser/parser.c | 1 + 5 files changed, 18 insertions(+), 1 deletion(-)