Closed Bug 1059204 Opened 5 years ago Closed 5 years ago

Prepare verification code for reuse

Categories

(Core Graveyard :: DOM: Apps, defect)

x86_64
Linux
defect
Not set

Tracking

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S5 (26sep)
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: mattias.ostergren, Assigned: vlatko.markovic)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
Assignee: nobody → vlatko.markovic
Blocks: 1016421
Whiteboard: [2.1-feature-qa+]
Target Milestone: --- → 2.1 S4 (12sep)
Attachment #8479812 - Flags: review?(fabrice)
Attachment #8479815 - Flags: review?(fabrice)
Attachment #8479812 - Flags: review?(fabrice) → review?(dveditz)
Attachment #8479815 - Flags: review?(fabrice) → review?(dveditz)
Whiteboard: [2.1-feature-qa+]
Attachment #8480661 - Flags: review?(dveditz)
Attachment #8479812 - Attachment is obsolete: true
Attachment #8479812 - Flags: review?(dveditz)
Attachment #8479815 - Attachment is obsolete: true
Attachment #8479815 - Flags: review?(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 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)
Attachment #8480661 - Flags: review?(dveditz) → review-
Attachment #8480661 - Attachment is obsolete: true
Attachment #8483443 - Flags: review?(dveditz)
Comment on attachment 8483443 [details] [diff] [review]
Bug_1059204-Prepare-verification-code-for-reuse.patch

r=dveditz
Attachment #8483443 - Flags: review?(dveditz) → review+
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".
Attachment #8483443 - Flags: review-
Depends on: 1011393
No longer depends on: 1011393
Updated version, please review.
Attachment #8487928 - Flags: review?(dkeeler)
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.
Attachment #8487928 - Flags: review?(dkeeler)
(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 on attachment 8487928 [details] [diff] [review]
Bug_1059204-Prepare-verification-code-for-reuse-2.patch

Zoran answered your question, please review again.
Attachment #8487928 - Flags: review?(dkeeler)
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.
Attachment #8487928 - Flags: review?(dkeeler) → review-
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
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.
Attachment #8490861 - Flags: review?(dkeeler) → review-
Attachment #8483443 - Attachment is obsolete: true
Attachment #8487928 - Attachment is obsolete: true
Attachment #8490861 - Attachment is obsolete: true
Attachment #8491733 - Flags: review?(dkeeler)
Attachment #8491733 - Flags: review?(dkeeler) → review?(rlb)
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)
Attachment #8491733 - Flags: review?(rlb) → review+
r+ from rbarnes
Attachment #8491733 - Attachment is obsolete: true
Attachment #8492191 - Flags: review+
Depends on: 1059216
Blocks: 1059216
No longer depends on: 1059216
https://hg.mozilla.org/mozilla-central/rev/9a3e34d36cc1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8492191 [details] [diff] [review]
0001-Bug-1059204-Prepare-verification-code-for-reuse.patch

got a=bajaj over email
Attachment #8492191 - Flags: approval-mozilla-aurora+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.