|
|
DescriptionAdd 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)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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= ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
scottkirkwood@chromium.org changed reviewers: + hanxi@chromium.org
https://codereview.chromium.org/2873943005/diff/1/chrome/android/webapk/libs/... 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/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:372: if (positionOfLastByteOfLastBlock > mCentralDirOffset + MAX_V2_SIGNING_BLOCK_SIZE) { Should we use |V2_SIGNING_MAGIC.length()| to replace |MAX_V2_SIGNING_BLOCK_SIZE| here, since we have the condition in line 371? I guess you could move this check before the check of |V2_SIGNING_MAGIC.equals(magic)| on line 371.
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2873943005/diff/1/chrome/android/webapk/libs/... 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/... 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 18:47:49, Xi Han wrote: > Should we use |V2_SIGNING_MAGIC.length()| to replace |MAX_V2_SIGNING_BLOCK_SIZE| > here, since we have the condition in line 371? > > I guess you could move this check before the check of > |V2_SIGNING_MAGIC.equals(magic)| on line 371. I've added a comment. We don't allow any gap between the last file and the central directory, unless it is v2 signed (has the V2_SIGNING_MAGIC string), then we allow an 8k gap.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
lgtm +pkotwicz: could you please take a look? Thanks! https://codereview.chromium.org/2873943005/diff/1/chrome/android/webapk/libs/... 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/... 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/16 12:38:30, ScottK wrote: > On 2017/05/15 18:47:49, Xi Han wrote: > > Should we use |V2_SIGNING_MAGIC.length()| to replace > |MAX_V2_SIGNING_BLOCK_SIZE| > > here, since we have the condition in line 371? > > > > I guess you could move this check before the check of > > |V2_SIGNING_MAGIC.equals(magic)| on line 371. > > I've added a comment. > We don't allow any gap between the last file and the central directory, unless > it is v2 signed (has the V2_SIGNING_MAGIC string), then we allow an 8k gap. Got it, thanks for the explanation!
Just two comments https://codereview.chromium.org/2873943005/diff/20001/chrome/android/webapk/l... 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/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:355: // Data descriptor: sig(4), crc-32(4), compressed size(4), Isn't the signature optional? https://codereview.chromium.org/2873943005/diff/20001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:374: if (positionOfLastByteOfLastBlock > mCentralDirOffset + MAX_V2_SIGNING_BLOCK_SIZE) { Shouldn't this be: positionOfLastByteOfLastBlock + MAX_V2_SIGNING_BLOCK_SIZE < mCentralDirOffset
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Thanks for the reviews! https://codereview.chromium.org/2873943005/diff/20001/chrome/android/webapk/l... 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/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:355: // Data descriptor: sig(4), crc-32(4), compressed size(4), On 2017/05/16 19:00:32, pkotwicz wrote: > Isn't the signature optional? Yes, you are correct. When we create a WebAPK we always have the data descriptor with the signature. I was hoping to avoid scanning for the signature, but I suppose it's not much extra work so I fixed it so it's more compatible. https://codereview.chromium.org/2873943005/diff/20001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:374: if (positionOfLastByteOfLastBlock > mCentralDirOffset + MAX_V2_SIGNING_BLOCK_SIZE) { On 2017/05/16 19:00:33, pkotwicz wrote: > Shouldn't this be: > > positionOfLastByteOfLastBlock + MAX_V2_SIGNING_BLOCK_SIZE < mCentralDirOffset Fixed, I need to generate a test APK for this one as well.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits https://codereview.chromium.org/2873943005/diff/40001/chrome/android/webapk/l... 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/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:356: long footerSize = 0; Nit: If you declare |lastByte| here you can use |lastByte| on line 358 long lastByte = block.mPosition + block.mHeaderSize + block.mCompressedSize; if ((flags & 0x8) != 0) { seek(lastByte); ... } and then increment |lastByte| by the appropriate amount on lines 362 and 366
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
https://codereview.chromium.org/2873943005/diff/40001/chrome/android/webapk/l... 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/l... 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: > Nit: If you declare |lastByte| here you can use |lastByte| on line 358 > > long lastByte = block.mPosition + block.mHeaderSize + block.mCompressedSize; > if ((flags & 0x8) != 0) { > seek(lastByte); > ... > } > > and then increment |lastByte| by the appropriate amount on lines 362 and 366 Nice find, thanks.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by pkotwicz@chromium.org
The CQ bit was checked by pkotwicz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2873943005/diff/60001/chrome/android/webapk/l... 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/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:356: long lastByte = block.mPosition + block.mHeaderSize + block.mCompressedSize; Something I just noticed. Is there a reason that |lastByte| is a long? (Given that |mEndOfCentralDirOffset| is an int?)
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by scottkirkwood@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2873943005/#ps60001 (title: "Resolve reviewer feedback.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by scottkirkwood@chromium.org to run a CQ dry run
https://codereview.chromium.org/2873943005/diff/60001/chrome/android/webapk/l... 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/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java:356: long lastByte = block.mPosition + block.mHeaderSize + block.mCompressedSize; On 2017/05/17 20:25:16, pkotwicz wrote: > Something I just noticed. Is there a reason that |lastByte| is a long? (Given > that |mEndOfCentralDirOffset| is an int?) we can make it all an int.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by scottkirkwood@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2873943005/#ps80001 (title: "Change long to int.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495204236589160, "parent_rev": "95acdc5a58015c3aefac2f05af19717ba104cae3", "commit_rev": "48a8f38f1be0b12ab76dd6d7543e0448726030a1"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/48a8f38f1be0b12ab76dd6d7543e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/48a8f38f1be0b12ab76dd6d7543e... |