Bug 145542 – [SOUP] Performs DNS prefetch when a proxy is configured (information leak)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Catanzaro]

Complaint: "Apparently it seems that even when configured to use Tor as proxy, epiphany is so "smart" to send DNS queries directly to the wire, thus making any effort of Tor useless."

I found in DNSSoup.cpp:

// There is no current reliable way to know if we're behind a proxy at
// this level. We'll have to implement it in
// SoupSession/SoupProxyURIResolver/GProxyResolver
bool DNSResolveQueue::platformProxyIsEnabledInSystemPreferences()
{
    return false;
}

Note: This is not really exploitable per se and it's public in three downstream bugtrackers, so no point in trying to hide this -> public intentionally.

[Michael Catanzaro]

CCing a few people who should know about security problems....

Since libsoup 2.42, there is a SoupSession:proxy-resolver property that we can use to fix this. Users who compile with older libsoup will need to be advised to turn off DNS prefetch.

[Michael Catanzaro]

Hm, that's not right. Returning false from DNSResolveQueue::platformProxyIsEnabledInSystemPreferences causes us to perform DNS prefetch when there is a proxy, which is a bug (the indented behavior is for prefetch to be disabled when there is a proxy). But that is no reason to believe that the prefetch we do perform doesn't use the proxy.

[Michael Catanzaro]

(In reply to comment #2)
> But that is no reason to believe that the prefetch we do perform doesn't use the proxy.

Well here's one reason: it wouldn't make any sense for us to send a DNS query if we're using a proxy, except if the DNS query is to find the proxy itself.

I'm not very good at Bugzilla, am I? :D

[Michael Catanzaro]

I will handle the CVE request.

I don't know how to write a test for this without using libpcap, which wouldn't be worth the effort and would be flaky if the system were to make some other DNS request for whatever reason. I verified this patch manually using the simple-proxy example program in the libsoup repository, this test program, and Wireshark:

int
main (void)
{
  GtkWidget *web_view;
  GtkWidget *window;

  gtk_init (NULL, NULL);

  window = gtk_window_new (GTK_WINDOW_TOPLEVEL);

  web_view = webkit_web_view_new ();

  webkit_web_context_prefetch_dns (webkit_web_context_get_default (), "eff.org");

  gtk_widget_set_size_request (web_view, 600, 500);
  gtk_widget_show_all (window);

  gtk_main ();

  return 0;
}

The DNS request is sent if and only if no proxy is configured in System Settings.

[Michael Catanzaro]

Created attachment 254204 [details]
Patch

[Michael Catanzaro]

Created attachment 254205 [details]
Patch

[Alexey Proskuryakov]

Moving the decision into a separate file makes it more important to have an accurate name.

It can be OK to do prefetching when using proxy - if it's the proxy that does DNS resolution. Some SOCKS proxies do.

I don't have a suggestion though.

[Michael Catanzaro]

(In reply to comment #7)
> Moving the decision into a separate file makes it more important to have an
> accurate name.
> 
> It can be OK to do prefetching when using proxy - if it's the proxy that
> does DNS resolution. Some SOCKS proxies do.
> 
> I don't have a suggestion though.

Hm, yes, that's a difference between our code: the CF backend will skip the prefetch only if using an HTTP or HTTPS proxy, while the soup backend will skip the prefetch if there is any proxy.

Naming things is hard... platformResolveIfAppropriate (loses the word "proxy", and what is appropriate?), platformResolveIfNotUsingUnacceptableProxy (pejorative!), platformResolveIfNotUsingHttpOrHttpsProxy (long, CF-specific), ... or keep it platformResolve and just add a comment that you need to check proxy settings, or platformMaybeResolve....

> Since libsoup 2.42, there is a SoupSession:proxy-resolver property that we
> can use to fix this. Users who compile with older libsoup will need to be
> advised to turn off DNS prefetch.

P.S. 2.42 is already our minimum.

[Carlos Garcia Campos]

Comment on attachment 254205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254205&action=review

Adding some comments to the soup part.

> Source/WebCore/platform/network/soup/DNSSoup.cpp:47
> +static void gotProxySettingsCallback(GObject* sourceObject, GAsyncResult* res, void* userData)

Use result instead of res for the GAsyncResult

> Source/WebCore/platform/network/soup/DNSSoup.cpp:49
> +    GRefPtr<GProxyResolver> resolver = adoptGRef(reinterpret_cast<GProxyResolver*>(sourceObject));

Use G_PROXY_RESOLVER() instead of reinterpret_cast.

> Source/WebCore/platform/network/soup/DNSSoup.cpp:55
> +        g_warning("Error determining proxy to use for %s: %s", hostname.get(), error->message);

Use WTFLogAlways instead of g_warning().

> Source/WebCore/platform/network/soup/DNSSoup.cpp:56
> +        goto out;

I personally don't like gotos, I would just free the array here and return instead. We could probably add a smart pointer for char**, and then we could just return here.

> Source/WebCore/platform/network/soup/DNSSoup.cpp:65
> +    if (!uris || !*uris || strcmp(*uris, "direct://"))
> +        goto out;

With a smart pointer this could be just a return. Otherwise I would invert the logic and move the soup_session_prefetch_dns inside the if instead of using a goto.
Shouldn't we also do the prefetch when the resolver lookup doesn't return any URI?

> Source/WebCore/platform/network/soup/DNSSoup.cpp:67
> +    soup_session_prefetch_dns(SoupNetworkSession::defaultSession().soupSession(), hostname.get(), nullptr, resolvedCallback, nullptr);

resolvedCallback code is just one line, we could probably use a lambda instead of a static function.

> Source/WebCore/platform/network/soup/DNSSoup.cpp:77
> +    char* uri = g_strdup(hostname.utf8().data()); // Freed by gotProxySettingsCallback

We can probably make this clear by not using the local variable.

> Source/WebCore/platform/network/soup/DNSSoup.cpp:82
> +    GProxyResolver* resolver = nullptr;
> +    g_object_get(SoupNetworkSession::defaultSession().soupSession(),
> +        "proxy-resolver", &resolver, // Ref adopted in gotProxySettingsCallback
> +        nullptr);

You are initializing resolver to nullptr but never checking the value after g_object_get. The proxy resolver can only be NULL if explicitly set to NULL, something we don't do, so I would add an assert right after the g_object_get.

> Source/WebCore/platform/network/soup/DNSSoup.cpp:84
> +    g_proxy_resolver_lookup_async(resolver, uri, nullptr, gotProxySettingsCallback, static_cast<void*>(uri));

We don't need to keep a ref to the resolver, since the async operation will ensure it's alive during itls lifetime, so I would explicitly unref the resolver here instead of adopting the ref in the callback. Is the void* cast actually needed here?

[Dan Winship]

(In reply to comment #4)
> I don't know how to write a test for this without using libpcap

glib/gio/tests/proxy-test.c has an example of overriding GResolver to ensure that it doesn't get called

[Michael Catanzaro]

(In reply to comment #9)
> Use result instead of res for the GAsyncResult

OK

> Use G_PROXY_RESOLVER() instead of reinterpret_cast.

OK

> Use WTFLogAlways instead of g_warning().

OK
 
> I personally don't like gotos, I would just free the array here and return
> instead. We could probably add a smart pointer for char**, and then we could
> just return here.

> With a smart pointer this could be just a return. Otherwise I would invert
> the logic and move the soup_session_prefetch_dns inside the if instead of
> using a goto.

I added a smart pointer.

> Shouldn't we also do the prefetch when the resolver lookup doesn't return
> any URI?

Well it doesn't matter much, because if that happens the GProxyResolver is broken, but if it does happen I would prefer to fail safe and not perform the prefetch.

> resolvedCallback code is just one line, we could probably use a lambda
> instead of a static function.

I didn't realize lambdas could be used for GAsyncReadyCallbacks. Cool.

> We can probably make this clear by not using the local variable.

It's more verbose since I have to write hostname.utf8().data() twice, but sure.

> You are initializing resolver to nullptr but never checking the value after
> g_object_get. The proxy resolver can only be NULL if explicitly set to NULL,
> something we don't do, so I would add an assert right after the g_object_get.

Good catch. Since the assert guards a null pointer dereference, it should be an ASSERT_WITH_SECURITY_IMPLICATION.
 
> We don't need to keep a ref to the resolver, since the async operation will
> ensure it's alive during itls lifetime, so I would explicitly unref the
> resolver here instead of adopting the ref in the callback.

OK. I couldn't find any documentation about this, so it seemed unwise to assume it would be reffed.

> Is the void* cast
> actually needed here?

Think-o, of course not.

(In reply to comment #10)
> glib/gio/tests/proxy-test.c has an example of overriding GResolver to ensure
> that it doesn't get called

Thank you for that *very* cool trick; I had never seen anything like that before. Unfortunately, this won't work for a WebKit test without adding API to access the SoupSession to replace the SoupProxyResolver. Not sure if we want to add such API (there have been requests, so maybe), but we can't really do so unless we add network process extensions (and even then, I'm not sure if the trick would work: since GResolver class would probably be initialized before dynamically loading the network extension shared object)... I'm not going to do that, certainly we should not just to test this. (I will neglect to mention how long it took me to realize the above. :)

[Michael Catanzaro]

> Well it doesn't matter much, because if that happens the GProxyResolver is
> broken, but if it does happen I would prefer to fail safe and not perform
> the prefetch.

Also, it would be wrong to send an HTTP request to the host if direct:// is not returned by the GProxyResolver, so it makes sense to not send a DNS request for the host either.

[Michael Catanzaro]

Created attachment 254467 [details]
Patch

[Build Bot]

Comment on attachment 254467 [details]
Patch

Attachment 254467 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4939555174088704

New failing tests:
platform/mac-wk2/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-horizontal.html

[Build Bot]

Created attachment 254469 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Michael Catanzaro]

The test failure is unrelated, bug #144811

[Carlos Garcia Campos]

Comment on attachment 254467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254467&action=review

Soup backend changes look good to me. Thanks!

> Source/WebCore/platform/network/soup/DNSSoup.cpp:73
> +    g_object_get(SoupNetworkSession::defaultSession().soupSession(),
> +        "proxy-resolver", &resolver.outPtr(),
> +        nullptr);

This could probably be one line.

> Source/WebCore/platform/network/soup/DNSSoup.cpp:76
> +    g_proxy_resolver_lookup_async(resolver.get(), hostname.utf8().data(), nullptr, gotProxySettingsCallback, g_strdup(hostname.utf8().data()));

Oh, I didn't notice hostname was used twice, sorry, in that case it's indeed better to use a local variable and avoid converting it to utf8 twice.

[Michael Catanzaro]

Created attachment 254487 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 254487 [details]
Patch

Cross-platform and Mac parts look good to me.

[WebKit Commit Bot]

Comment on attachment 254487 [details]
Patch

Clearing flags on attachment: 254487

Committed r185320: <http://trac.webkit.org/changeset/185320>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

We were declined a CVE since the proxy-detection code was deliberately disabled: http://seclists.org/oss-sec/2015/q2/662

[Gavin Barraclough]

This regressed page loading performance - see:
    https://bugs.webkit.org/show_bug.cgi?id=146198

[Carlos Garcia Campos]

(In reply to comment #23)
> This regressed page loading performance - see:
>     https://bugs.webkit.org/show_bug.cgi?id=146198

Why didn't you use webkitbot to rollout the patch? The EWS bot would have caught the build failure, and the commit message would be more informative, including information about the rolled out revision. I'm fixing the build now. Reopening this bug, as webkitbot would have done too.

[Michael Catanzaro]

Hm, webkitbot is good at rollouts. :) I am not sure how the rollout went so wrong (you even removed the original changelog entry, which was probably excessive!). Anyway, it's fixed, so moving on:

I don't think the performance problem can be related to the synchronous proxy configuration check performed by the CFNet backend every five seconds, since (a) it only happens once every five seconds, and (b) that behavior hasn't changed. Instead, I guess that the regression is due to no longer returning early in DNSResolveQueue::add() and/or DNSResolveQueue::timerFired() in the case a system proxy is configured. It seems like it should not be a big deal, but I don't see anything else that could be problematic, so I guess that is it. Can you confirm that the performance tests run on a machine with a system proxy configured? If not, then I am not sure what is the cause.

Anyway, I don't see a good way to fix this besides to change the soup backend to perform the check synchronously instead of asynchronously. We would then need to run the check only once every five seconds and only for example.com (which is less "correct" but good enough), same as CFNet does. That way, we would not need to change any cross-platform code, and could not possibly hurt performance for the CFNet backend.

[Gavin Barraclough]

(In reply to comment #24)
> (In reply to comment #23)
> > This regressed page loading performance - see:
> >     https://bugs.webkit.org/show_bug.cgi?id=146198
> 
> Why didn't you use webkitbot to rollout the patch? The EWS bot would have
> caught the build failure, and the commit message would be more informative,
> including information about the rolled out revision. I'm fixing the build
> now. Reopening this bug, as webkitbot would have done too.

My apologies - I still have a pretty archaic workflow & do everything with svn directly. Sorry for the breakage.

[Michael Catanzaro]

Created attachment 261583 [details]
Patch

[Michael Catanzaro]

Created attachment 261994 [details]
Patch

[Michael Catanzaro]

Ping reviewers, we're still getting complaints about this....

[Darin Adler]

Comment on attachment 261994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261994&action=review

I made some comments about the style of the code and about the CFNetwork implementation that are not new things introduced in this patch.

> Source/WebCore/platform/network/DNSResolveQueue.cpp:70
> +    // Note that m_cachedProxyEnabledStatus must be initially true so as to ensure that prefetch is
> +    // disabled until the system proxy settings are available. This makes no difference for ports
> +    // that look up proxy settings synchronously, as DNS prefetch will become enabled during the
> +    // first call to isUsingProxy, but the Soup port does so asynchronously, and prefetch should not
> +    // be enabled until that completes.

A comment this long is often a sign of trouble. I would write something at least a little shorter. Here’s my attempt:

    // isUsingProxy will return the initial value of m_cachedProxyEnabledStatus at first on platforms that have
    // an asynchronous implementation of platformUpdateCachedProxyEnabledStatus, so initialize it to true so
    // we won't prefetch before we know if we are using a proxy.

I think m_cachedProxyEnabledStatus is an unnecessarily confusing and long name. We should rename it to m_isUsingProxy.

> Source/WebCore/platform/network/DNSResolveQueue.cpp:76
>      static const double minimumProxyCheckDelay = 5;

This 5-second polling strategy is really strange on iOS and Mac. Surely there are notifications of changes in proxy status that we could take advantage of instead of using polling. Someone should talk to CFNetwork experts about this at some point.

> Source/WebCore/platform/network/DNSResolveQueue.h:55
> +    void platformUpdateCachedProxyEnabledStatus();

I would call this updateProxyStatus or even possibly updateIsUsingProxy or updateUsingProxy. I don’t think the platform prefix is helpful, although I do understand that this function has a platform-specific implementation. I also don’t think that cached or enabled help clarify the function’s purpose; while they make the function’s name slightly more precise, they also make it so long it’s hard to comprehend.

> Source/WebCore/platform/network/cf/DNSCFNet.cpp:54
>      // Don't do DNS prefetch if proxies are involved. For many proxy types, the user agent is never exposed
>      // to the IP address during normal operation. Querying an internal DNS server may not help performance,

This comment belongs in the cross-platform code, not here in the CFNetwork-specific implementation.

> Source/WebCore/platform/network/soup/DNSSoup.cpp:65
> +    static bool isUsingHttpProxy = true;
> +    static bool isUsingHttpsProxy = true;

A bit peculiar. We could use global variables for all the state in DNSResolveQueue, but we use data members of an immortal object instead. I suppose it’s all right, and an expedient way to keep platform-specific details out of the DNSResolveQueue header, but it’s kind of funny to have an immortal object with some data members and some global variables.

> Source/WebCore/platform/network/soup/DNSSoup.cpp:83
> +    *data->cachedProxyEnabledStatus = isUsingHttpProxy || isUsingHttpsProxy;

Is there a threading issue here? Since DNSResolveQueue is an immortal object there is no need to worry about object lifetime, but on some architectures it’s not safe to set a boolean on one thread while reading it on another, especially if also we might be setting adjacent data members on the original thread. Typically we would avoid this problem by using callOnMainThread before writing a new value to the data member. I don’t know what the thread semantics of g_proxy_resolver_lookup_async are.

> Source/WebCore/platform/network/soup/DNSSoup.cpp:92
> +    auto data = new ProxyResolvedUserData {ProxyType::Http, &m_cachedProxyEnabledStatus};

Seems unnecessary to allocate this on the heap since there will only ever be two of them, given we also use global variables inside the callback function. It’s a strange mix of some state in DNSResolveQueue, other state in a dynamically allocated and then deleted object, and yet other state in global variables inside the callback function. I think this could be tightened up a bit, but probably OK as is.

[Michael Catanzaro]

(In reply to comment #30)
> A comment this long is often a sign of trouble. I would write something at
> least a little shorter. Here’s my attempt:
> 
>     // isUsingProxy will return the initial value of
> m_cachedProxyEnabledStatus at first on platforms that have
>     // an asynchronous implementation of
> platformUpdateCachedProxyEnabledStatus, so initialize it to true so
>     // we won't prefetch before we know if we are using a proxy.

OK.

> I think m_cachedProxyEnabledStatus is an unnecessarily confusing and long
> name. We should rename it to m_isUsingProxy.

Probably, OK.

> > Source/WebCore/platform/network/DNSResolveQueue.cpp:76
> >      static const double minimumProxyCheckDelay = 5;
> 
> This 5-second polling strategy is really strange on iOS and Mac. Surely
> there are notifications of changes in proxy status that we could take
> advantage of instead of using polling. Someone should talk to CFNetwork
> experts about this at some point.

Yes, it's quite hackish. I don't know what the "right" behavior is for CFNet (and I don't plan to change it), but on Linux the "right" approach is to check the proxy status once for each URI, since proxies can be configured on a per-URI basis. E.g. it is possible to configure a proxy to be used only for particular sites, in which case

My original patch up above accounted for that, but it slowed down the Mac performance tests (which was a bit unexpected, as it didn't change the CFNet behavior, but it got rid of a couple early returns in the cross-platform code). In this second attempt, I simply copied the CFNet behavior. The alternative would have been to add #ifdefs in DNSResolveQueue to avoid changing the behavior there on Mac, which would make things messy but guarantee I can make my changes without messing up your performance tests. Maybe that would have been a better approach.

> > Source/WebCore/platform/network/DNSResolveQueue.h:55
> > +    void platformUpdateCachedProxyEnabledStatus();
> 
> I would call this updateProxyStatus or even possibly updateIsUsingProxy or
> updateUsingProxy. I don’t think the platform prefix is helpful, although I
> do understand that this function has a platform-specific implementation. I
> also don’t think that cached or enabled help clarify the function’s purpose;
> while they make the function’s name slightly more precise, they also make it
> so long it’s hard to comprehend.

OK

> > Source/WebCore/platform/network/cf/DNSCFNet.cpp:54
> >      // Don't do DNS prefetch if proxies are involved. For many proxy types, the user agent is never exposed
> >      // to the IP address during normal operation. Querying an internal DNS server may not help performance,
> 
> This comment belongs in the cross-platform code, not here in the
> CFNetwork-specific implementation.

I'll move it, either in this patch or in a follow-up.

> > Source/WebCore/platform/network/soup/DNSSoup.cpp:65
> > +    static bool isUsingHttpProxy = true;
> > +    static bool isUsingHttpsProxy = true;
> 
> A bit peculiar. We could use global variables for all the state in
> DNSResolveQueue, but we use data members of an immortal object instead. I
> suppose it’s all right, and an expedient way to keep platform-specific
> details out of the DNSResolveQueue header, but it’s kind of funny to have an
> immortal object with some data members and some global variables.

True... but it's needed by non-member functions, so these would have to be public and static in DNSResolveQueue, which swings this in favor of keeping them in DNSSoup.cpp IMO.

> > Source/WebCore/platform/network/soup/DNSSoup.cpp:83
> > +    *data->cachedProxyEnabledStatus = isUsingHttpProxy || isUsingHttpsProxy;
> 
> Is there a threading issue here? Since DNSResolveQueue is an immortal object
> there is no need to worry about object lifetime, but on some architectures
> it’s not safe to set a boolean on one thread while reading it on another,
> especially if also we might be setting adjacent data members on the original
> thread. Typically we would avoid this problem by using callOnMainThread
> before writing a new value to the data member. I don’t know what the thread
> semantics of g_proxy_resolver_lookup_async are.

There's no threading here; both callbacks will execute on the same main context, so they can't happen simultaneously.

> > Source/WebCore/platform/network/soup/DNSSoup.cpp:92
> > +    auto data = new ProxyResolvedUserData {ProxyType::Http, &m_cachedProxyEnabledStatus};
> 
> Seems unnecessary to allocate this on the heap since there will only ever be
> two of them, given we also use global variables inside the callback
> function. It’s a strange mix of some state in DNSResolveQueue, other state
> in a dynamically allocated and then deleted object, and yet other state in
> global variables inside the callback function. I think this could be
> tightened up a bit, but probably OK as is.

I agree, this is not great... I can avoid the allocations by passing only the enum as the user data, and assigning DNSResolveQueue::m_cachedProxyEnabledStatus (well, renamed to something better) to a file-scope pointer the first time through platformUpdateCachedProxyEnabledStatus (where DNSResolveQueue::m_cachedProxyEnabledStatus is visible; making it public isn't right; it's an implementation detail).

Yuckiness like this is hard to avoid when using C APIs; I don't think I can convert lambdas to GCallbacks, which would make things simpler since then the callbacks would be in the scope of DNSResolveQueue.

[Michael Catanzaro]

(In reply to comment #31)
> E.g. it is possible to configure a proxy to be used only
> for particular sites, in which case

in which case performing the prefetch would be an info leak, even though it would be fine to prefetch www.example.com. My original patch (above) accounted for that, but this patch does not. The downside is that for our port, checking the proxy status is an async call that might do file I/O to read the proxy configuration, so there could be some performance penalty.

[Darin Adler]

(In reply to comment #31)
> but on Linux the "right" approach is to check the proxy status once for each URI,
> since proxies can be configured on a per-URI basis

Sure, all major platforms support that; there's a thing called a "proxy configuration file" that is an arbitrary JavaScript programs that runs and decides whether a proxy should be used. But likely it’s not practical to do that check every time if the goal is to have this be an effective performance optimization.

> The
> alternative would have been to add #ifdefs in DNSResolveQueue to avoid
> changing the behavior there on Mac, which would make things messy but
> guarantee I can make my changes without messing up your performance tests.

Hmm, not sure what we would be doing with the #if.

Maybe you're saying that you would be willing to make the non-CFNetwork-based platforms slower? If so, I am not sure that's a good idea. I don't think the slowdown observed on Mac is really a platform-specific issue, it's simply that we are measuring performance carefully there.

> I agree, this is not great... I can avoid the allocations by passing only
> the enum as the user data

Or by using two different callback functions that call the same helper function to do the work (they in turn could pass an enum if that's what how you want to structure things). Then you could use the user data pointer for the one pointer you were wanting to pass through.

[Darin Adler]

I think the intent should be to turn off DNS prefetch entirely if any proxying is enabled, rather than to decide per-hostname or per-URL. The implementation quality question would be how each particular platform exposes the status of whether *any* HTTP or HTTPS proxying is enabled, and any changes in that status.

[Michael Catanzaro]

(In reply to comment #33)
> Maybe you're saying that you would be willing to make the
> non-CFNetwork-based platforms slower? If so, I am not sure that's a good
> idea. I don't think the slowdown observed on Mac is really a
> platform-specific issue, it's simply that we are measuring performance
> carefully there.

Honestly, since we don't measure performance on our port, I have no clue what the performance impact would be and we have no reasonable way to use that for decision-making, so indeed my priority is just to avoid a performance regression for you. (Besides, if I fail, I know my change will just be reverted again. :) Anyway, the penalty could be worse for you than for us, since your proxy check requires synchronous I/O, or it could be that the slowdown was entirely due to the minor changes in the cross-platform code that were not directly related to the proxy-lookup. Also, your performance tests load pages locally, right? I suspect performing any prefetch can only slow down the performance tests!

> Or by using two different callback functions that call the same helper
> function to do the work (they in turn could pass an enum if that's what how
> you want to structure things). Then you could use the user data pointer for
> the one pointer you were wanting to pass through.

Hm, yeah, that would work... a bit messy, but avoids the allocations.

(In reply to comment #34)
> I think the intent should be to turn off DNS prefetch entirely if any
> proxying is enabled, rather than to decide per-hostname or per-URL. The
> implementation quality question would be how each particular platform
> exposes the status of whether *any* HTTP or HTTPS proxying is enabled, and
> any changes in that status.

The problem for us is that we unfortunately have no way to query whether a proxy is enabled for any possible URI, only for any particular URI. So the approach of checking http://example.com is going to have to be good enough. I guess if you've been doing it for a while and nobody has complained, it's probably good enough for us, too. We can reconsider this approach if we get complaints, or if the CFNet implementation changes, but it's clearly superior to what we're doing now (no proxy check at all).

[Darin Adler]

(In reply to comment #35)
> The problem for us is that we unfortunately have no way to query whether a
> proxy is enabled for any possible URI, only for any particular URI.

Yes, that is a problem.

[Darin Adler]

What I meant to say is:

If this information leak issue is real enough to be concerned about, then we should consider, on all platforms we support, to get suitable underlying API to tell us reliably when a proxy is known to not be involved, so we don't have to probe and guess about whether there is a proxy involved.

An alternative is to forgo the DNS prefetch optimization entirely if such API is not available.

I am not satisfied with the current implementation for Mac and iOS.

[Darin Adler]

(In reply to comment #35)
> (In reply to comment #33)
> > Or by using two different callback functions that call the same helper
> > function to do the work (they in turn could pass an enum if that's what how
> > you want to structure things). Then you could use the user data pointer for
> > the one pointer you were wanting to pass through.
> 
> Hm, yeah, that would work... a bit messy, but avoids the allocations.

Maybe not as messy as you think.

We add two functions with single line bodies.

We remove a struct, two lines of code to allocate/initialize the struct, and a line of code inside the callback function that puts the pointer into a unique_ptr to guarantee it's deleted.

Overall, I think it would be slightly less messy or at least a wash.

[Michael Catanzaro]

(In reply to comment #37)
> What I meant to say is:
> 
> If this information leak issue is real enough to be concerned about,

Well it rather defeats the point of using a proxy, so it's a problem, yes.

> then we
> should consider, on all platforms we support, to get suitable underlying API
> to tell us reliably when a proxy is known to not be involved, so we don't
> have to probe and guess about whether there is a proxy involved.
> 
> An alternative is to forgo the DNS prefetch optimization entirely if such
> API is not available.

I really don't want to do that, but got to admit, I know you are right....

This will be some effort to fix in glib/libsoup, since it requires changing the GProxyResolver interface, all implementations (e.g. SoupProxyResolver, but I think that's not the only one in GNOME), and also libproxy (px_proxy_factory_get_proxies has a mandatory uri argument). So realistically, not going to be fixed anytime soon. I think libproxy upstream does not even exist anymore, since Google Code shut down, so we'd need to resurrect it on new infrastructure in order to make any changes....

The GTK+ port has had hover prefetch disabled forever anyway (not sure why; I was planning to look into enabling that once we fix this), and Epiphany stopped doing manual prefetch recently (due to problems with the "awesome bar" implementation), so it's not a big deal for us to turn this off....

(In reply to comment #38)
> Overall, I think it would be slightly less messy or at least a wash.

OK, I agree.

[Michael Catanzaro]

Attaching an updated patch which addresses Darin's review comments....

(In reply to comment #37)
> If this information leak issue is real enough to be concerned about, then we
> should consider, on all platforms we support, to get suitable underlying API
> to tell us reliably when a proxy is known to not be involved, so we don't
> have to probe and guess about whether there is a proxy involved.
> 
> An alternative is to forgo the DNS prefetch optimization entirely if such
> API is not available.

I'm torn on whether I should land this patch or disable DNS prefetch entirely. I intend to pick one way or another within the next week, as either option is clearly better than the status quo. I am leaning towards landing this patch, even though it would leave the privacy violation partially unfixed, since it would be good enough for 99% of users and disabling prefetch could have severe negative performance impact.

Adding API to determine whether any proxy might ever be selected would be a fairly large project requiring new API in libproxy, glib, glib-networking, and possibly libsoup, so it's not going to happen anytime soon. I'm pretty sure libsoup should be responsible for handling the decision of whether or not to prefetch anyway, and WebKit should really not have to bother with checking proxy settings at all. If CFNet already has this API, you should count yourself lucky!

(We also do not have any notifications for proxy settings changes. Part of the problem is that the GProxyResolver interface is very generic; by default, it's backed by libproxy, but the libproxy implementation can be replaced with any arbitrary implementation by users at runtime!)

> I am not satisfied with the current implementation for Mac and iOS.

I filed bug #152457 so we can track that separately.

The curl backend seems to be the only one without these issues -- because it doesn't support DNS prefetch at all.

[Michael Catanzaro]

Created attachment 267679 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 267679 [details]
Patch

Clearing flags on attachment: 267679

Committed r194323: <http://trac.webkit.org/changeset/194323>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.