| Message ID | 20200824105300.392536-1-baptiste@bitsofnetworks.org |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [opkg] libopkg: move file size check after checksum verification | expand |
On 24/08/2020 07:53, Baptiste Jonglez wrote: > The file size check was added in cb6640381808dd ("libopkg: check for file > size mismatches"). Its purpose is to provide an additional line of > defense against hash collisions. > > It is more user-friendly to tell the user that the checksum is wrong, so > move the file size check at the end. It is also far more expensive in the failure case, not to mention the fact that you're going to process data you KNOW to be wrong when you could have easily avoided it. This does NOT look like a good idea to me.
On 24-08-20, Henrique de Moraes Holschuh wrote: > On 24/08/2020 07:53, Baptiste Jonglez wrote: > > The file size check was added in cb6640381808dd ("libopkg: check for file > > size mismatches"). Its purpose is to provide an additional line of > > defense against hash collisions. > > > > It is more user-friendly to tell the user that the checksum is wrong, so > > move the file size check at the end. > > It is also far more expensive in the failure case, not to mention the fact > that you're going to process data you KNOW to be wrong when you could have > easily avoided it. I agree, this leads to unnecessary processing in the failure case, i.e. when the size & checksum are wrong. However, this failure case is rather unexpected, and I doubt that a slightly higher processing time (if it is even measurable) is an issue when you are dealing with corrupted packages. In the general case where the checksum and size are right, this patch does not change the processing cost. Baptiste
On 24/08/2020 09:01, Baptiste Jonglez wrote: > On 24-08-20, Henrique de Moraes Holschuh wrote: >> On 24/08/2020 07:53, Baptiste Jonglez wrote: >>> It is more user-friendly to tell the user that the checksum is wrong, so >>> move the file size check at the end. >> >> It is also far more expensive in the failure case, not to mention the fact >> that you're going to process data you KNOW to be wrong when you could have >> easily avoided it. > > I agree, this leads to unnecessary processing in the failure case, > i.e. when the size & checksum are wrong. > > However, this failure case is rather unexpected, and I doubt that a > slightly higher processing time (if it is even measurable) is an issue > when you are dealing with corrupted packages. Give it an ultra-large sparse file as input, on purpose (attacker) or due to corrupted filesystem with weird inode data. Suddenly, you would be much happier had you checked the file size first (i.e. without this change)... IMHO, your proposed change should be backed by a strong reason that offsets the increased risk. Again IMHO, the commit log doesn't currently provide a strong enough reason. But that's IMO. Let's wait for other opinions (or you could provide a stronger reason to apply this change, perhaps?)
On 24-08-20, Henrique de Moraes Holschuh wrote: > On 24/08/2020 09:01, Baptiste Jonglez wrote: > > On 24-08-20, Henrique de Moraes Holschuh wrote: > > > On 24/08/2020 07:53, Baptiste Jonglez wrote: > > > > It is more user-friendly to tell the user that the checksum is wrong, so > > > > move the file size check at the end. > > > > > > It is also far more expensive in the failure case, not to mention the fact > > > that you're going to process data you KNOW to be wrong when you could have > > > easily avoided it. > > > > I agree, this leads to unnecessary processing in the failure case, > > i.e. when the size & checksum are wrong. > > > > However, this failure case is rather unexpected, and I doubt that a > > slightly higher processing time (if it is even measurable) is an issue > > when you are dealing with corrupted packages. > > Give it an ultra-large sparse file as input, on purpose (attacker) or due to > corrupted filesystem with weird inode data. Suddenly, you would be much > happier had you checked the file size first (i.e. without this change)... Ok, I can see how it could go wrong. Note that the size check is quite recent (January 2019). Before this size check was introduced, the situation you describe could already happen. > IMHO, your proposed change should be backed by a strong reason that offsets > the increased risk. Again IMHO, the commit log doesn't currently provide a > strong enough reason. > > But that's IMO. Let's wait for other opinions (or you could provide a > stronger reason to apply this change, perhaps?) No, it's really just cosmetic, the change was based on a private discussion. In the end I don't mind one way of the other, and your argument makes sense regarding the trade-off. Please disregard this patch, and I will send a v2 of the other patch so that it doesn't depend on this one. Baptiste
diff --git a/libopkg/opkg_install.c b/libopkg/opkg_install.c index 27c9484..183a1dc 100644 --- a/libopkg/opkg_install.c +++ b/libopkg/opkg_install.c @@ -1367,30 +1367,6 @@ int opkg_install_pkg(pkg_t * pkg, int from_upgrade) } #endif - /* Check file size */ - err = lstat(local_filename, &pkg_stat); - - if (err) { - opkg_msg(ERROR, "Failed to stat %s: %s\n", - local_filename, strerror(errno)); - return -1; - } - - pkg_expected_size = pkg_get_int(pkg, PKG_SIZE); - - if (pkg_expected_size > 0 && pkg_stat.st_size != pkg_expected_size) { - if (!conf->force_checksum) { - opkg_msg(ERROR, - "Package size mismatch: %s is %lld bytes, expecting %lld bytes\n", - pkg->name, (long long int)pkg_stat.st_size, pkg_expected_size); - return -1; - } else { - opkg_msg(NOTICE, - "Ignored %s size mismatch.\n", - pkg->name); - } - } - /* Check for md5 values */ pkg_md5 = pkg_get_md5(pkg); if (pkg_md5) { @@ -1434,6 +1410,30 @@ int opkg_install_pkg(pkg_t * pkg, int from_upgrade) free(file_sha256); } + /* Check file size */ + err = lstat(local_filename, &pkg_stat); + + if (err) { + opkg_msg(ERROR, "Failed to stat %s: %s\n", + local_filename, strerror(errno)); + return -1; + } + + pkg_expected_size = pkg_get_int(pkg, PKG_SIZE); + + if (pkg_expected_size > 0 && pkg_stat.st_size != pkg_expected_size) { + if (!conf->force_checksum) { + opkg_msg(ERROR, + "Package size mismatch: %s is %lld bytes, expecting %lld bytes\n", + pkg->name, (long long int)pkg_stat.st_size, pkg_expected_size); + return -1; + } else { + opkg_msg(NOTICE, + "Ignored %s size mismatch.\n", + pkg->name); + } + } + if (conf->download_only) { if (conf->nodeps == 0) { err = satisfy_dependencies_for(pkg);