| Message ID | 20241031055741.91045-3-dominique.martinet@atmark-techno.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] cpio_util: remove unused cpio_scan | expand |
Dominique Martinet wrote on Thu, Oct 31, 2024 at 02:57:41PM +0900: > artifacts in cpio after sw-description has been parsed can have their size > checked with the new 'size' attribute, but sw-description itself needs another > way of limiting the file size to protect tmpdir. > > The default does not limit anything, and users who want to set a limit can set > it manually > > Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> > --- > So this appears to work and the default isn't changed, what do you > think? > It's a bit more controversial than the first patch, so happy to discuss > this some more. Gentle ping on this. Happy if it doesn't get in in this form or if it takes a bit more time to review, but we're updating swupdate at the end of the month[1] (in a bit more than a week) and I'd like to avoid diverging from upstream more than required so would be great to at least fix the config item so I don't end up renaming it later. I think a config item is required because someone somewhere will try to do something unreasonable and require more, but having a non-zero default value could be something to consider in my opinion; I've just taken the steadier approach of defaulting to not changing the behaviour in the first place. [1] that internal update will have all the work I've done recently e.g. chunked sha256, size attribute and this; we'll rotate our signing key at the same time and older SWUs will be rendered non-installable (easily) to avoid abusing old releases. The tree is always visible here: https://github.com/atmark-techno/swupdate/commits/next/ Cheers,
Hi Dominique, On 14.11.24 01:06, Dominique Martinet wrote: > Dominique Martinet wrote on Thu, Oct 31, 2024 at 02:57:41PM +0900: >> artifacts in cpio after sw-description has been parsed can have their size >> checked with the new 'size' attribute, but sw-description itself needs another >> way of limiting the file size to protect tmpdir. >> >> The default does not limit anything, and users who want to set a limit can set >> it manually >> >> Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> >> --- >> So this appears to work and the default isn't changed, what do you >> think? >> It's a bit more controversial than the first patch, so happy to discuss >> this some more. > > Gentle ping on this. > Happy if it doesn't get in in this form or if it takes a bit more time > to review, but we're updating swupdate at the end of the month[1] (in a bit > more than a week) and I'd like to avoid diverging from upstream more > than required so would be great to at least fix the config item so I > don't end up renaming it later I applied yesterday, but coverity had still to run. I merged my test branch today and I pushed a couple of minutes ago. . > > I think a config item is required because someone somewhere will try to > do something unreasonable and require more, but having a non-zero > default value could be something to consider in my opinion; I've just > taken the steadier approach of defaulting to not changing the behaviour > in the first place. Yes, and I agree with this to avoid any breakage. Best regards, Stefano > > > > [1] that internal update will have all the work I've done recently > e.g. chunked sha256, size attribute and this; we'll rotate our signing > key at the same time and older SWUs will be rendered non-installable > (easily) to avoid abusing old releases. > > The tree is always visible here: > https://github.com/atmark-techno/swupdate/commits/next/ > > > Cheers,
Stefano Babic wrote on Thu, Nov 14, 2024 at 03:27:50PM +0100: > > Happy if it doesn't get in in this form or if it takes a bit more time > > to review, but we're updating swupdate at the end of the month[1] (in a bit > > more than a week) and I'd like to avoid diverging from upstream more > > than required so would be great to at least fix the config item so I > > don't end up renaming it later > > I applied yesterday, but coverity had still to run. I merged my test > branch today and I pushed a couple of minutes ago. Thank you! Great timing, as usual :)
diff --git a/core/stream_interface.c b/core/stream_interface.c index d84f339ebf5b..bb2e9eddcb92 100644 --- a/core/stream_interface.c +++ b/core/stream_interface.c @@ -73,7 +73,8 @@ pthread_cond_t stream_cond = PTHREAD_COND_INITIALIZER; static struct installer inst; -static int extract_file_to_tmp(int fd, const char *fname, unsigned long *poffs, bool encrypted) +static int extract_file_to_tmp(int fd, const char *fname, unsigned long *poffs, + bool encrypted, int max_size) { char output_file[MAX_IMAGE_FNAME]; struct filehdr fdh; @@ -95,6 +96,11 @@ static int extract_file_to_tmp(int fd, const char *fname, unsigned long *poffs, ERROR("Path too long: %s%s", TMPDIR, fdh.filename); return -1; } + if (max_size && fdh.size >= max_size) { + ERROR("%s size (%ld) exceeds configured max of %d, aborting", + fdh.filename, fdh.size, max_size); + return -1; + } TRACE("Found file"); TRACE("\tfilename %s", fdh.filename); TRACE("\tsize %u", (unsigned int)fdh.size); @@ -172,7 +178,8 @@ static int extract_files(int fd, struct swupdate_cfg *software) switch (status) { /* Waiting for the first Header */ case STREAM_WAIT_DESCRIPTION: - if (extract_file_to_tmp(fd, SW_DESCRIPTION_FILENAME, &offset, encrypted_sw_desc) < 0 ) + if (extract_file_to_tmp(fd, SW_DESCRIPTION_FILENAME, &offset, + encrypted_sw_desc, software->swdesc_max_size) < 0 ) return -1; status = STREAM_WAIT_SIGNATURE; @@ -181,7 +188,8 @@ static int extract_files(int fd, struct swupdate_cfg *software) case STREAM_WAIT_SIGNATURE: #ifdef CONFIG_SIGNED_IMAGES snprintf(output_file, sizeof(output_file), "%s.sig", SW_DESCRIPTION_FILENAME); - if (extract_file_to_tmp(fd, output_file, &offset, false) < 0 ) + if (extract_file_to_tmp(fd, output_file, &offset, false, + software->swdesc_max_size) < 0 ) return -1; #endif snprintf(output_file, sizeof(output_file), "%s%s", TMPDIR, SW_DESCRIPTION_FILENAME); @@ -461,6 +469,17 @@ static int save_stream(int fdin, struct swupdate_cfg *software) tmpsize += NPAD_BYTES(tmpsize); tmpsize -= sizeof(struct new_ascii_header); + /* + * This doubles with check in extract_file_to_tmp, but it does not make + * sense to check after having copied everything once in tmpfd... + */ + if (software->swdesc_max_size && fdh.size >= software->swdesc_max_size) { + ERROR("sw-description size (%ld) exceeds configured max of %d, aborting", + fdh.size, software->swdesc_max_size); + ret = -EINVAL; + goto no_copy_output; + } + /* * copy the remaining bytes to have a complete cpio block */ @@ -473,14 +492,16 @@ static int save_stream(int fdin, struct swupdate_cfg *software) lseek(tmpfd, 0, SEEK_SET); offset = 0; - if (extract_file_to_tmp(tmpfd, SW_DESCRIPTION_FILENAME, &offset, encrypted_sw_desc) < 0) { + if (extract_file_to_tmp(tmpfd, SW_DESCRIPTION_FILENAME, &offset, + encrypted_sw_desc, software->swdesc_max_size) < 0) { ERROR("%s cannot be extracted", SW_DESCRIPTION_FILENAME); ret = -EINVAL; goto no_copy_output; } #ifdef CONFIG_SIGNED_IMAGES snprintf(output_file, sizeof(output_file), "%s.sig", SW_DESCRIPTION_FILENAME); - if (extract_file_to_tmp(tmpfd, output_file, &offset, false) < 0 ) { + if (extract_file_to_tmp(tmpfd, output_file, &offset, false, + software->swdesc_max_size) < 0 ) { ERROR("Signature cannot be extracted:%s", output_file); ret = -EINVAL; goto no_copy_output; diff --git a/core/swupdate.c b/core/swupdate.c index 4b98add938c9..aec2cfe399f1 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -362,14 +362,16 @@ static int read_globals_settings(void *elem, void *data) char software_select[SWUPDATE_GENERAL_STRING_SIZE] = ""; GET_FIELD_STRING(LIBCFG_PARSER, elem, "select", software_select); - GET_FIELD_STRING(LIBCFG_PARSER, elem, - "gpg-home-dir", sw->gpg_home_directory); - GET_FIELD_STRING(LIBCFG_PARSER, elem, - "gpgme-protocol", sw->gpgme_protocol); if (software_select[0] != '\0') { /* by convention, errors in a configuration section are ignored */ (void)parse_image_selector(software_select, sw); } + GET_FIELD_STRING(LIBCFG_PARSER, elem, + "gpg-home-dir", sw->gpg_home_directory); + GET_FIELD_STRING(LIBCFG_PARSER, elem, + "gpgme-protocol", sw->gpgme_protocol); + GET_FIELD_INT(LIBCFG_PARSER, elem, "sw-description-max-size", + &sw->swdesc_max_size); return 0; } diff --git a/include/swupdate.h b/include/swupdate.h index 6a60d357d2fd..e825e07d9e13 100644 --- a/include/swupdate.h +++ b/include/swupdate.h @@ -86,6 +86,7 @@ struct swupdate_cfg { const char *embscript; char gpg_home_directory[SWUPDATE_GENERAL_STRING_SIZE]; char gpgme_protocol[SWUPDATE_GENERAL_STRING_SIZE]; + int swdesc_max_size; }; struct swupdate_cfg *get_swupdate_cfg(void);
artifacts in cpio after sw-description has been parsed can have their size checked with the new 'size' attribute, but sw-description itself needs another way of limiting the file size to protect tmpdir. The default does not limit anything, and users who want to set a limit can set it manually Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com> --- So this appears to work and the default isn't changed, what do you think? It's a bit more controversial than the first patch, so happy to discuss this some more. Note in my test I've never gone through save_stream(), unlike cpio_scan it isn't technically dead in that the function can be called but e.g. disable_store_swu is never set anywhere so I'm not sure in what case exactly it's used (that variable itself isn't a problem, it's software->output that is empty in my case... ah, sw-description encryption uses it? I don't use it but will give it a test later.) (the unrelated gpg-home-dir etc variable parsing move is just that the software_select[0] check was out of place and bugged me, happy to remove it) core/stream_interface.c | 31 ++++++++++++++++++++++++++----- core/swupdate.c | 10 ++++++---- include/swupdate.h | 1 + 3 files changed, 33 insertions(+), 9 deletions(-)