1461037 - ProcessUserNotice doesn't handle invalid UTF-8 safely

Status: RESOLVED FIXED

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

Description

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Stumbled on this while printing out each built-in certificate in an xpcshell test:

 0:02.21 pid:22096 \x07[22096, Main Thread] ###!!! ASSERTION: Input wasn't UTF8 or incorrect length was calculated: 'Error', file /home/keeler/mozilla-unified/xpcom/string/nsReadableUtils.cpp, line 380
 0:02.86 pid:22096 #01: AppendUTF8toUTF16(nsTSubstring<char> const&, nsTSubstring<char16_t>&) (/home/keeler/mozilla-unified/xpcom/string/nsReadableUtils.cpp:344)
 0:03.15 pid:22096 #02: NS_ConvertUTF8toUTF16 (/home/keeler/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/nsString.h:130)
 0:05.58 pid:22096 #03: ProcessUserNotice(SECItemStr*, nsTSubstring<char16_t>&) (/home/keeler/mozilla-unified/security/manager/ssl/nsNSSCertHelper.cpp:1129)
 0:05.59 pid:22096 #04: ProcessCertificatePolicies(SECItemStr*, nsTSubstring<char16_t>&) (/home/keeler/mozilla-unified/security/manager/ssl/nsNSSCertHelper.cpp:1197)
 0:05.59 pid:22096 #05: ProcessSingleExtension(CERTCertExtensionStr*, nsIASN1PrintableItem**) (/home/keeler/mozilla-unified/security/manager/ssl/nsNSSCertHelper.cpp:1421)
 0:05.60 pid:22096 #06: ProcessExtensions(CERTCertExtensionStr**, nsIASN1Sequence*) (/home/keeler/mozilla-unified/security/manager/ssl/nsNSSCertHelper.cpp:1622)
 0:05.60 pid:22096 #07: nsNSSCertificate::CreateTBSCertificateASN1Struct(nsIASN1Sequence**) (/home/keeler/mozilla-unified/security/manager/ssl/nsNSSCertHelper.cpp:1768)
 0:05.61 pid:22096 #08: nsNSSCertificate::CreateASN1Struct(nsIASN1Object**) (/home/keeler/mozilla-unified/security/manager/ssl/nsNSSCertHelper.cpp:1797)
 0:05.62 pid:22096 #09: NS_InvokeByIndex (/home/keeler/mozilla-unified/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:135)
 0:05.69 pid:22096 #10: CallMethodHelper::Call() (/home/keeler/mozilla-unified/js/xpconnect/src/XPCWrappedNative.cpp:1268)
 0:05.70 pid:22096 #11: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (/home/keeler/mozilla-unified/js/xpconnect/src/XPCWrappedNative.cpp:1235)
 0:05.71 pid:22096 #12: XPC_WN_GetterSetter(JSContext*, unsigned int, JS::Value*) (/home/keeler/mozilla-unified/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:949)
 0:05.87 pid:22096 #13: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/keeler/mozilla-unified/js/src/vm/JSContext-inl.h:281)
 0:05.87 pid:22096 #14: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (/home/keeler/mozilla-unified/js/src/vm/Interpreter.cpp:467)
 0:05.88 pid:22096 #15: InternalCall(JSContext*, js::AnyInvokeArgs const&) (/home/keeler/mozilla-unified/js/src/vm/Interpreter.cpp:516)
 0:05.88 pid:22096 #16: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (/home/keeler/mozilla-unified/js/src/vm/Interpreter.cpp:535)
 0:05.89 pid:22096 #17: js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) (/home/keeler/mozilla-unified/js/src/vm/Interpreter.cpp:650)
 0:06.74 pid:22096 #18: CallGetter(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<js::Shape*>, JS::MutableHandle<JS::Value>) (/home/keeler/mozilla-unified/js/src/vm/NativeObject.cpp:2173)
 0:06.74 pid:22096 #19: bool GetExistingProperty<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) (/home/keeler/mozilla-unified/js/src/vm/NativeObject.cpp:2226)
 0:06.75 pid:22096 #20: bool NativeGetPropertyInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, IsNameLookup, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) (/home/keeler/mozilla-unified/js/src/vm/NativeObject.cpp:2439)
 0:09.30 pid:22096 #21: js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) (/home/keeler/mozilla-unified/js/src/vm/NativeObject.h:1646)
 0:09.31 pid:22096 #22: js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) (/home/keeler/mozilla-unified/js/src/vm/JSObject.h:808)
 0:09.32 pid:22096 #23: JO(JSContext*, JS::Handle<JSObject*>, (anonymous namespace)::StringifyContext*) (/home/keeler/mozilla-unified/js/src/builtin/JSON.cpp:413)
 0:09.81 pid:22096 #24: Str(JSContext*, JS::Value const&, (anonymous namespace)::StringifyContext*) (/home/keeler/mozilla-unified/obj-x86_64-pc-linux-gnu/js/src/Unified_cpp_js_src0.cpp:?)
 0:09.83 pid:22096 #25: js::Stringify(JSContext*, JS::MutableHandle<JS::Value>, JSObject*, JS::Value const&, js::StringBuffer&, js::StringifyBehavior) (/home/keeler/mozilla-unified/js/src/builtin/JSON.cpp:758)
 0:09.84 pid:22096 #26: json_stringify(JSContext*, unsigned int, JS::Value*) (/home/keeler/mozilla-unified/js/src/builtin/JSON.cpp:943)
 0:09.84 pid:22096 #27: ??? (???:???)
 0:09.84 pid:22096 [22096, Main Thread] ###!!! ASSERTION: Input wasn't UTF8 or incorrect length was calculated: 'Error', file /home/keeler/mozilla-unified/xpcom/string/nsReadableUtils.cpp, line 380
 0:09.84 pid:22096 Hit MOZ_CRASH() at /home/keeler/mozilla-unified/memory/mozalloc/mozalloc_abort.cpp:34

