1470156 - Crash in mozilla::OriginAttributes::CreateSuffix const

Status: RESOLVED FIXED

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

Description

[dswhite21]

I can reliably cause a crash in 60.0.x by going to and IPv6 IP address while using the Cookie-AutoDelete extension. If I disable the extension, it will not crash. I also opened a ticket with the extension: https://github.com/Cookie-AutoDelete/Cookie-AutoDelete/issues/398

* Example webaddress to cause the crash: https://[2606:4700:4700::1111]/ (IPv6 version of https://1.1.1.1/)
* OS/version: Macbook Pro High Sierra 10.13.4
* Browser/version: Firefox 60.0.1 and 60.0.2
* Cookie AutoDelete version: 2.2.0



This bug was filed from the Socorro interface and is
report bp-bc911615-e96f-4b51-8103-154b30180621.
=============================================================

Top 10 frames of crashing thread:

0 XUL mozilla::OriginAttributes::CreateSuffix const caps/OriginAttributes.cpp:131
1 XUL mozilla::net::nsCookieKey::HashKey netwerk/cookie/nsCookieKey.h:56
2 XUL PLDHashTable::Search xpcom/ds/PLDHashTable.cpp:519
3 XUL nsCookieService::GetCookiesFromHost xpcom/ds/nsTHashtable.h:135
4 XUL NS_InvokeByIndex 
5 XUL XPCWrappedNative::CallMethod js/xpconnect/src/XPCWrappedNative.cpp:1951
6 XUL XPC_WN_CallMethod js/xpconnect/src/XPCWrappedNativeJSOps.cpp:913
7 XUL js::InternalCallOrConstruct js/src/vm/JSContext-inl.h:290
8 XUL Interpret js/src/vm/Interpreter.cpp:523
9 XUL js::RunScript js/src/vm/Interpreter.cpp:418

=============================================================

Comment 1

[Johann Hofmann [:johannh]]

Seems to be hitting a release assert added in bug 1260931.

Comment 2

[dswhite21]

Reproduced on Ubuntu: bp-5f7fc933-e399-4d75-95f0-a1d380180621
Reproduced on Windows 10: bp-47cedb8d-9f44-46e0-8be6-699820180621
Reproduced on Android: bp-ffd21862-d53b-432c-8a92-e6d170180621

Comment 3

[Tim Huang[:timhuang]]

The problem here is that the IPv6 address contains an invalid character which is ':' in this case. So it will try to use this invalid character to create a suffix when passing a IPv6 address as a firstPartyDomain into cookies.getAll(). This causes this crash to happen.

We should sanitize the firstPartyDomain when it is an IP address in this case, i.e. replacing invalid characters with '+', like the QuotaManager does.

However, there is another problem here. In current implementation, we won't put a firstPartyDomain into OA if it is an IP address because of we use a TLD service to generate a firstPartyDomain. So, in this case, there will be no firstPartyDomain if the first party is an IP address. So, people could be tracked if they browse the Web in a pure IP address manner. I don't know anyone does this, though. But, I think we should fix this.

What do you think, Baku and Arthur?

Comment 4

[Andrea Marchesini [:baku]]

It seems there are 2 separate bugs here. Let's fix the crash here only. I'm OK with sanitizing the firstPartyDomain value with '+'.

For the firstPartyDomain + IP address, we should have a separate bug.

Comment 5

[dswhite21]

I saw the new release (61.0) runs web extensions in their own process on Mac. Just out of curiosity I reproduced again on mac running 61.0 bp-6b3c7bbb-88e1-4746-b4a0-157d50180626

Comment 6

[Phabricator Automation]

Attachment: Bug 1470156 - Part 1: Adding a test case for reassuring mozilla::OriginAttributes::CreateSuffix won't be crashed with invalid characters in firstParty domain. r=baku, mixedpuppy

Comment 7

[Phabricator Automation]

Attachment: Bug 1470156 - Part 2: Fixing the crashing problem when using an invalid character in a firstPartyDomain. r=baku

This patch adds a sanitization of firstPartyDomain when calling the
OriginAttributes::CreateSuffix() and remove the release assert there.
The cookies API for the web extension can use a arbitrary string for the
firstPartyDomain. So, we should sanitize the firstPartyDomain before
we creating a suffix. The release assert is not required anymore since
the firstPartyDomain is sanitized

Depends on D1845.

Comment 8

[Phabricator Automation]

Comment on attachment 8988219 [details]
Bug 1470156 - Part 1: Adding a test case for reassuring mozilla::OriginAttributes::CreateSuffix won't be crashed with invalid characters in firstParty domain. r=baku, mixedpuppy

Shane Caraveo (:mixedpuppy) has approved the revision.

https://phabricator.services.mozilla.com/D1845

Comment 9

[Phabricator Automation]

Comment on attachment 8988433 [details]
Bug 1470156 - Part 2: Fixing the crashing problem when using an invalid character in a firstPartyDomain. r=baku

Andrea Marchesini [:baku] has approved the revision.

https://phabricator.services.mozilla.com/D1856

Comment 10

[Phabricator Automation]

Comment on attachment 8988219 [details]
Bug 1470156 - Part 1: Adding a test case for reassuring mozilla::OriginAttributes::CreateSuffix won't be crashed with invalid characters in firstParty domain. r=baku, mixedpuppy

Andrea Marchesini [:baku] has approved the revision.

https://phabricator.services.mozilla.com/D1845

Comment 11

[Georg Koppen]

(In reply to Tim Huang[:timhuang] from comment #3)
> However, there is another problem here. In current implementation, we won't
> put a firstPartyDomain into OA if it is an IP address because of we use a
> TLD service to generate a firstPartyDomain. So, in this case, there will be
> no firstPartyDomain if the first party is an IP address. So, people could be
> tracked if they browse the Web in a pure IP address manner. I don't know
> anyone does this, though. But, I think we should fix this.

I agree.

Comment 12

[Tim Huang[:timhuang]]

Try looks good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=634ba754a91e57ef08a09ccdda32ae6aff86576b

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/604fb83b6b61
Part 1: Adding a test case for reassuring mozilla::OriginAttributes::CreateSuffix won't be crashed with invalid characters in firstParty domain. r=baku,mixedpuppy

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17e4514dc6e6
Part 2: Fixing the crashing problem when using an invalid character in a firstPartyDomain. r=baku

Comment 15

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 604fb83b6b61 (bug 1470156) for mochitest failures on test_ext_cookies_first_party.html

Backout: https://hg.mozilla.org/integration/autoland/rev/8cd5cb7aebe27dd339772edd1127c6aa80f617a3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=604fb83b6b61c66ad29f46d81fb9fa58bd9b55fc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=186274752

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=186276538&repo=autoland&lineNumber=4819

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e78128970a60
Part 1: Adding a test case for reassuring mozilla::OriginAttributes::CreateSuffix won't be crashed with invalid characters in firstParty domain. r=baku,mixedpuppy

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/17e4514dc6e6
https://hg.mozilla.org/mozilla-central/rev/e78128970a60