Closed Bug 1461037 Opened 5 years ago Closed 5 years ago

ProcessUserNotice doesn't handle invalid UTF-8 safely

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: keeler, Assigned: keeler)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

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.)
Assignee: nobody → dkeeler
Priority: -- → P1
Whiteboard: [psm-assigned]
Attachment #8976724 - Flags: review?(franziskuskiefer) → review?(jjones)
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.
Attachment #8976724 - Flags: review?(jjones) → review+
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db3a97478cbf
lossily convert invalid UTF8 in certificates for display purposes r=jcj
https://hg.mozilla.org/mozilla-central/rev/db3a97478cbf
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.