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

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: 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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Francois, something you could take a quick look at?

Comment 2

[François Marier [: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-

Comment 3

[François Marier [:francois]]

Attachment: Bug 1364262 - Convert SRI metadata to ASCII before parsing it.

Review commit: https://reviewboard.mozilla.org/r/139918/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139918/

Comment 4

[François Marier [: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".

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(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 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8868345 [details]
Bug 1364262 - Convert SRI metadata to ASCII before parsing it.

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

thanks

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9bb34819be6
Convert SRI metadata to ASCII before parsing it. r=ckerschb

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e9bb34819be6

Comment 9

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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?

Comment 10

[François Marier [:francois]]

(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.