Predator did not provide any possible suspects.
Assigning to the concern owner from CL --
https://chromium.googlesource.com/chromium/src/+log/e5e81dd5d79c96f679275de1244aa645b5af44bc..7edc791763ef56992687f32632011290bf967b0a?pretty=fuller

Suspecting Commit#
https://chromium.googlesource.com/chromium/src/+/cba51dd8181fd3a661ea14b5a1368f21c499d088

@vogelheim -- Could you please look into the issue, kindly re-assign if this is not related to your changes.
Thank You.

@msrchandra: Yeah, that's my CL. Thanks for forwarding.

The test file has a resource with zero bytes content. The check is meant to check whether the bytes were discarded; not whether there weren't any to start with. Need to refine that, somehow.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/269a2c54a0931022f1436060a6e4523633e694f3

commit 269a2c54a0931022f1436060a6e4523633e694f3
Author: Daniel Vogelheim <vogelheim@chromium.org>
Date: Mon Jul 24 11:15:32 2017

Fix ScriptResource::CheckResourceIntegrity for zero-length resources.

Background: This fixes a regression from https://chromium-review.googlesource.com/c/556033/, which
has fixed the incorrect, original implementation of ScriptResource::CheckResourceIntegrity:

The original implementation would return true if it couldn't access the raw resource
data. This happened in some good cases (if the check had already been done before, or if the
resource was legitimately empty), but also in some not-so-good ones (see the test cases for
the previous CL). The previous CL tightened those restrictions, so that we no longer return
out of caution, but only when we have actually checked that data. This check overlooked the
case of zero byte long resources. So originally, these would always pass (regardless of any
integrity=... attribute); then they would crash (because of a nullptr dereference); and now
things should work per spec.

Bug:  740867 
Change-Id: I23a19f63823510baf6847425423a13c579b13a61
Reviewed-on: https://chromium-review.googlesource.com/568481
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Daniel Vogelheim <vogelheim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488945}
[add] https://crrev.com/269a2c54a0931022f1436060a6e4523633e694f3/third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/resources/empty.js
[add] https://crrev.com/269a2c54a0931022f1436060a6e4523633e694f3/third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-empty-resource-allowed-expected.txt
[add] https://crrev.com/269a2c54a0931022f1436060a6e4523633e694f3/third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-empty-resource-allowed.html
[add] https://crrev.com/269a2c54a0931022f1436060a6e4523633e694f3/third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-empty-resource-blocked-expected.txt
[add] https://crrev.com/269a2c54a0931022f1436060a6e4523633e694f3/third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-empty-resource-blocked.html
[modify] https://crrev.com/269a2c54a0931022f1436060a6e4523633e694f3/third_party/WebKit/Source/core/loader/resource/ScriptResource.cpp

ClusterFuzz has detected this issue as fixed in range 488849:488966.

Detailed report: https://clusterfuzz.com/testcase?key=5591993408552960

Fuzzer: ochang_domfuzzer
Job Type: linux_tsan_chrome_mp
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  !!ResourceBuffer() in ScriptResource.cpp
  
