mbox series

[OpenWrt-Devel,ucert,00/13] ucert fixes and cleanup

Message ID cover.1589663193.git.mschiffer@universe-factory.net
Headers show
Series ucert fixes and cleanup | expand

Message

Matthias Schiffer May 16, 2020, 9:13 p.m. UTC 
While looking for a build issue (see [1]), I noticed various issues in
the ucert code (and this should not be applied before [1] is applied to
usign). There might well be more problems lurking - I did not read all
the code.

In particular patch 12/12 is critical: It must be applied before the
attached libubox patch to avoid a new security issue.

The libubox patch is necessary to make ucert verification work at all
again; without it, cert_load() will always fail, and in consequence, all
images will be found invalid when REQUIRE_IMAGE_SIGNATURE is enabled.


[1] https://patchwork.ozlabs.org/project/openwrt/patch/8ead1fd6a61117b54b4efd5111fe0d19e4eef9c5.1589642591.git.mschiffer@universe-factory.net/

Matthias Schiffer (13):
  stdout/stderr improvements
  Fix return code of write_file()
  Introduce read_file() helper, improve error reporting
  usign-exec: simplify usign execv calls
  usign-exec: fix exec error handling
  usign-exec: do not close stdin and stderr before exec
  usign-exec: change usign_f_* fingerprint argument to char[17]
  usign-exec: remove redundant return statements
  usign-exec: close writing end of pipe early in parent process
  usign-exec: return code fixes
  usign-exec: improve usign -F output handling
  Fix length checks in cert_load()
  Do not print line number in debug messages

 tests/cram/test_ucert.t |   4 +-
 ucert.c                 | 147 +++++++++++++++++++++++-----------------
 usign-exec.c            | 115 +++++++++++++------------------
 usign.h                 |   8 ++-
 4 files changed, 138 insertions(+), 136 deletions(-)

Comments

Daniel Golle May 18, 2020, 9:26 a.m. UTC | #1 
Hi Matthias,

thank you for taking care of that, it has been on my todo list for
a while.

On Sat, May 16, 2020 at 11:13:49PM +0200, Matthias Schiffer wrote:
> While looking for a build issue (see [1]), I noticed various issues in
> the ucert code (and this should not be applied before [1] is applied to
> usign). There might well be more problems lurking - I did not read all
> the code.

Jo-Philipp Wich pointed out to me on IRC a while ago that we would need
to enforce a normalized way to store and read the ucert attributes,
eg. by forcing attributes to appear in alphabetical order and
elminitate other freedoms the parsing in libubox may currently grant.
Right now, those freedoms to store a semantically identical ucert
structure in different ways may allow for brute-forcing a
hash-collission and hence puts more stress on the properties of the
hash algorithm. Hence this apporach is currently not really
cryptographically sound (as also mentioned in usert's README.md).

> 
> In particular patch 12/12 is critical: It must be applied before the
> attached libubox patch to avoid a new security issue.

ucert changes:
Acked-by: Daniel Golle <daniel@makrotopia.org>

> 
> The libubox patch is necessary to make ucert verification work at all
> again; without it, cert_load() will always fail, and in consequence, all
> images will be found invalid when REQUIRE_IMAGE_SIGNATURE is enabled.
> 
> 
> [1] https://patchwork.ozlabs.org/project/openwrt/patch/8ead1fd6a61117b54b4efd5111fe0d19e4eef9c5.1589642591.git.mschiffer@universe-factory.net/
> 
> Matthias Schiffer (13):
>   stdout/stderr improvements
>   Fix return code of write_file()
>   Introduce read_file() helper, improve error reporting
>   usign-exec: simplify usign execv calls
>   usign-exec: fix exec error handling
>   usign-exec: do not close stdin and stderr before exec
>   usign-exec: change usign_f_* fingerprint argument to char[17]
>   usign-exec: remove redundant return statements
>   usign-exec: close writing end of pipe early in parent process
>   usign-exec: return code fixes
>   usign-exec: improve usign -F output handling
>   Fix length checks in cert_load()
>   Do not print line number in debug messages
> 
>  tests/cram/test_ucert.t |   4 +-
>  ucert.c                 | 147 +++++++++++++++++++++++-----------------
>  usign-exec.c            | 115 +++++++++++++------------------
>  usign.h                 |   8 ++-
>  4 files changed, 138 insertions(+), 136 deletions(-)
> 
> -- 
> 2.26.2
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel