[PATCH] libopkg/pkg.c: Check downloaded file size in pkg_verify()

[Haris Okanovic]

Hash algorithms (particularly weak ones like md5) are susceptible to
hash collisions -- a condition where two files/strings results in the
same hash. This can be exploited, e.g. to defeat digital signatures, by
padding malicious files with garbage data to coerce an identical hash
to that of legitimately signed files. Such exploits are significantly
harder when file size cannot change.

This change adds a file size constraint to pkg_verify() before checking
hashes: Downloaded IPKs must have the same file size that's encoded in
feed index (Packages file).

This is particularly useful when using opkg with signed feeds, I.e.
feeds with signed Packages files of otherwise unsigned IPKs. It's much
harder to defeat package hashes of a signed feed.

Signed-off-by: Haris Okanovic <haris.o...@ni.com>
---
libopkg/pkg.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/libopkg/pkg.c b/libopkg/pkg.c
index ca5ff08..daa7f94 100644
--- a/libopkg/pkg.c
+++ b/libopkg/pkg.c
@@ -1504,12 +1504,32 @@ int pkg_write_changed_filelists(void)
int pkg_verify(pkg_t * pkg)
{
int err;
+ struct stat pkg_stat;
char *local_sig_filename = NULL;

- /* Exit if the package doesn't exist locally as the caller may be about to
- * download it. */
- if (!file_exists(pkg->local_filename))
- return 1;
+ err = xlstat(pkg->local_filename, &pkg_stat);
+ if (err) {
+ if (errno == ENOENT) {
+ /* Exit with soft error 1 if the package doesn't exist.
+ * This allows the caller to download it without nasty
+ * messages in the error log.
+ */
+ return 1;
+ }
+ else {
+ opkg_msg(ERROR, "Failed to stat %s: %s\n",
+ pkg->local_filename, strerror(errno));
+ goto fail;
+ }
+ }
+
+ /* Check size to mitigate hash collisions. */
+ if (pkg_stat.st_size < 1 || pkg_stat.st_size != pkg->size) {
+ err = -1;
+ opkg_msg(ERROR, "File size mismatch: %s is %lld bytes, expecting %lu bytes\n",
+ pkg->local_filename, (long long int)pkg_stat.st_size, pkg->size);
+ goto fail;
+ }

#ifdef HAVE_SHA256
if (pkg->sha256sum) {
--
2.20.0

[Alejandro Del Castillo]

Thanks, I like this approach a whole lot more!. Now opkg is insulated
from gpg changing structure.

merged

Cheers,

Alejandro

[Alejandro Del Castillo]

apologies, my message was meant for your other patchset.

This patch looks good, but the ATS needs to change since its not
currently storing Size (make check fails).

Could you change the test\opk.py to include Size?

On 1/30/19 4:32 PM, Haris Okanovic wrote:

Cheers,

Alejandro

[Haris Okanovic]

On 2/4/19 10:41 AM, Alejandro Del Castillo wrote:
> apologies, my message was meant for your other patchset.
>
> This patch looks good, but the ATS needs to change since its not
> currently storing Size (make check fails).
>
> Could you change the test\opk.py to include Size?

I'll submit a V2 shortly. I'm having trouble building opkg on my Arch
desktop, so I was unable to run `make check`. I tested everything in OE.

-- Haris

[Haris Okanovic]

Hash algorithms (particularly weak ones like md5) are susceptible to
hash collisions -- a condition where two files/strings results in the
same hash. This can be exploited, e.g. to defeat digital signatures, by
padding malicious files with garbage data to coerce an identical hash
to that of legitimately signed files. Such exploits are significantly
harder when file size cannot change.

This change adds a file size constraint to pkg_verify() before checking
hashes: Downloaded IPKs must have the same file size that's encoded in
feed index (Packages file).

This is particularly useful when using opkg with signed feeds, I.e.

feeds with signed Packages files of otherwise unsigned IPKs. It makes
it harder to defeat the enclosed hashes.

Signed-off-by: Haris Okanovic <haris.o...@ni.com>
---
libopkg/pkg.c | 28 ++++++++++++++++++++++++----

tests/opk.py | 2 ++
2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/tests/opk.py b/tests/opk.py
index 63c3e00..c818f1c 100644
--- a/tests/opk.py
+++ b/tests/opk.py
@@ -142,8 +142,10 @@ class OpkGroup:
f.write("{}: {}\n".format(k, opk.control[k]))
fpattern = "{Package}_{Version}_{Architecture}.opk"
fname = fpattern.format(**opk.control)
+ fsize = os.stat(fname).st_size
md5sum = md5sum_file(fname)
f.write("Filename: {}\n".format(fname))
+ f.write("Size: {}\n".format(fsize))
f.write("MD5Sum: {}\n".format(md5sum))
f.write("\n")
f.close()
--
2.20.0

[Haris Okanovic]

Running `DEBUG_OPKG_CMDS=1 make check` prints every opkg cmdline and
it's resulting stdout/stderr during tests. This change makes it easier
to debug problems during tests. Seeing opkg's stderr on failures is
particularly useful.

Signed-off-by: Haris Okanovic <haris.o...@ni.com>
---

tests/opkgcl.py | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/tests/opkgcl.py b/tests/opkgcl.py
index 8098ee1..3df2a32 100755
--- a/tests/opkgcl.py
+++ b/tests/opkgcl.py
@@ -3,12 +3,18 @@
import os, subprocess
import cfg
vardir=os.environ['VARDIR']
+debug_opkg_cmds=bool(os.environ.get("DEBUG_OPKG_CMDS") or False)

def opkgcl(opkg_args):
cmd = "{} -o {} {}".format(cfg.opkgcl, cfg.offline_root, opkg_args)
+ if debug_opkg_cmds:
+ print("DEBUG cmd: {}".format(cmd))
p = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE,
stderr=subprocess.STDOUT)
(stdout_data, stderr_data) = p.communicate()
+ if debug_opkg_cmds:
+ print("DEBUG stdout: {}".format(stdout_data.decode("utf-8") if stdout_data else None))
+ print("DEBUG stderr: {}".format(stderr_data.decode("utf-8") if stderr_data else None))
status = p.returncode
return (status, stdout_data.decode("utf-8"))

--
2.20.0

[Haris Okanovic]

Hash algorithms (particularly weak ones like md5) are susceptible to
hash collisions -- a condition where two files/strings results in the
same hash. This can be exploited, e.g. to defeat digital signatures, by
padding malicious files with garbage data to coerce an identical hash
to that of legitimately signed files. Such exploits are significantly
harder when file size cannot change.

This change adds a file size constraint to pkg_verify() before checking
hashes: Downloaded IPKs must have the same file size that's encoded in
feed index (Packages file).

This is particularly useful when using opkg with signed feeds, I.e.

feeds with signed Packages files of otherwise unsigned IPKs. It makes
it harder to defeat the enclosed hashes.

Signed-off-by: Haris Okanovic <haris.o...@ni.com>
---

[PATCH v2]
* Fix tests/opk.py: add "Size" attribute to index

[PATCH v3]
* Use stat() instead of lstat() to accommodate unit tests which use
symlink farm to implement feeds

---
libopkg/pkg.c | 28 ++++++++++++++++++++++++----
tests/opk.py | 2 ++

2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/libopkg/pkg.c b/libopkg/pkg.c
index ca5ff08..450dc39 100644

--- a/libopkg/pkg.c
+++ b/libopkg/pkg.c
@@ -1504,12 +1504,32 @@ int pkg_write_changed_filelists(void)
int pkg_verify(pkg_t * pkg)
{
int err;
+ struct stat pkg_stat;
char *local_sig_filename = NULL;

- /* Exit if the package doesn't exist locally as the caller may be about to
- * download it. */
- if (!file_exists(pkg->local_filename))
- return 1;

+ err = stat(pkg->local_filename, &pkg_stat);

+ if (err) {
+ if (errno == ENOENT) {
+ /* Exit with soft error 1 if the package doesn't exist.
+ * This allows the caller to download it without nasty
+ * messages in the error log.
+ */
+ return 1;
+ }
+ else {
+ opkg_msg(ERROR, "Failed to stat %s: %s\n",
+ pkg->local_filename, strerror(errno));
+ goto fail;
+ }
+ }
+
+ /* Check size to mitigate hash collisions. */
+ if (pkg_stat.st_size < 1 || pkg_stat.st_size != pkg->size) {
+ err = -1;
+ opkg_msg(ERROR, "File size mismatch: %s is %lld bytes, expecting %lu bytes\n",
+ pkg->local_filename, (long long int)pkg_stat.st_size, pkg->size);
+ goto fail;
+ }

#ifdef HAVE_SHA256
if (pkg->sha256sum) {

[Haris Okanovic]

Thanks for all your help in getting my build issues resolved. I made
another small fixup and posted a V3. `make check` passes.

-- Haris

[Alejandro Del Castillo]

merged

Cheers,

Alejandro

[Alejandro Del Castillo]

merged

On 2/4/19 3:40 PM, Haris Okanovic wrote:

Cheers,

Alejandro