Sanitizer: thread (TSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=484998:485004
Fixed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=488849:488966

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5591993408552960


See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 5591993408552960 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Hi, I just noticed that this fix needs merge, because this regressed on M-61 and fixed on M-62.
61 crashes on M-61 beta:
https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.magic_signature_1.name%3D%27blink%3A%3AScriptResource%3A%3ACheckResourceIntegrity%27%20AND%20custom_data.ChromeCrashProto.channel%3D%27beta%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D&unnest=#-property-selector,samplereports:5,+productversion

However, the revised CHECK is also failing on ToT (Issue 752830).
I'll do an initial investigation on Issue 752830 and I might request merge here depending on that.

Summary:

There are two failing CHECK()s around here:
(A) The crash of  Issue 740867 .
    This crashes for integrity check + empty body.
(B) The crash of Issue 752830.
    This crashes for integrity check
    + non-empty body before revalidation
    + empty body after revalidation.
And two related (non-crashing) SRI bugs:
(C) Issue 757125: SRI isn't checked for the body after revalidation.
(D)  Issue 701943 : SRI is not properly enabled on preloaded CSSs and
      thus the CSSs are refetched with SRI again.

Timeline:
(C) and (D) have existed since before.

[0] https://chromium-review.googlesource.com/c/556033: 61.0.3152.0
  Introduced (A).

Crash range of (A): 61.0.3152.0 -- 62.0.3164.0

[1] https://chromium-review.googlesource.com/c/568481: 62.0.3166.0
  Fixed (A).
  Introduced (B), but this doesn't crash at this time
    because (C) is not fixed -- no SRI check after revalidation
    and thus no crash there.

No (A) crashes observed after [1].
No (B) crashes observed after [1] before [2].

[2] https://chromium-review.googlesource.com/c/576950: 62.0.3175.0
  Fixed (C), therefore exposes (B) on ScriptResource.

Crash range of (B): 62.0.3175.0 -- 62.0.3189.0 -- current.

[3] https://chromium-review.googlesource.com/c/562859: 62.0.3180.0
  Fixed (D), and this also exposes (B) on CSSStylesheetResource.
  (Not so sure why though)

[4a] https://chromium-review.googlesource.com/c/621239 and
[4b] https://chromium-review.googlesource.com/c/622278
  Will fix (B).

The tests for (B) I used are:
  https://chromium-review.googlesource.com/c/621240
For crash ranges see crbug.com/752830#c2.

Current status on branches:
  M-61 canary branch: (A) is crashing (61 crashes on beta, see Comment #8)
  M-62 ToT branch: (A) is fixed but (B) is crashing. I hope [4a] and [4b] fix th

So candidate plans for merging is:
Plan 1: Merge [1] to M-61.
  As no crashes are observed just after [1] (until [2]), probably it's safe to merge?

Plan 2: Merge [1], [4a] and [4b] to M-61.
  (Because [2] is not merged, [4b] needs rebasing though)
  Probably safer than Plan 1?

Plan 3: Turn the CHECK (A) into DCHECK on M-61.
Plan 4: Merge [1] to M-61 and then turn the CHECK (B) into DCHECK on M-61.
  As SRI is somehow still broken (provided (C) and (D) are not merged) on M-61,
  we might want to suppress the CHECK crash on stable?

I'm not sure which is the best.
vogelheim@ (and security feature team) and M-61 branch owners, what do you think?

This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

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

+ abdulsyed@ (Chrome Desktop TPM) and +awhalley@ (Security TPM) to take their input on this merge (pls see comment #9 for more details).

#9: Many thanks for investigation!


CHECK -> DCHECK: I fully agree. Given that breaking the CHECK condition doesn't actually cause any direct harm (other than the CHECK failure itself), I think turning it into a DCHECK is the correct action.

Looking back, I was over-confident in thinking I had figured out all the edge cases, and should have made this a DCHECK in the first place. Sorry for that!


Merging: I think we should definitely convert the CHECK->DCHECK on M61. Additionally merging the actual fixes would IMHO be best. But if release managers have stability concerns, then I think only dis-arming the CHECKs would be fine.

If the fix is important to get in from a security perspective, and is safe to merge, well tested, I am fine approving it to merge for M61. 

No security implication either way, so I'm OK with either approach.

Fix is wrong

 DCHECK(resource->ResourceBuffer());
    if (resource->ResourceBuffer()->size() < small_script_threshold_)
      return;

Won't we crash on the if line after skipping the DCHECK in production build.

govind asked for additional commentary; I'm not really sure what I'm expected to do.


My understanding is: We're good for M62 (with Hiroshige's upcoming fixes, at least); the remaining question is whether we need to backmerge some (or all) of the fixes into M61, per Hiroshige's thoughtful request in #9.


#14: I agree: What's left are stability, not security concerns.

The fix(es) here touch some (minor) security issues; but if they aren't backmerged, then the security will be as good or bad as in previous releases. What might have gotten worse is stability, in that an incomplete set of fixes might cause additional renderer crashes (because of bugs in the fixes + a release check that finds those).


#13: Given what I said above, the first of those conditions isn't met. I take it this implies 'please don't backmerge'. I'm fine with that.

(My thinking would have been more in line w/ Hiroshige's in #9; but I can certainly see that there's a risk in the backmerges themselves.)


#15: I don't know what to make of this contribution.

I don't think that's the assertion in question; it's certainly not the one I've been looking at. Also, if the "fix is wrong", we'll notice this in M62.


Please let me know whether release owners stand by #13, and whether I'm reading it correctly.

Thank you vogelheim@.

Rejecting merge to M61 branch 3163 based on comment #17. Please let me know if anyone has any concern here. Thank you.