WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
182924
Potential privacy issue: DNS prefetching can be re-enabled
https://bugs.webkit.org/show_bug.cgi?id=182924
Summary
Potential privacy issue: DNS prefetching can be re-enabled
jens.a.mueller+webkit
Reported
2018-02-19 05:40:34 PST
In the scope of academic research we systematically analyzed various email clients for `web bugs' -- 1x1 pixel images in HTML emails used by spammers to track if their mails to a certain address are actually read. To respect the privacy of their customers most email clients, by default, block external content like images in HTML emails. This can be bypassed on Trojitá and Evolution by re-enabling DNS prefetching within the HTML email itself: <meta http-equiv="x-dns-prefetch-control" content="on"> <a href="
http://tracking-id.attacker.com
"></a> The related bug reports can be found here:
https://bugs.kde.org/show_bug.cgi?id=390452
https://bugzilla.gnome.org/show_bug.cgi?id=793449
Both mail clients use WebKit to render HTML emails, so it may actually be a WebKit issue and should be fixed here. For the testing the mail clients we used Debian GNU/Linux 9.3 with: - libwebkit2gtk-4.0-37:amd64 (version 2.16.6+0+deb9u1) - libqt5webkit5:amd64 (version 5.7.1+dfsg-1)
Attachments
proposed patch
(1.27 KB, patch)
2018-02-21 05:58 PST
,
Milan Crha
no flags
Details
Formatted Diff
Diff
proposed patch (fixed tab in ChangeLog)
(1.28 KB, patch)
2018-02-21 06:14 PST
,
Milan Crha
no flags
Details
Formatted Diff
Diff
proposed patch ][
(2.34 KB, patch)
2018-02-22 01:33 PST
,
Milan Crha
mcatanzaro
: review-
mcatanzaro
: commit-queue-
Details
Formatted Diff
Diff
proposed patch ]I[
(1.21 KB, patch)
2018-02-26 10:19 PST
,
Milan Crha
mcatanzaro
: commit-queue-
Details
Formatted Diff
Diff
proposed patch IV
(1.12 KB, patch)
2018-02-27 00:32 PST
,
Milan Crha
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Milan Crha
Comment 1
2018-02-21 05:58:11 PST
Created
attachment 334367
[details]
proposed patch Let's prefer settings value over the on ein the page. That is, once the DNS prefething is disabled in the WebKit settings, it cannot be enabled by the page.
EWS Watchlist
Comment 2
2018-02-21 06:01:03 PST
Attachment 334367
[details]
did not pass style-queue: ERROR: Source/WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Milan Crha
Comment 3
2018-02-21 06:14:13 PST
Created
attachment 334372
[details]
proposed patch (fixed tab in ChangeLog)
Michael Catanzaro
Comment 4
2018-02-21 12:29:45 PST
Comment on
attachment 334372
[details]
proposed patch (fixed tab in ChangeLog) Tor users are not going to be happy about this. (Another good example of why you should only use Tor with Tor Browser Bundle....) I think this patch makes sense, but since no ports enable DNS prefetch by default, this really totally disables DNS prefetch in WebKit, to the point where we should be considering removing the feature entirely. And that is rather strange, since prefetch is considered to be a major performance boost. By coincidence, I had started a discussion about this yesterday:
https://lists.webkit.org/pipermail/webkit-dev/2018-February/029881.html
Milan Crha
Comment 5
2018-02-21 13:13:19 PST
Tor users can enable the DNS prefetch in settings and it'll make them happy again. Evolution explicitly disables the prefetch. I didn't know all webkit has this disabled by default, good to know. As this will be all client-driven, there is no need to remove the setting and the code around it, it's all fine. I believe the intention to have the page override this option is just a security hole, especially after Jens explained it (to me) in the GNOME bugzilla. I hesitate to insert at the top of each generated HTML code by evolution (in iframe-s too?) a new <meta> first, to explicitly disable prefetch, then any similar tag in received mail would not enable it, but if you think it's the way to go, then let it be the way to go. Another approach would be to have enable-dns-prefetch a three-state value: 1) enabled always 2) disabled always 3) disabled, but allow the page enable it (to mimic the current behaviour and expectations) and the most the page will be always able to disable DNS pefetch, but enable it only if the setting will be the value 3) from the above list.
Chris Dumez
Comment 6
2018-02-21 13:18:17 PST
Comment on
attachment 334372
[details]
proposed patch (fixed tab in ChangeLog) View in context:
https://bugs.webkit.org/attachment.cgi?id=334372&action=review
> Source/WebCore/dom/Document.cpp:-5763 > - if (equalLettersIgnoringASCIICase(dnsPrefetchControl, "on") && !m_haveExplicitlyDisabledDNSPrefetch) {
I'd rather we return early in this method if !settings().dnsPrefetchingEnabled(). Otherwise, we end up changing the value of m_haveExplicitlyDisabledDNSPrefetch.
Milan Crha
Comment 7
2018-02-21 13:33:56 PST
(In reply to Chris Dumez from
comment #6
)
> I'd rather we return early in this method if > !settings().dnsPrefetchingEnabled(). Otherwise, we end up changing the value > of m_haveExplicitlyDisabledDNSPrefetch.
I've been thinking of it too, but: a) I wanted ensure that the prefetch will be disabled b) that variable is used basically only here (and in initDNSPrefetch(), where it's set to 'false' again) and after this change is means "explicitly disabled by settings" here, from my point of view. I added some debug prints around and the initDNSPrefetch() is called plenty of times after each page reload (or load of a new page in case of Evolution), thus the value is reset often.
Michael Catanzaro
Comment 8
2018-02-21 14:25:48 PST
(In reply to Milan Crha from
comment #5
)
> Tor users can enable the DNS prefetch in settings and it'll make them happy > again.
I mean: if you use tor, you really don't want to be locally prefetching anything.
Michael Catanzaro
Comment 9
2018-02-21 14:28:47 PST
I also think early return would be cleaner. Chris, you're OK with changing the semantics of this HTTP header to only allow disabling prefetch, never enabling it? I think we need to; the bug report is valid.
Chris Dumez
Comment 10
2018-02-21 15:33:33 PST
(In reply to Michael Catanzaro from
comment #9
)
> I also think early return would be cleaner. > > Chris, you're OK with changing the semantics of this HTTP header to only > allow disabling prefetch, never enabling it? I think we need to; the bug > report is valid.
I think the change makes sense. Client setting should override anything else IMHO.
Milan Crha
Comment 11
2018-02-22 01:33:10 PST
Created
attachment 334442
[details]
proposed patch ][ In case you want the page only to disable prefetching, but never enable it, then the m_haveExplicitlyDisabledDNSPrefetch property can be removed and the code simplified as much as this.
Chris Dumez
Comment 12
2018-02-22 09:00:32 PST
Comment on
attachment 334442
[details]
proposed patch ][ View in context:
https://bugs.webkit.org/attachment.cgi?id=334442&action=review
> Source/WebCore/dom/Document.cpp:-5769 > - m_haveExplicitlyDisabledDNSPrefetch = true;
I do not understand why it is OK to drop this flag. This flag has nothing to do with the page setting.
Michael Catanzaro
Comment 13
2018-02-22 10:36:01 PST
Comment on
attachment 334442
[details]
proposed patch ][ View in context:
https://bugs.webkit.org/attachment.cgi?id=334442&action=review
> Source/WebCore/dom/Document.cpp:5765 > + // let the page only disable prefetching, not enable it
WebKit style is to write the comment as a full sentence, with a capital L and a period at the end.
>> Source/WebCore/dom/Document.cpp:-5769 >> - m_haveExplicitlyDisabledDNSPrefetch = true; > > I do not understand why it is OK to drop this flag. This flag has nothing to do with the page setting.
I think m_haveExplicitlyDisabledDNSPrefetch is there to ensure that if prefetch is disabled, either with the HTTP header or HTML meta http-equiv, it can't ever be reenabled the same way. Thanks for trying to clean up the code, but I think we should keep it.
Milan Crha
Comment 14
2018-02-22 23:39:45 PST
Well, when I change it to something like this: diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp index 0a5a1df61a..db08fb9b7a 100644 --- a/Source/WebCore/dom/Document.cpp +++ b/Source/WebCore/dom/Document.cpp @@ -5765,10 +5765,12 @@ void Document::initDNSPrefetch() void Document::parseDNSPrefetchControlHeader(const String& dnsPrefetchControl) { - if (equalLettersIgnoringASCIICase(dnsPrefetchControl, "on") && !m_haveExplicitlyDisabledDNSPrefetch) { - m_isDNSPrefetchEnabled = true; + if (!settings().dnsPrefetchingEnabled()) + return; + + // Let the page only disable prefetching, not enable it. + if (equalLettersIgnoringASCIICase(dnsPrefetchControl, "on")) return; - } m_isDNSPrefetchEnabled = false; m_haveExplicitlyDisabledDNSPrefetch = true; then it's only written to m_haveExplicitlyDisabledDNSPrefetch, never read from it. That's why I removed it. I think the change is that simple that you can correct it the way which prefer to you, I really do not care of credits, I only would like to have this included in the next stable release of WebKitGTK+ and not miss it due to nitpicks.
Michael Catanzaro
Comment 15
2018-02-23 11:32:13 PST
Just don't get rid of the && !m_haveExplicitlyDisabledDNSPrefetch check: void Document::parseDNSPrefetchControlHeader(const String& dnsPrefetchControl) { // Prefetch disabled in settings if (!settings().dnsPrefetchingEnabled()) return; // Prefetch requested by website *AND not previously turned off by website* if (equalLettersIgnoringASCIICase(dnsPrefetchControl, "on") && !m_haveExplicitlyDisabledDNSPrefetch) { m_isDNSPrefetchEnabled = true; return; } // Prefetch turned off by website (any value for the header except "on") m_isDNSPrefetchEnabled = false; m_haveExplicitlyDisabledDNSPrefetch = true; }
Milan Crha
Comment 16
2018-02-26 01:32:15 PST
(In reply to Chris Dumez from
comment #10
)
> (In reply to Michael Catanzaro from
comment #9
) > > I also think early return would be cleaner. > > > > Chris, you're OK with changing the semantics of this HTTP header to only > > allow disabling prefetch, never enabling it? I think we need to; the bug > > report is valid. > > I think the change makes sense. Client setting should override anything else > IMHO.
The suggestion in
comment #15
goes against the above quoted part, from my point of view. Especially when we agree that the web page should not ever enable the DNS prefetch on its own when user settings is OFF. I look on it this way: - settings = OFF, then DNS prefetch is kept off whatever the page or response headers will claim - settings = ON, then the DNS prefetch is enabled and the page can only disable the prefetch; when the page uses "on", then it won't change anything (prefetch is already enabled) * if the page (or the transport headers) disable DNS prefetching and then the page "changes its mind" and will try to enable it again, then the code with m_haveExplicitlyDisabledDNSPrefetch doesn't make much sense to me, because in that case WebKit would try to dictate some "workflow" to the web pages, which looks odd, because WebKit has the settings to let the clients (users of WebKit) influence the DNS prefetching behaviour, and the user already said it's fine to do DNS prefetching, thus it doesn't matter whether the page disabled and then enabled it again (or ten times). I think that the code complexity with m_haveExplicitlyDisabledDNSPrefetch is not needed, unless you are afraid of (or you want to cover) some header injection on the response headers/page content(/iframes?), but even then the last word has the WebKit client through the settings and if it is fine with DNS prefetching, then let the page do it or change it on/off to its likings.
Michael Catanzaro
Comment 17
2018-02-26 09:11:50 PST
(In reply to Milan Crha from
comment #16
)
> The suggestion in
comment #15
goes against the above quoted part, from my > point of view. Especially when we agree that the web page should not ever > enable the DNS prefetch on its own when user settings is OFF. I look on it > this way: > > - settings = OFF, then DNS prefetch is kept off whatever the page or > response headers will claim
Yes
> - settings = ON, then the DNS prefetch is enabled and the page can only > disable the prefetch; when the page uses "on", then it won't change > anything (prefetch is already enabled)
Yes
> * if the page (or the transport headers) disable DNS prefetching > and then the page "changes its mind" and will try to enable it again, > then the code with m_haveExplicitlyDisabledDNSPrefetch doesn't > make much sense to me, because in that case WebKit would try to > dictate > some "workflow" to the web pages, which looks odd, because WebKit has > the settings to let the clients (users of WebKit) influence the DNS > prefetching behaviour, and the user already said it's fine to do DNS > prefetching, thus it doesn't matter whether the page disabled and then > enabled it again (or ten times). > > I think that the code complexity with m_haveExplicitlyDisabledDNSPrefetch is > not needed, unless you are afraid of (or you want to cover) some header > injection on the response headers/page content(/iframes?), but even then the > last word has the WebKit client through the settings and if it is fine with > DNS prefetching, then let the page do it or change it on/off to its likings.
Since this setting affects the security of the page, the current code ensures that it is not possible to re-enable it once it has been disabled. There doesn't seem to be any strong reason for your patch to remove this restriction. I would prefer that your patch make only the originally-desired change.
Milan Crha
Comment 18
2018-02-26 10:19:53 PST
Created
attachment 334626
[details]
proposed patch ]I[ Okay okay :)
Michael Catanzaro
Comment 19
2018-02-26 12:38:05 PST
Comment on
attachment 334626
[details]
proposed patch ]I[ View in context:
https://bugs.webkit.org/attachment.cgi?id=334626&action=review
Now, this patch looks good to me. Maybe Chris will want to give final review.
> Source/WebCore/dom/Document.cpp:5771 > + // Prefetch requested by the website AND not previously turned off by the website
Please don't add this comment to the code; I had meant that to be a comment for Bugzilla purposes only. :) It's somewhat obvious, and also not a complete sentence.
Milan Crha
Comment 20
2018-02-27 00:32:53 PST
Created
attachment 334678
[details]
proposed patch IV
Michael Catanzaro
Comment 21
2018-02-27 07:54:42 PST
Comment on
attachment 334678
[details]
proposed patch IV Chris, this look good to you?
Chris Dumez
Comment 22
2018-02-27 08:15:10 PST
Comment on
attachment 334678
[details]
proposed patch IV Yes.
WebKit Commit Bot
Comment 23
2018-02-27 08:39:16 PST
Comment on
attachment 334678
[details]
proposed patch IV Clearing flags on attachment: 334678 Committed
r229061
: <
https://trac.webkit.org/changeset/229061
>
WebKit Commit Bot
Comment 24
2018-02-27 08:39:18 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug