Closed Bug 1470156 Opened 2 years ago Closed 2 years ago

Crash in mozilla::OriginAttributes::CreateSuffix const

Categories

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

60 Branch
Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: dswhite21, Assigned: timhuang)

References

()

Details

(Whiteboard: [domsecurity-active])

Crash Data

Attachments

(2 files)

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

=============================================================
Component: General → Networking: Cookies
Product: Firefox → Core
Seems to be hitting a release assert added in bug 1260931.
Blocks: 1260931
Component: Networking: Cookies → DOM: Security
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
Assignee: nobody → tihuang
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P2
Whiteboard: [domsecurity-active]
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?
Flags: needinfo?(arthuredelstein)
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
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
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.
Attachment #8988219 - Attachment description: 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, rpl → 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 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
Attachment #8988219 - Flags: review+
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
Attachment #8988433 - Flags: review+
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
Attachment #8988219 - Flags: review+
(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.
Flags: needinfo?(arthuredelstein)
Pushed by tihuang@mozilla.com:
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
Pushed by tihuang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17e4514dc6e6
Part 2: Fixing the crashing problem when using an invalid character in a firstPartyDomain. r=baku
Pushed by cbrindusan@mozilla.com:
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
Flags: needinfo?(tihuang)
https://hg.mozilla.org/mozilla-central/rev/17e4514dc6e6
https://hg.mozilla.org/mozilla-central/rev/e78128970a60
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1534339
You need to log in before you can comment on or make changes to this bug.