1125503 - SEGV in nsSiteSecurityService::GetHost

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Atte Kettunen]

Attachment: repro-file

Tested on:

OS: Ubuntu 14.04

Firefox: ASAN build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1422098911/

This crash looks like a fairly new regression, I have seen it on my cluster for couple of weeks now.

Not sure about the security impact, but set restricted just to be on the safe side.

ASAN-trace:

==21785==ERROR: AddressSanitizer: SEGV on unknown address 0x7f81c1e2399f (pc 0x7f80bf5a3d77 sp 0x7fffb8ce3aa0 bp 0x7fffb8ce3c90 T0)
    #0 0x7f80bf5a3d76 in Last /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/security/manager/boot/src/../../../../dist/include/nsTSubstring.h:280
    #1 0x7f80bf5a3d76 in get /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/security/manager/boot/src/PublicKeyPinningService.cpp:378
    #2 0x7f80bf5a3d76 in nsSiteSecurityService::GetHost(nsIURI*, nsACString_internal&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/security/manager/boot/src/nsSiteSecurityService.cpp:287
    #3 0x7f80bf5a8f17 in nsSiteSecurityService::IsSecureURI(unsigned int, nsIURI*, unsigned int, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/security/manager/boot/src/nsSiteSecurityService.cpp:804
    #4 0x7f80b94c58b5 in mozilla::net::nsHttpChannel::Connect() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/protocol/http/nsHttpChannel.cpp:309
    #5 0x7f80b94f49dd in mozilla::net::nsHttpChannel::ContinueBeginConnect() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/protocol/http/nsHttpChannel.cpp:5036
    #6 0x7f80b94f3b48 in mozilla::net::nsHttpChannel::BeginConnect() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/protocol/http/nsHttpChannel.cpp:4977
    #7 0x7f80b94f5274 in mozilla::net::nsHttpChannel::OnProxyAvailable(nsICancelable*, nsIChannel*, nsIProxyInfo*, tag_nsresult) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/protocol/http/nsHttpChannel.cpp:5098
    #8 0x7f80b915e600 in nsAsyncResolveRequest::DoCallback() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsProtocolProxyService.cpp:280
    #9 0x7f80b912d182 in Run /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsProtocolProxyService.cpp:167
    #10 0x7f80b912d182 in nsProtocolProxyService::AsyncResolveInternal(nsIChannel*, unsigned int, nsIProtocolProxyCallback*, nsICancelable**, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/netwerk/base/nsProtocolProxyService.cpp:1246
.
.
.

Comment 1

[Dana Keeler (she/her) [:keeler]]

Attachment: patch

Turns out, we need to check Length() > 0 before calling Last() on strings.

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8554743 [details] [diff] [review]
patch

Review of attachment 8554743 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/unit/test_sts_fqdn.js
@@ +44,5 @@
>                                          "example.com..", 0));
> +
> +  // Somehow creating this malformed URI succeeds - we need to handle it
> +  // gracefully.
> +  uri = Services.io.newURI("https://../foo", null, null);

We should probably file bugs on not being able to create malformed URIs like this.

Comment 3

[Dana Keeler (she/her) [:keeler]]

Thanks for the quick review.
For the record, this was introduced by bug 1065909. In most cases this is a simple read-out-of-bounds by 1, and I think the worst that can happen is a crash.

Comment 4

[Dana Keeler (she/her) [:keeler]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d5f07809c98
https://hg.mozilla.org/integration/mozilla-inbound/rev/35f422e0053c

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35f422e0053c

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta/b2g34 approval on this when you get a chance :)

Comment 7

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8554743 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1065909
[User impact if declined]: crashes
[Describe test coverage new/current, TreeHerder]: good test coverage (also added a new test)
[Risks and why]: low - this is a simple fix
[String/UUID change made/needed]: none

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ba6239fc70f5
https://hg.mozilla.org/releases/mozilla-beta/rev/e25b169e456b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ba6239fc70f5
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/17a38b0a22e7