Closed Bug 1364262 Opened 3 years ago Closed 3 years ago

Assertion failure: metadataList.Length() <= aMetadataList.Length(), @ [/home/worker/workspace/build/src/dom/security/SRICheck.cpp:115]

Categories

(Core :: DOM: Security, defect, P1)

43 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: jkratzer, Assigned: francois)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [domsecurity-active])

Attachments

(2 files)

Attached file Testcase
Testcase found while fuzzing mozilla-central rev 20170511-3b96f2773257.

Assertion failure: metadataList.Length() <= aMetadataList.Length(), at /home/worker/workspace/build/src/dom/security/SRICheck.cpp:115

ASAN:DEADLYSIGNAL
=================================================================
==21174==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f4355488190 bp 0x7ffd934a38d0 sp 0x7ffd934a3120 T0)
==21174==The signal is caused by a WRITE memory access.
==21174==Hint: address points to the zero page.
    #0 0x7f435548818f in mozilla::dom::SRICheck::IntegrityMetadata(nsAString const&, nsACString const&, nsIConsoleReportCollector*, mozilla::dom::SRIMetadata*) /home/worker/workspace/build/src/dom/security/SRICheck.cpp:115:3
    #1 0x7f43560df37d in mozilla::css::Loader::CreateSheet(nsIURI*, nsIContent*, nsIPrincipal*, mozilla::css::SheetParsingMode, mozilla::CORSMode, mozilla::net::ReferrerPolicy, nsAString const&, bool, bool, nsAString const&, mozilla::css::StyleSheetState&, bool*, RefPtr<mozilla::StyleSheet>*) /home/worker/workspace/build/src/layout/style/Loader.cpp:1256:7
    #2 0x7f43560e2a89 in mozilla::css::Loader::LoadStyleLink(nsIContent*, nsIURI*, nsAString const&, nsAString const&, bool, mozilla::CORSMode, mozilla::net::ReferrerPolicy, nsAString const&, nsICSSLoaderObserver*, bool*) /home/worker/workspace/build/src/layout/style/Loader.cpp:2111:8
    #3 0x7f43530ec58b in nsStyleLinkElement::DoUpdateStyleSheet(nsIDocument*, mozilla::dom::ShadowRoot*, nsICSSLoaderObserver*, bool*, bool*, bool) /home/worker/workspace/build/src/dom/base/nsStyleLinkElement.cpp:455:7
    #4 0x7f43530ecd3d in nsStyleLinkElement::UpdateStyleSheetInternal(nsIDocument*, mozilla::dom::ShadowRoot*, bool) /home/worker/workspace/build/src/dom/base/nsStyleLinkElement.cpp:234:10
    #5 0x7f4354bd4421 in mozilla::dom::HTMLLinkElement::AfterSetAttr(int, nsIAtom*, nsAttrValue const*, bool) /home/worker/workspace/build/src/dom/html/HTMLLinkElement.cpp:393:7
    #6 0x7f4352e4956e in mozilla::dom::Element::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, unsigned char, bool, bool, bool, nsIDocument*, mozAutoDocUpdate const&) /home/worker/workspace/build/src/dom/base/Element.cpp:2501:10
    #7 0x7f4352e48c76 in mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString const&, bool) /home/worker/workspace/build/src/dom/base/Element.cpp:2378:10
    #8 0x7f4354ca7699 in nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString const&, bool) /home/worker/workspace/build/src/dom/html/nsGenericHTMLElement.cpp:830:34
    #9 0x7f4352e43398 in mozilla::dom::Element::SetAttribute(nsAString const&, nsAString const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/Element.cpp:1237:14
    #10 0x7f4354232919 in mozilla::dom::ElementBinding::setAttribute(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/ElementBinding.cpp:726:9
    #11 0x7f43546a7a1c in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2954:13
    #12 0x7f4358cf6241 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15
    #13 0x7f4358cf5ded in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:470:16
    #14 0x7f4358cf6c95 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:515:12
    #15 0x7f4358cead03 in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3025:18
