when i looked through things, it seemed that PEM was the format we were using everywhere, not DER.  do we have a doc that says one over the other somewhere ?  if not, seems like we should change imageloader to normalize on PEM.

The SignatureVerifier class in Chrome takes a "DER encoded ASN.1 SubjectPublicKeyInfo structure"
(https://cs.chromium.org/chromium/src/crypto/signature_verifier.h?q=crypto::SignatureVerifier+package:%5Echromium$&l=23), so the key should already be in the format expected by the SignatureVerifier and not converted.

IIRC, PEM is essentially base64-encoded DER ;-)

Given that the key is coming from a trusted location, I wouldn't be too worried about parsing bugs - as long as attackers can't inject a malicious PEM key, we don't have a security risk in parsing.

On the other hand, if its more convenient to consume DER, then there's no reason why we shouldn't do it. Both formats are used widely in various software, including Chrome (OS) code.

It might be unreasonable prejudice of mine, but I really prefer to avoid any direct calls to OpenSSL APIs in the code. 

Looking at  crbug.com/717030  where a trivially easy to make mistake in the usage of the API, which is not documented in the man page, caused imageloader to experience memory corruption and crash repeatedly on launch. That could have potentially been exploited.

How would this be exploitable if you crash immediately after launch at a point where you haven't consumed attacker-controlled data yet?

Sorry, I meant that generally speaking memory corruption bugs can be exploitable, and given a history of problems with some of the openssl APIs, I prefer to avoid using it to convert formats when possible. It's quicker, easier, safer, etc. 

Keeping things simple is definitely a good motivation. Given that we've established that chances are very low this is exploitable, should we turn this bug into a regular one and drop the security labels?

i guess now would be the time to make the change before we have to worry about compatibility :)

opening this up as a normal bug makes sense to me

This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Downgrading this bug to a regular one, no need to hold the beta release over this.

I was probably paranoid in making this a security bug, but I would really appreciate if we could fix this before we actually ship anything.

that's fine

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/imageloader/+/a290471be8063ef1a1173d891e70ee6607d129ac

commit a290471be8063ef1a1173d891e70ee6607d129ac
Author: Mike Frysinger <vapier@chromium.org>
Date: Sun May 07 02:52:32 2017

load container key from DER file

Since we're inserting the key into the rootfs in the DER format,
we can read it directly and avoid doing any parsing on it.

BUG= chromium:718184 
TEST=mount_extension_image still works

Change-Id: If3b6ce915cfda58e5cddba113445324ec90bde5c
Reviewed-on: https://chromium-review.googlesource.com/497948
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/a290471be8063ef1a1173d891e70ee6607d129ac/imageloader_main.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/1aabe7111e6e71d48bb3d684880489f14b39b17e

commit 1aabe7111e6e71d48bb3d684880489f14b39b17e
Author: Mike Frysinger <vapier@chromium.org>
Date: Wed May 10 18:58:19 2017

image_signing: output pubkey in DER format

BRANCH=None
BUG= chromium:718184 
TEST=new imageloader works

Change-Id: I430ed616954c820d3d1607eefd4f8e1c60863a8f
Reviewed-on: https://chromium-review.googlesource.com/497914
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>

[modify] https://crrev.com/1aabe7111e6e71d48bb3d684880489f14b39b17e/scripts/image_signing/insert_container_publickey.sh

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/b660356d515473a81e424410ead2cdf566159406

commit b660356d515473a81e424410ead2cdf566159406
Author: Mike Frysinger <vapier@chromium.org>
Date: Tue May 16 17:43:14 2017

image_signing: fix key insert logic

We don't want to override the common trap as the common sh files already
have handlers installed to clean up files/mounts.  Re-use those helpers
to avoid leaking loopback mounts.

BRANCH=None
BUG= chromium:718184 
TEST=signing images still works

Change-Id: I749ce5075194356219fea51152154fdc5a2e3b99
Reviewed-on: https://chromium-review.googlesource.com/505575
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Commit-Queue: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/b660356d515473a81e424410ead2cdf566159406/scripts/image_signing/insert_container_publickey.sh

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/cros-signing/+/d63d7a40b8b0ec8e6614c6c9b82ceb4596259f03

commit d63d7a40b8b0ec8e6614c6c9b82ceb4596259f03
Author: Mike Frysinger <vapier@chromium.org>
Date: Tue May 16 19:12:20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/9d11bb1b1d0ca7503b195c1de4463f1d1e1ab4d2

commit 9d11bb1b1d0ca7503b195c1de4463f1d1e1ab4d2
Author: Mike Frysinger <vapier@chromium.org>
Date: Fri May 19 04:13:09 2017

image_signing: unify output helpers

We have `err_die` and `die` helpers that do the same thing, but some
scripts just have to know which one to use based on their runtime.
Just unify them as the more common `die` so all scripts can use it.

Similarly, we provide info, warn, and error to dev scripts, but not
to the runtime ones.  Add small stubs in common_minimal.sh so the
API is consistent.

BRANCH=None
BUG= chromium:718184 
TEST=scripts still work

Change-Id: Id44fb27900c37f4e357d20817f909e4534d1c5b3
Reviewed-on: https://chromium-review.googlesource.com/507990
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: David Riley <davidriley@chromium.org>
Commit-Queue: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/9d11bb1b1d0ca7503b195c1de4463f1d1e1ab4d2/scripts/image_signing/tofactory.sh
[modify] https://crrev.com/9d11bb1b1d0ca7503b195c1de4463f1d1e1ab4d2/scripts/image_signing/common.sh
[modify] https://crrev.com/9d11bb1b1d0ca7503b195c1de4463f1d1e1ab4d2/scripts/image_signing/make_dev_firmware.sh
[modify] https://crrev.com/9d11bb1b1d0ca7503b195c1de4463f1d1e1ab4d2/scripts/image_signing/common_minimal.sh
[modify] https://crrev.com/9d11bb1b1d0ca7503b195c1de4463f1d1e1ab4d2/scripts/image_signing/sign_firmware.sh
[modify] https://crrev.com/9d11bb1b1d0ca7503b195c1de4463f1d1e1ab4d2/scripts/image_signing/make_dev_ssd.sh