Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(804)

Issue 2873943005: Add additional checks for comment signed WebAPKs. (Closed)

Created:
3 years, 7 months ago by ScottK
Modified:
3 years, 7 months ago
Reviewers:
Xi Han, pkotwicz
CC:
chromium-reviews, zpeng+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add additional checks for comment signed WebAPKs. * Make sure there's no block before the start of the APK. * Make sure there's no block at the end of the APK. * Make sure there's no block before EOCD or CD. * If it appears to be v2 signed, then we allow a medium sized block before the Central Directory (CD). BUG=721530 Review-Url: https://codereview.chromium.org/2873943005 Cr-Commit-Position: refs/heads/master@{#473200} Committed: https://chromium.googlesource.com/chromium/src/+/48a8f38f1be0b12ab76dd6d7543e0448726030a1

Patch Set 1 #

Total comments: 3

Patch Set 2 : Add clarifying comment. #

Total comments: 4

Patch Set 3 : Resolve reviewer comments. #

Total comments: 2

Patch Set 4 : Resolve reviewer feedback. #

Total comments: 2

Patch Set 5 : Change long to int. #

Patch Set 6 : Fix incorrect conditional. Add more apk test files. #

Messages

Total messages: 49 (36 generated)
ScottK
3 years, 7 months ago (2017-05-12 13:49:20 UTC) #7
Xi Han
https://codereview.chromium.org/2873943005/diff/1/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2873943005/diff/1/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java#newcode372 chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:372: if (positionOfLastByteOfLastBlock > mCentralDirOffset + MAX_V2_SIGNING_BLOCK_SIZE) { Should we ...
3 years, 7 months ago (2017-05-15 18:47:49 UTC) #8
ScottK
https://codereview.chromium.org/2873943005/diff/1/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2873943005/diff/1/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java#newcode372 chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:372: if (positionOfLastByteOfLastBlock > mCentralDirOffset + MAX_V2_SIGNING_BLOCK_SIZE) { On 2017/05/15 ...
3 years, 7 months ago (2017-05-16 12:38:30 UTC) #11
Xi Han
lgtm +pkotwicz: could you please take a look? Thanks! https://codereview.chromium.org/2873943005/diff/1/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2873943005/diff/1/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java#newcode372 chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:372: ...
3 years, 7 months ago (2017-05-16 18:03:30 UTC) #15
pkotwicz
Just two comments https://codereview.chromium.org/2873943005/diff/20001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2873943005/diff/20001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java#newcode355 chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:355: // Data descriptor: sig(4), crc-32(4), compressed ...
3 years, 7 months ago (2017-05-16 19:00:33 UTC) #16
ScottK
Thanks for the reviews! https://codereview.chromium.org/2873943005/diff/20001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2873943005/diff/20001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java#newcode355 chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:355: // Data descriptor: sig(4), crc-32(4), ...
3 years, 7 months ago (2017-05-16 22:40:42 UTC) #18
pkotwicz
LGTM with nits https://codereview.chromium.org/2873943005/diff/40001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2873943005/diff/40001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java#newcode356 chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:356: long footerSize = 0; Nit: If ...
3 years, 7 months ago (2017-05-17 15:41:35 UTC) #26
ScottK
https://codereview.chromium.org/2873943005/diff/40001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2873943005/diff/40001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java#newcode356 chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:356: long footerSize = 0; On 2017/05/17 15:41:35, pkotwicz wrote: ...
3 years, 7 months ago (2017-05-17 19:06:38 UTC) #28
pkotwicz
https://codereview.chromium.org/2873943005/diff/60001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2873943005/diff/60001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java#newcode356 chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:356: long lastByte = block.mPosition + block.mHeaderSize + block.mCompressedSize; Something ...
3 years, 7 months ago (2017-05-17 20:25:16 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2873943005/60001
3 years, 7 months ago (2017-05-18 14:54:58 UTC) #38
ScottK
https://codereview.chromium.org/2873943005/diff/60001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java (right): https://codereview.chromium.org/2873943005/diff/60001/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java#newcode356 chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:356: long lastByte = block.mPosition + block.mHeaderSize + block.mCompressedSize; On ...
3 years, 7 months ago (2017-05-18 17:37:13 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2873943005/80001
3 years, 7 months ago (2017-05-19 14:31:12 UTC) #46
commit-bot: I haz the power
3 years, 7 months ago (2017-05-19 15:35:50 UTC) #49
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/48a8f38f1be0b12ab76dd6d7543e...

Powered by Google App Engine
This is Rietveld 408576698