Flags: in-testsuite?
Francois, something you could take a quick look at?
Flags: needinfo?(francois)
It seems that this UTF16-to-UTF8 conversion (https://dxr.mozilla.org/mozilla-central/rev/b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5/dom/security/SRICheck.cpp#111):

  NS_ConvertUTF16toUTF8 metadataList(aMetadataList);

takes an 11-byte long string and converts it to a 15-byte long one:

  [Main Thread]: D/SRI nsStyleLinkElement::DoUpdateStyleSheet, integrity=sha512-
Flags: needinfo?(francois)
Nice, the weird characters caused comment 2 to get truncated. Let me try again (with censored output).

It seems that this UTF16-to-UTF8 conversion (https://dxr.mozilla.org/mozilla-central/rev/b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5/dom/security/SRICheck.cpp#111):

  NS_ConvertUTF16toUTF8 metadataList(aMetadataList);

takes an 11-byte long string and converts it to a 15-byte long one:

  [Main Thread]: D/SRI nsStyleLinkElement::DoUpdateStyleSheet, integrity=sha512-<censored>
  [Main Thread]: D/SRI css::Loader::CreateSheet, integrity=sha512-<censored>
  [Main Thread]: D/SRI SRICheck::IntegrityMetadata, metadataList.Length()=15 aMetadataList.Length()=11

I'm not sure why but given that SRI metadata is defined to be ASCII (https://www.w3.org/TR/SRI/#the-integrity-attribute), we could do a lossy conversion instead or we could check that the length is the same after the conversion and return NS_ERROR_SRI_CORRUPT immediately.

My patch does the former. The problem with returning early is that it means the metadata is considered empty as opposed to invalid. Therefore we allow the load instead of blocking it as we should.

I don't think the lossy conversion is a problem because it will replace non-ASCII characters with characters that are still outside the range of allowed values for the SRI metadata so there's no way to get a valid hash value (one that will base64-decode correctly) if any of the original characters are "lost".
Assignee: nobody → francois
Status: NEW → ASSIGNED
(In reply to François Marier [:francois] from comment #4)
> Nice, the weird characters caused comment 2 to get truncated. Let me try
> again (with censored output).

:-)

> I don't think the lossy conversion is a problem because it will replace
> non-ASCII characters with characters that are still outside the range of
> allowed values for the SRI metadata so there's no way to get a valid hash
> value (one that will base64-decode correctly) if any of the original
> characters are "lost".

I agree and I think using NS_LossyConvertUTF16toASCII is what we should do.
Comment on attachment 8868345 [details]
Bug 1364262 - Convert SRI metadata to ASCII before parsing it.

https://reviewboard.mozilla.org/r/139918/#review145006

thanks
Attachment #8868345 - Flags: review?(ckerschb) → review+
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9bb34819be6
Convert SRI metadata to ASCII before parsing it. r=ckerschb
Priority: -- → P1
Whiteboard: [domsecurity-active]
https://hg.mozilla.org/mozilla-central/rev/e9bb34819be6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
AFAICT, this goes back to Fx43 when the code originally landed in bug 992096. Is this likely to manifest in a user-facing way, Francois? And if so, is this something we should consider uplifting or can it ride the trains?
Blocks: 992096
Flags: needinfo?(francois)
Flags: in-testsuite?
Flags: in-testsuite+
Version: unspecified → 43 Branch
(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> AFAICT, this goes back to Fx43 when the code originally landed in bug
> 992096. Is this likely to manifest in a user-facing way, Francois? And if
> so, is this something we should consider uplifting or can it ride the trains?

Both the UTF-8 string and the ASCII string end up using invalid characters, so they both get rejected in the same way. I don't believe it will be a user-visible change in non-DEBUG builds.
Flags: needinfo?(francois)
You need to log in before you can comment on or make changes to this bug.