1059204 - Prepare verification code for reuse

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Mattias Östergren]

Some modifications of verification routines are required for verifiying manifests of trusted hosted apps. Refactoring on existing routines is done to make it easier to reuse.

Comment 1

[Mattias Östergren]

Attachment: Bug_1016421-0001-SIGNATURE-Extract-VerifySignature-method.patch

Comment 2

[Mattias Östergren]

Attachment: Bug_1016421-0002-SIGNATURE-Extract-InputStream-related-methods.patch

Comment 3

[Vlatko Markovic]

Attachment: Bug_1059204-Prepare-verification-code-for-reuse.patch

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 8480661 [details] [diff] [review]
Bug_1059204-Prepare-verification-code-for-reuse.patch

Review of attachment 8480661 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/apps/AppUtils.cpp
@@ +46,5 @@
> +  static_assert(MAX_LENGTH < UINT32_MAX, "MAX_LENGTH < UINT32_MAX");
> +  if (NS_WARN_IF(MAX_LENGTH < len64)) {
> +    return NS_ERROR_FILE_CORRUPTED;
> +  }
> +  if (NS_WARN_IF(UINT32_MAX < len64)) { // bug 164695

When you flipped the two comparisons with len64 you needed to use "<="

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 8480661 [details] [diff] [review]
Bug_1059204-Prepare-verification-code-for-reuse.patch

Review of attachment 8480661 [details] [diff] [review]:
-----------------------------------------------------------------

r- need a new patch to fix the minor off-by-one, but otherwise looks fine.

::: security/apps/AppUtils.cpp
@@ +46,5 @@
> +  static_assert(MAX_LENGTH < UINT32_MAX, "MAX_LENGTH < UINT32_MAX");
> +  if (NS_WARN_IF(MAX_LENGTH < len64)) {
> +    return NS_ERROR_FILE_CORRUPTED;
> +  }
> +  if (NS_WARN_IF(UINT32_MAX < len64)) { // bug 164695

(because in ReadStream you allocate length+1 and it will exceed your max lengths if they're equal)

Comment 6

[Vlatko Markovic]

Attachment: Bug_1059204-Prepare-verification-code-for-reuse.patch

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 8483443 [details] [diff] [review]
Bug_1059204-Prepare-verification-code-for-reuse.patch

r=dveditz

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8483443 [details] [diff] [review]
Bug_1059204-Prepare-verification-code-for-reuse.patch

Review of attachment 8483443 [details] [diff] [review]:
-----------------------------------------------------------------

Is there an advantage to adding four new files rather than refactoring/adding new code to the existing file?
Also, please make sure the code you add/modify follows the style guidelines at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

::: security/apps/AppUtils.cpp
@@ +11,5 @@
> +#include "seccomon.h"
> +
> +using namespace mozilla;
> +
> +namespace mozilla {

This doesn't need both "using namespace mozilla;" and "namespace mozilla {", right?

@@ +15,5 @@
> +namespace mozilla {
> +
> +nsresult
> +GetStreamLength(const nsCOMPtr<nsIInputStream> &stream,
> +                /*out*/ uint64_t *length)

Every "&" or "*" in arguments should be next to the type, not the name (i.e. move each of these to the left)

@@ +17,5 @@
> +nsresult
> +GetStreamLength(const nsCOMPtr<nsIInputStream> &stream,
> +                /*out*/ uint64_t *length)
> +{
> +  nsresult rv = NS_ERROR_FAILURE;

nit: move this down to where it's actually set for the first time (stream->Available)

@@ +20,5 @@
> +{
> +  nsresult rv = NS_ERROR_FAILURE;
> +
> +  if (!length) {
> +    return rv;

return NS_ERROR_INVALID_ARG

@@ +58,5 @@
> +
> +nsresult
> +ReadStream(const nsCOMPtr<nsIInputStream> &stream,
> +           uint64_t length,
> +           /*out*/ SECItem & buf)

Again, placement of &
Also, please keep lines as close to 80 characters as possible (while not going over - i.e. no unnecessary line-breaks)

::: security/apps/AppUtils.h
@@ +21,5 @@
> +
> +nsresult
> +ReadStream(const nsCOMPtr<nsIInputStream> &stream,
> +           uint64_t length,
> +           /*out*/ SECItem & buf);

Again, placement of "*"/"&" and line lengths

::: security/apps/VerifySignature.cpp
@@ +13,5 @@
> +
> +using namespace mozilla;
> +using namespace mozilla::pkix;
> +
> +namespace mozilla {

Again, if this is already in the namespace mozilla, we don't need the "using namespace mozilla".

Comment 9

[Markus Nilsson]

Attachment: Bug_1059204-Prepare-verification-code-for-reuse-2.patch

Updated version, please review.

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8487928 [details] [diff] [review]
Bug_1059204-Prepare-verification-code-for-reuse-2.patch

Review of attachment 8487928 [details] [diff] [review]:
-----------------------------------------------------------------

I would appreciate an answer to the question I asked in my previous review (top of comment 8) before reviewing this further.

Comment 11

[Zoran Jovanovic]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #10)
> Comment on attachment 8487928 [details] [diff] [review]
> Bug_1059204-Prepare-verification-code-for-reuse-2.patch
> 
> Review of attachment 8487928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would appreciate an answer to the question I asked in my previous review
> (top of comment 8) before reviewing this further.

The intention was to minimize the footprint of code changes in existing files thus making it easier to both apply the patch and integrate/cherry pick on different branches. We thought that it fits architecturally and would help readability and maintainability. In bug 1059216 we add AppManifestVerification.cpp that will use AppUtils and VerifySignature the way that we refactored AppSignatureVerification.cpp to use them.

Would you like these changes localized in AppSignatureVerification.cpp? In bug 1059216 we would then modify AppSignatureVerification.cpp to verify the signature of a manifest of a trusted hosted app...

Comment 12

[Markus Nilsson]

Comment on attachment 8487928 [details] [diff] [review]
Bug_1059204-Prepare-verification-code-for-reuse-2.patch

Zoran answered your question, please review again.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8487928 [details] [diff] [review]
Bug_1059204-Prepare-verification-code-for-reuse-2.patch

Review of attachment 8487928 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for answering my questions. Long story short, I think it would be best if these changes were all in AppSignatureVerification.cpp.

(In reply to Zoran Jovanovic from comment #11)
> The intention was to minimize the footprint of code changes in existing
> files

In doing so, this patch actually makes large changes to AppSignatureVerification.cpp.

> thus making it easier to both apply the patch and integrate/cherry
> pick on different branches. 

I can understand that. Then again, I don't think there's a whole lot of churn on AppSignatureVerification.cpp (other than maybe some namespace/type name changes, which will all have to be changed anyway, making cherry-picking not trivial).

> We thought that it fits architecturally and
> would help readability and maintainability. In bug 1059216 we add
> AppManifestVerification.cpp that will use AppUtils and VerifySignature the
> way that we refactored AppSignatureVerification.cpp to use them.

Well, again, why can't the new code go in this same file? It seems they serve similar functions.

> Would you like these changes localized in AppSignatureVerification.cpp?

I would, yes.

> In
> bug 1059216 we would then modify AppSignatureVerification.cpp to verify the
> signature of a manifest of a trusted hosted app...

I think that's a reasonable approach.

Comment 14

[Zoran Jovanovic]

Attachment: 0001-Bug-1059204-Prepare-verification-code-for-reuse.patch

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8490861 [details] [diff] [review]
0001-Bug-1059204-Prepare-verification-code-for-reuse.patch

Review of attachment 8490861 [details] [diff] [review]:
-----------------------------------------------------------------

Some conerns:
- The preexisting comments are essentially out of date and may not still apply
- GetStreamLength isn't a general-purpose function and has some under-documented limitations (e.g. it'll return a maximum of 1MB for the length, regardless of the length of the stream, which would only be apparent by reading the implementation)

Here's what I would do:
- Factor out ReadStream like you've done, but not GetStreamLength
- Do the appropriate length checks in ReadStream, limit it to 1MB, then add 1 for null-termination (since 1MB is nowhere near the limit of uint32_t, this won't result in anything like bug 164695)
- Allocate the buffer, read the stream, null-terminate it, and return like you've done
- Clean up the comments

::: security/apps/AppSignatureVerification.cpp
@@ +53,5 @@
> +  }
> +  *length = 0;
> +
> +  // The size returned by Available() might be inaccurate so we need to check
> +  // that Available() matches up with the actual length of the file.

I don't see this check anywhere in this function.

@@ +67,5 @@
> +  // someone has specifically crafted the entry to cause OOM or to consume
> +  // massive amounts of disk space.
> +  //
> +  // Also, keep in mind bug 164695 and that we must leave room for
> +  // null-terminating the buffer.

I don't see this implemented anywhere in this function.

@@ +73,5 @@
> +  static_assert(MAX_LENGTH < UINT32_MAX, "MAX_LENGTH < UINT32_MAX");
> +  if (NS_WARN_IF(MAX_LENGTH <= len64)) {
> +    return NS_ERROR_FILE_CORRUPTED;
> +  }
> +  if (NS_WARN_IF(UINT32_MAX <= len64)) { // bug 164695

Haven't we already checked that len64 < UINT32_MAX by the fact that MAX_LENGTH < UINT32_MAX and len64 < MAX_LENGTH?

@@ +83,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +ReadStream(const nsCOMPtr<nsIInputStream>& stream, uint64_t length,

If we're concerned that ReadStream will read the wrong amount of data, it should determine the appropriate amount to read itself, rather than trusting whatever is passed to it.

Comment 16

[Zoran Jovanovic]

Attachment: 0001-Bug-1059204-Prepare-verification-code-for-reuse.patch

Comment 17

[Richard Barnes [:rbarnes]]

Comment on attachment 8491733 [details] [diff] [review]
0001-Bug-1059204-Prepare-verification-code-for-reuse.patch

Review of attachment 8491733 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these fixed

::: security/apps/AppSignatureVerification.cpp
@@ +54,5 @@
> +// @param stream  The input stream to read from.
> +// @param buf     The buffer that we read the stream into, which must have
> +//                already been allocated.
> +nsresult
> +ReadStream(const nsCOMPtr<nsIInputStream>& stream, /*out*/ SECItem& buf)

It's not clear why you didn't just copy and paste the other code.  Please at least add back the comments that got lost in translation.  The one referencing bug 164695 especially

@@ +76,5 @@
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +  if (bytesRead != length) {
> +    return NS_ERROR_FAILURE;

It would be better to return the more expressive error here (as before)

Comment 18

[Zoran Jovanovic]

Attachment: 0001-Bug-1059204-Prepare-verification-code-for-reuse.patch

r+ from rbarnes

Comment 19

[[:fabrice] Fabrice Desré]

try push:
https://tbpl.mozilla.org/?tree=Try&rev=260408e62753

Comment 20

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/9a3e34d36cc1

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a3e34d36cc1

Comment 22

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c61d4add4ba5

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8492191 [details] [diff] [review]
0001-Bug-1059204-Prepare-verification-code-for-reuse.patch

got a=bajaj over email