Lo and behold:

    switch (notice->displayText.type) {
      case siAsciiString:
      case siVisibleString:
      case siUTF8String:
        text.Append(NS_ConvertUTF8toUTF16((const char*)notice->displayText.data,
                                          notice->displayText.len));
        break;

The type is provided by the certificate, so there's no guarantee that the data actually is a UTF-8 string (and maybe whatever a "visible" string is isn't valid UTF-8?)
(This probably only affects debug builds, but still.)

Comment 1

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: bug 1461037 - lossily convert invalid UTF8 in certificates for display purposes

In debug builds, we assert if any UTF8-to-UTF16 conversion fails. If we have
invalid UTF8 in a certificate, we don't want to assert. So, we now lossily
convert invalid UTF8 in certificates for any display purposes.
This also handles fields that are supposed to be ASCII in a similar way.

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

Comment 2

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8976724 [details]
bug 1461037 - lossily convert invalid UTF8 in certificates for display purposes

https://reviewboard.mozilla.org/r/244794/#review251916

This looks okay to me. Note that the Adopt in `LossyUTF8toUTF16` could yield a result string that's marked VOIDED under memory pressure, but I think that's just display issues. It looks like it'll be safe.

::: security/manager/ssl/nsNSSCertHelper.cpp:769
(Diff revision 1)
> +
>  static nsresult
>  ProcessRDN(CERTRDN* rdn, nsAString& finalString)
>  {
> -  nsresult rv;
> -  CERTAVA** avas;
> +  CERTAVA** avas = rdn->avas;
> +  for (auto i = 0; avas[i]; i++) {

oh, my, thank you.

::: security/manager/ssl/nsNSSCertificate.cpp:367
(Diff revision 1)
>      mCert->emailAddr
>    };
>  
> -  nsAutoCString nameOption;
> -  for (auto nameOptionPtr : nameOptions) {
> -    nameOption.Assign(nameOptionPtr);
> +  for (auto nameOption : nameOptions) {
> +    if (nameOption) {
> +      LossyUTF8ToUTF16(nameOption, strlen(nameOption), aDisplayName);

FWIW, for all the `strlen` here in nsNSSCertificate, I've checked that these all ultimately come from either PORTString that eforce a null-terminator, or from `mCert`'s `CERT_DerNameToAscii` method that does the same.

Comment 3

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8976724 [details]
bug 1461037 - lossily convert invalid UTF8 in certificates for display purposes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244794/diff/1-2/

Comment 4

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8976724 [details]
bug 1461037 - lossily convert invalid UTF8 in certificates for display purposes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244794/diff/2-3/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db3a97478cbf
lossily convert invalid UTF8 in certificates for display purposes r=jcj

Comment 6

[Eliza Balazs [:ebalazs_]]

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